Skip to main content
Code Review

Return to Answer

Fixed an error
Source Link
R Sahu
R Sahu
  • 3.5k
  • 12
  • 20

  1. You can remove the member variables rows and cols.

  2. Implementation of the constructor can be simplified to:

     squares(int row, int col) : blockArray(row, std::vector<block>(col, block())) {}

  3. You get automatic memory cleanup. In your posted code, you don't have a user defined destructor. As a result, you have a memory leak. By using std::vectors, the default destructor provided by the compiler will take care of releasing memory used by the std::vectors.

  4. It is easier to iterate over the array. You can replace the following block:

     for(int i = 0; i < rows; i++){
     for(int k = 0; k < cols; k++){
         ary[i][k].a=0;
     }
 }

  1. You can remove the member variables rows and cols.

  2. Implementation of the constructor can be simplified to:

     squares(int row, int col) : blockArray(row, std::vector<block>(col, block)) {}

  3. You get automatic memory cleanup. In your posted code, you don't have a user defined destructor. As a result, you have a memory leak. By using std::vectors, the default destructor provided by the compiler will take care of releasing memory used by the std::vectors.

  4. It is easier to iterate over the array. You can replace the following block:

     for(int i = 0; i < rows; i++){
     for(int k = 0; k < cols; k++){
         ary[i][k].a=0;
     }
 }

  1. You can remove the member variables rows and cols.

  2. Implementation of the constructor can be simplified to:

     squares(int row, int col) : blockArray(row, std::vector<block>(col, block())) {}

  3. You get automatic memory cleanup. In your posted code, you don't have a user defined destructor. As a result, you have a memory leak. By using std::vectors, the default destructor provided by the compiler will take care of releasing memory used by the std::vectors.

  4. It is easier to iterate over the array. You can replace the following block:

     for(int i = 0; i < rows; i++){
     for(int k = 0; k < cols; k++){
         ary[i][k].a=0;
     }
 }
Refined the answer
Source Link
R Sahu
R Sahu
  • 3.5k
  • 12
  • 20

If you useBenefits of using a std::vector<std::vector<block>>, you can remove the member variables rows and cols. The constructor of squares can be simplified to:

squares(int row, int col) : blockArray(row, std::vector<block>(col, block)) {}

  1. You can remove the member variables rows and cols.

  2. Implementation of the constructor can be simplified to:

     squares(int row, int col) : blockArray(row, std::vector<block>(col, block)) {}

  3. You get automatic memory cleanup. In your posted code, you don't have a user defined destructor. As a result, you have a memory leak. By using std::vectors, the default destructor provided by the compiler will take care of releasing memory used by the std::vectors.

  4. It is easier to iterate over the array. You can replace the following block:

     for(int i = 0; i < rows; i++){
     for(int k = 0; k < cols; k++){
         ary[i][k].a=0;
     }
 }

In your posted code, you don't have a user defined destructor. As a result, you have a memory leak. By using std::vectors, the default destructor provided by the compiler will take care of releasing memory used by the std::vectors.

The other benefit of using std::vectors is ease of iteration. You can replace the following block:

for(int i = 0; i < rows; i++){
    for(int k = 0; k < cols; k++){
        ary[i][k].a=0;
    }
}

by

for ( auto& row : blockArray) {
       for ( auto& block : row ) {
          block.a = 0;
       }
    }

If you use a std::vector<std::vector<block>>, you can remove the member variables rows and cols. The constructor of squares can be simplified to:

squares(int row, int col) : blockArray(row, std::vector<block>(col, block)) {}

In your posted code, you don't have a user defined destructor. As a result, you have a memory leak. By using std::vectors, the default destructor provided by the compiler will take care of releasing memory used by the std::vectors.

The other benefit of using std::vectors is ease of iteration. You can replace the following block:

for(int i = 0; i < rows; i++){
    for(int k = 0; k < cols; k++){
        ary[i][k].a=0;
    }
}

by

for ( auto& row : blockArray) {
   for ( auto& block : row ) {
      block.a = 0;
   }
}

Benefits of using a std::vector<std::vector<block>>:

  1. You can remove the member variables rows and cols.

  2. Implementation of the constructor can be simplified to:

     squares(int row, int col) : blockArray(row, std::vector<block>(col, block)) {}

  3. You get automatic memory cleanup. In your posted code, you don't have a user defined destructor. As a result, you have a memory leak. By using std::vectors, the default destructor provided by the compiler will take care of releasing memory used by the std::vectors.

  4. It is easier to iterate over the array. You can replace the following block:

     for(int i = 0; i < rows; i++){
     for(int k = 0; k < cols; k++){
         ary[i][k].a=0;
     }
 }

by

    for ( auto& row : blockArray) {
       for ( auto& block : row ) {
          block.a = 0;
       }
    }
Source Link
R Sahu
R Sahu
  • 3.5k
  • 12
  • 20

Remove unused functions and member variables

block::getValue() is not used at all.

squares::freeSpots is not used.

squares::neighbors is not used. There is a function local variable of the same name in squares::neighborCount but the class member variable is not used at all.

squares::x is not used. There is a function local variable of the same name in squares::placeSquares but the class member variable is not used at all.

squares::y is not used. There is a function local variable of the same name in squares::placeSquares but the class member variable is not used at all.

Replace hard coded numbers with suitably named constants

You have:

if( i>=1 && k >=1 && k<49 && i<49){

That would be more readable if you use:

const int ABCD_LIMIT = 49;
if( i>=1 && k >=1 && k<ABCD_LIMIT && i<ABCD_LIMIT){

Other examples of hard coded numbers:

if(xClick >= 450)

fill_rectangle(10*i,10*k,10,10,color::white);

fill_rectangle(i*10,k*10,10,10, color::black);

make_window(row*10,col*10);

move_to(0,i*10);
draw_to(col*10,i*10);

move_to(i*10, 0);
draw_to(i*10,row*10);

Replace use of pointers with std::vectors

You have

block** ary;

That can easily be

std::vector<std::vector<block>> ary;

I would also recommend changing the name ary to blockArray. A more descriptive name is better than a cryptic one.

std::vector<std::vector<block>> blockArray;

If you use a std::vector<std::vector<block>>, you can remove the member variables rows and cols. The constructor of squares can be simplified to:

squares(int row, int col) : blockArray(row, std::vector<block>(col, block)) {}

In your posted code, you don't have a user defined destructor. As a result, you have a memory leak. By using std::vectors, the default destructor provided by the compiler will take care of releasing memory used by the std::vectors.

The other benefit of using std::vectors is ease of iteration. You can replace the following block:

for(int i = 0; i < rows; i++){
    for(int k = 0; k < cols; k++){
        ary[i][k].a=0;
    }
}

by

for ( auto& row : blockArray) {
   for ( auto& block : row ) {
      block.a = 0;
   }
}