HiveBrain v1.2.0
Get Started
← Back to all entries
snippetcppMinor

Read matrix from file, the C++11/14 way

Submitted by: @import:stackexchange-codereview··
0
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 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 3


the code would build the matrix

0 0 0 1
0 0 2 3
0 0 0 1
0 4 0 1

Solution

I see some things that may help you improve your code.

Use the required #includes

The 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 function

Rather 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                      retq


This 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                      retq


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

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.