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

Simple matrix class - version 2

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
versionmatrixsimpleclass

Problem

As suggested on my previous question, I am posting my revised code for feedback.

One thing that caught my attention is that my program actually runs slower after adding the modifications. It was previously running near 128 iterations per second on a single core machine and 382 in a 4-core machine. It now runs at 125 in the single core and 360 in the 4-core machine. I am not sure the way I was managing memory in my first version is faster or if I implemented it wrong.

Grid3d.h:

#ifndef _GRID3D_
#define _GRID3D_

#include 

class Grid3d
{
private:
    size_t M, N, L;
    double* buffer;
    size_t compute_index(size_t, size_t, size_t) const;

public:
    //Constructores
    Grid3d(size_t,size_t,size_t);
    Grid3d();
    Grid3d(const Grid3d&);
    ~Grid3d();

    //Operadores
    double& operator()(size_t, size_t, size_t);
    double  operator()(size_t, size_t, size_t) const;
    Grid3d& operator=(Grid3d);

    //Acceso
    int get_L();
    int get_M();
    int get_N();
    double get(size_t,size_t,size_t) const;
    void reset();
};

std::ostream & operator<< (std::ostream &,Grid3d);
#endif


Grid3d.cc:

```
#include "Grid3d.h"
#include
#include
#include

using namespace std;

//------------------------------------------------------------------
// Constructores
//------------------------------------------------------------------

//Constructor
Grid3d::Grid3d(size_t M, size_t N, size_t L)
: M(M), N(N), L(L), buffer(new double[M N L])
{
for (size_t l=0;l= 0 and i = 0 and j = 0 and k = 0 and i = 0 and j = 0 and k = 0 and i = 0 and j = 0 and k < L);
return buffer[compute_index(i, j, k)];
}

void Grid3d::reset()
{
for (size_t l=0;l<MNL;l++){
buffer[l] = NAN;
}
}

//------------------------------------------------------------------
// Others
//------------------------------------------------------------------

//cout
std::ostream & operator << (std::ostream & os , Grid3d g)
{
int M = g.get_M();
int N = g.get_N();

Solution

I will expand on this when I have more time, but a few things jumped out:

get_M/get_L/get_N should return a std::size_t, not an int.

get_M/get_L/get_N should be const.

It might make sense in your field of study, but to me, NAN is weird for the initial value. I would expect the initial values to be either undefined or 0. Whatever works best for you though. Initializing to NAN would, after all, provide a few convenient properties.

All of the size_t in the header should be std::size_t.

Rather than duplicate the exact same code, the constructor should use reset.

Be careful with using namespace std;, even in implementation files. If you want to read way more than you ever wanted to on it, see this. The "tl;dr" of it is that a potentially application breaking bug might happen years down the road do to a change a dozen files away, but that can all be avoided with 5 extra characters (std::).

When outputting potentially huge chunks of data, try not to flush until you either need to or the end (in single threaded applications, that's almost always going to be the end).

In particular, std::endl is not equivalent to "\n". std::cout << "\n"; pushes a new line into std::couts buffer. std::cout << std::endl; pushes a newline into std::cout's buffer and then flushes the buffer. This means that your operator<< isn't really using buffered IO since it flushes after every output.

Simple fix: use "\n" and then just do std::flush(std::cout); (or std::cout << std::flush) at the end.

cout.width(10); in your operator<< should be os.width(10).

Context

StackExchange Code Review Q#27752, answer score: 7

Revisions (0)

No revisions yet.