snippetcppMinor
Read matrix from file, the C++11/14 way
Viewed 0 times
filethereadwayfrommatrix
Problem
I am trying to get acquainted with C++11/14, so please tell me if the code below could be written in a more "modern" way. Or, of course, if it could be improved in any way.
The function
The function builds the input for a Sokoban game from a file. Basically, it builds a "map" (a matrix) with several types of objects placed on it.
The file has the following format:
For example, for the following file content:
the code would build the matrix
The function
buildInput reads the content of a file and builds a matrix with it.std::vector> buildInput(std::string fileName) {
auto rows = 0, cols = 0;
auto objectsCount = 0;
std::ifstream in(fileName);
// Throw exceptions in case any i/o operation fails.
in.exceptions(std::ifstream::failbit | std::ifstream::badbit);
in >> rows >> cols >> objectsCount;
std::vector> input(rows, std::vector(cols, 0));
for(int i = 0; i > type >> row >> col;
input[row][col] = type;
}
return input;
}The function builds the input for a Sokoban game from a file. Basically, it builds a "map" (a matrix) with several types of objects placed on it.
The file has the following format:
rows_count columns_count objects_count
[object_type object_row_number object_column_number](objects_count times)For example, for the following file content:
4 4 6
1 0 3
2 1 2
3 1 3
1 2 3
4 3 1
1 3 3the code would build the matrix
0 0 0 1
0 0 2 3
0 0 0 1
0 4 0 1Solution
I see some things that may help you improve your code.
Use the required
The code uses
Think of the user
The provided example input requires 21 numbers so that the computer can create a matrix containing just 16. There is a lot of redundancy there! It seems to me that simply having your example output be the input format would make a lot more sense and require less space, less typing and less interpretation.
Sanitize user input better
If I change the last line of the sample input to
Pass a stream rather than a filename
The current design is rather inflexible in that it can only use an actual file as input. I'd recommend making it more generic and accepting a
Use a better data structure
The choice of a vector of vectors for a matrix is not a very good one because unlike a matrix, there is nothing to assure that all of the vectors are the same size. I'd recommend creating your own custom object which hides the messy details of the actual storage (it might be a
Use a
Rather than having a separate function, it would be nicer to be able to write code something like this:
Alternatively,
To do this, we could write an extractor. Here's a sketch of an example:
A word about addressing individual squares
A question in the comments was whether doing the mathematics (multiplication) might be longer than the addressing of a vector of vectors. On my 64-bit Linux machine using gcc 6.2.1 with -O3 optimization, I coded two different versions.
First, here is the test source code:
Note that this is a poor program lacking all error checking, but the purpose was only to extract the assembly language for the two versions of
produced this assembly code
This version
produced this assembly code
I was not able to detect any timing differences with the testing I did, but we can see that the difference is that there are 5 memory fetches for the vector of vectors version, versus th
Use the required
#includesThe code uses
std::vector which means that it should #include . It was not difficult to infer, but it helps reviewers if the code is complete. The full set of required includes appears to be this:#include
#include
#include Think of the user
The provided example input requires 21 numbers so that the computer can create a matrix containing just 16. There is a lot of redundancy there! It seems to me that simply having your example output be the input format would make a lot more sense and require less space, less typing and less interpretation.
Sanitize user input better
If I change the last line of the sample input to
1 9 9 it is asking the code to initialize a location that is outside the bounds of the vector and on my machine the computer segfaults and the program dies. Some bounds checking would be prudent.Pass a stream rather than a filename
The current design is rather inflexible in that it can only use an actual file as input. I'd recommend making it more generic and accepting a
std::istream & as a parameter rather than a file name.Use a better data structure
The choice of a vector of vectors for a matrix is not a very good one because unlike a matrix, there is nothing to assure that all of the vectors are the same size. I'd recommend creating your own custom object which hides the messy details of the actual storage (it might be a
std::array or a single std::vector) but could do bounds checking and retrieval using the same coordinate system. An example is shown below.Use a
friend extractor instead of a standalone functionRather than having a separate function, it would be nicer to be able to write code something like this:
SokobanBoard board;
std::cin >> board;Alternatively,
SokobanBoard board;
ifstream in(fileName);
in >> board;To do this, we could write an extractor. Here's a sketch of an example:
class SokobanBoard {
public:
SokobanBoard(std::size_t rows, std::size_t cols);
int at(std::size_t x, std::size_t y) const;
friend std::ostream& operator>(std::istream& in, SokobanBoard &sb);
private:
std::size_t m_height;
std::size_t m_width;
std::vector m_board;
};A word about addressing individual squares
A question in the comments was whether doing the mathematics (multiplication) might be longer than the addressing of a vector of vectors. On my 64-bit Linux machine using gcc 6.2.1 with -O3 optimization, I coded two different versions.
First, here is the test source code:
#include
#include
class Test1 {
public:
Test1(int rows, int cols) :
vec(rows)
{
for (auto &row: vec) {
row.reserve(cols);
}
}
int fetch(int x, int y) const;
private:
std::vector> vec;
};
class Test2 {
public:
Test2(int rows, int cols) :
height{rows},
width{cols},
vec(height * width)
{}
int fetch(int x, int y) const;
private:
const size_t height;
const size_t width;
std::vector vec;
};
int Test1::fetch(int x, int y) const { return vec[x][y]; }
int Test2::fetch(int x, int y) const { return vec[x+y*height]; }
int main() {
std::size_t rows, cols;
std::cin >> rows >> cols;
Test1 test1(rows, cols);
Test2 test2(rows, cols);
std::size_t x, y;
std::cin >> x >> y;
int m = test1.fetch(x, y);
int n = test2.fetch(x, y);
std::cout << "m = " << m << ", and n = " << n << "\n";
}Note that this is a poor program lacking all error checking, but the purpose was only to extract the assembly language for the two versions of
fetch. First, this version int Test1::fetch(int x, int y) const { return vec[x][y]; }produced this assembly code
0000000000000000 :
Test1::fetch(int, int) const():
0: 48 63 f6 movslq %esi,%rsi
3: 48 8b 0f mov (%rdi),%rcx
6: 48 63 d2 movslq %edx,%rdx
9: 48 8d 04 76 lea (%rsi,%rsi,2),%rax
d: 48 8d 04 c1 lea (%rcx,%rax,8),%rax
11: 48 8b 00 mov (%rax),%rax
14: 8b 04 90 mov (%rax,%rdx,4),%eax
17: c3 retqThis version
int Test2::fetch(int x, int y) const { return vec[x+y*height]; }produced this assembly code
0000000000000020 :
Test2::fetch(int, int) const():
20: 48 63 d2 movslq %edx,%rdx
23: 48 8b 47 10 mov 0x10(%rdi),%rax
27: 48 63 f6 movslq %esi,%rsi
2a: 48 0f af 17 imul (%rdi),%rdx
2e: 48 01 d6 add %rdx,%rsi
31: 8b 04 b0 mov (%rax,%rsi,4),%eax
34: c3 retqI was not able to detect any timing differences with the testing I did, but we can see that the difference is that there are 5 memory fetches for the vector of vectors version, versus th
Code Snippets
#include <iostream>
#include <fstream>
#include <vector>SokobanBoard board;
std::cin >> board;SokobanBoard board;
ifstream in(fileName);
in >> board;class SokobanBoard {
public:
SokobanBoard(std::size_t rows, std::size_t cols);
int at(std::size_t x, std::size_t y) const;
friend std::ostream& operator<<(std::ostream& out, const SokobanBoard &sb);
friend std::istream& operator>>(std::istream& in, SokobanBoard &sb);
private:
std::size_t m_height;
std::size_t m_width;
std::vector<int> m_board;
};#include <vector>
#include <iostream>
class Test1 {
public:
Test1(int rows, int cols) :
vec(rows)
{
for (auto &row: vec) {
row.reserve(cols);
}
}
int fetch(int x, int y) const;
private:
std::vector<std::vector<int>> vec;
};
class Test2 {
public:
Test2(int rows, int cols) :
height{rows},
width{cols},
vec(height * width)
{}
int fetch(int x, int y) const;
private:
const size_t height;
const size_t width;
std::vector<int> vec;
};
int Test1::fetch(int x, int y) const { return vec[x][y]; }
int Test2::fetch(int x, int y) const { return vec[x+y*height]; }
int main() {
std::size_t rows, cols;
std::cin >> rows >> cols;
Test1 test1(rows, cols);
Test2 test2(rows, cols);
std::size_t x, y;
std::cin >> x >> y;
int m = test1.fetch(x, y);
int n = test2.fetch(x, y);
std::cout << "m = " << m << ", and n = " << n << "\n";
}Context
StackExchange Code Review Q#150390, answer score: 8
Revisions (0)
No revisions yet.