Simple matrix class

I have built a 3D matrix class in C++, mainly for data I/O, for a project I am working on. The code works ok, but I would like you to help me improve it.

• Are there any bad programming practices I am doing?

• Is there any code that could be simplified?

• Is there something that would give me fastest access to the data stored in the object? 4. Is there something that could lead me to error in the future?

Grid3d.h:

#ifndef _GRID3D_
#define _GRID3D_

#include 
#include 
#include   // setprecision()
#include   // assert()

using namespace std;

class Grid3d
{
private:
    int L;
    int M;
    int N;
    double *** G;

public:
    //Constructors
    Grid3d(int,int,int);
    Grid3d();
    Grid3d(const Grid3d &);
    ~Grid3d();

    //Operators
    double & operator()(int,int,int);
    Grid3d & operator=(Grid3d);

    //Access
    int get_L();
    int get_M();
    int get_N();
    double get(int,int,int);
    void clean();

    void swap(Grid3d &);
    friend std::ostream & operator<< (std::ostream &,Grid3d &);
};

#endif

Grid3d.cc:

```
#include "Grid3d.h"

//Constructor
Grid3d::Grid3d(int L,int M,int N)
:L(L), M(M), N(N)
{
int i,j,k;
G = new double ** [L];
for (i=0;i= 0 && i = 0 && j = 0 && k < N);
return G[i][j][k];
}

//Assignment operator
Grid3d& Grid3d::operator = (Grid3d A)
{
swap(A);
return *this;
}

//------------------------------------------------------------------
// Access
//------------------------------------------------------------------

int Grid3d::get_L()
{
return L;
}

int Grid3d::get_M()
{
return M;
}

int Grid3d::get_N()
{
return N;
}

double Grid3d::get(int i,int j,int k)
{
return G[i][j][k];
}

void Grid3d::clean()
{
int i,j,k;
for (i=0;i<L;i++){
for (j=0;j<M;j++){
for (k=0;k<N;k++){
G[i][j][k] = NAN;
}
}
}
}

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

We can improve this code.

1: First of all, I urge you to change your underlying data structure. double *** G; hurts my eyes. Consider using for example std::vector > > instead. (If you are using C++11, you can drop the spaces. Yay!) Using std::vector has a plethora of advantages. The two most important are that it will be easier to get the code to work correctly, the other is readability.

If for some masochistic reason you don't want to use std::vector or a similar type, at least use arrays instead of raw pointers.

2: L, M, N and G are horrific variable names. Consider using for example width, length, depth and matrix -- or something along those lines -- instead. The same goes for your getter functions.

3: Your output stream operator (operatorYou probably want to use operator[] instead of operator() for indexing operations. When in doubt, do as the built-ins do, and arrays use foo[42] for indexing. Update: For multiple indices, it's perfectly okay to use operator().

9: Use as few #includes as possible in your header file. Move what you can into your implementation file. After removing the friend declaration (as discussed in point 3), you can move all your #includes to your implementation file.

10: Don't ever have using statements or using directives in a header file. Any file #includeing your header will have its global namespace polluted. You don't want that. Use full namespace specifiers in header files. You are already doing this, so the using` directive can safely be removed.

There are some other issues, but these are the most important ones. They should give you something to work on. I suggest you create a new thread with your improved code after incorporating these changes, for further feedback.

StackExchange Code Review Q#27573, answer score: 9