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 <iostream>
#include <cmath>
#include <iomanip> // setprecision()
#include <cassert> // 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<L;i++){
G[i] = new double * [M];
for (j=0;j<M;j++){
G[i][j] = new double [N];
for (k=0;k<N;k++){
G[i][j][k] = NAN;
}
}
}
}
//Empty constructor
Grid3d::Grid3d()
:L(0), M(0), N(0)
{
G = NULL;
}
//Copy constructor
Grid3d::Grid3d(const Grid3d &A)
:L(A.L), M(A.M), N(A.N)
{
G = new double ** [L];
int i,j,k;
for (i=0;i<L;i++){
G[i] = new double * [M];
for (j=0;j<M;j++){
G[i][j] = new double [N];
for (k=0;k<N;k++){
G[i][j][k] = A.G[i][j][k];
}
}
}
}
//Destructor
Grid3d::~Grid3d()
{
// Free memory
for (int i=0;i<L;i++){
for (int j=0;j<M;j++){
delete [] G[i][j];
G[i][j] = NULL;
}
delete [] G[i];
G[i] = NULL;
}
delete G;
G = NULL;
}
//------------------------------------------------------------------
// Operators
//------------------------------------------------------------------
//Access operator ():
double& Grid3d::operator()(int i,int j,int k)
{
assert(i >= 0 && i < L);
assert(j >= 0 && j < M);
assert(k >= 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
//------------------------------------------------------------------
//Swap
void Grid3d::swap(Grid3d & A)
{
std::swap(L, A.L);
std::swap(M, A.M);
std::swap(N, A.N);
std::swap(G, A.G);
}
//cout
std::ostream & operator << (std::ostream & os , Grid3d & g)
{
int L = g.get_L();
int M = g.get_M();
int N = g.get_N();
for (int k=0;k<N;k++){
cout << "k = " << k << endl;
for (int i=0;i<L;i++){
for (int j=0;j<M;j++){
cout.width(10);
os << setprecision(3) << g.G[i][j][k] << " ";
}
os << endl;
}
os << endl;
}
return os;
}
This is a sample function in which I am using these objects to evaluate the curl of a vector field in a point:
double System::curl_r(Grid3d &Ath,Grid3d &Aph,int i,int j,int k)
{
double curl_rA = ( (SIN[j+1]*Aph(i,j+1,k)-SIN[j-1]*Aph(i,j-1,k))/dth - (Ath(i,j,k+1)-Ath(i,j,k-1))/dph )/(2.0e0*R[i]*SIN[j]);
return curl_rA;
}
For example:
Grid3d Er = Grid3d(Nr,Nth,Nph);
Grid3d Eth = Grid3d(Nr,Nth,Nph);
Grid3d Eph = Grid3d(Nr,Nth,Nph);
Grid3d curlE_r = Grid3d(Nr,Nth,Nph);
...
curlE_r(i,j,k) = curl_r(Eth,Eph,i,j,k);
delete G
; should bedelete[] G;
since you allocated the contents ofG
withnew[]
. \$\endgroup\$