patterncppMinor
Simple matrix class - version 2
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:
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();
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);
#endifGrid3d.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:
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
Rather than duplicate the exact same code, the constructor should use
Be careful with
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
Simple fix: use "\n" and then just do
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.