5
\$\begingroup\$

I went ahead and quickly typed this up and am unsure on how to improve. How could this be optimized? Note: there are some functions from a custom library here that do exactly what they sound like they do (e.g. make_window or fill_rectangle).

struct block{
    int a, b;

    block(){}

    block(int first, int second)
    {a = first; b = second;}

    int getVal(int x){
        if(x==1)
            return a;
        else 
            return b;
    }
};

struct squares{
    int x, y;
    int neighbors;
    int freeSpots;
    block** ary;
    int rows, cols;

    squares(int row, int col){
        //ary = new int*[row];
        ary = new block*[row];
        for(int i = 0; i < row; ++i)
            ary[i] = new block[col];
        rows = row; cols = col;

        for(int i = 0; i < row; i++){
            for(int k = 0; k < col; k++){
                ary[i][k].a=0;
                ary[i][k].b=0;
            }
        }
    }

    void clearArr(int which){
        cout<<"clear arr"<< endl;
        if(which == 0){
            for(int i = 0; i < rows; i++){
                for(int k = 0; k < cols; k++){
                    ary[i][k].a=0;
                }
            }
        }else{
            for(int i = 0; i < rows; i++){
                for(int k = 0; k < cols; k++){
                    ary[i][k].b=0;
                }
            }
        }
    }

    void placeSquares(){
        set_pen_color(color::black);
        while(1){
            wait_for_mouse_click();
            int xClick = get_click_x();
            int yClick = get_click_y();
            int x = xClick/10;
            int y = yClick/10;
            cout<< x << " " << y << endl;
            ary[x][y].a = 1;
            fill_rectangle(x*10,y*10,10,10);
            if(xClick >= 450)
                break;
        }
    }

    int neighborCount(int i, int k){
        int neighbors = 0;
            if( i>=1 && k >=1 && k<49 && i<49){
                if(ary[i+1][k].a == 1)
                    neighbors += 1;
                if(ary[i-1][k].a == 1)
                    neighbors += 1;
                if(ary[i][k+1].a == 1)
                    neighbors += 1;
                if(ary[i][k-1].a == 1)
                    neighbors += 1;
                if(ary[i+1][k+1].a == 1)
                    neighbors += 1;
                if(ary[i+1][k-1].a == 1)
                    neighbors += 1;
                if(ary[i-1][k+1].a == 1)
                    neighbors += 1;
                if(ary[i-1][k-1].a == 1)
                    neighbors += 1;
            }

        return neighbors;
    }

    void redraw(){
        while(1){
        for(int i = 0; i < rows; i++){
            for(int k = 0; k < cols; k++){
                int n = neighborCount(i,k);
                if(ary[i][k].a == 1){

                    if(n<2 || n>3)              // if less than two or more than three neighbors => death
                    {cout<< "kill"<<endl; ary[i][k].b = 0;}
                    if(n==2 || n ==3)
                        ary[i][k].b = 1;
                }else if(ary[i][k].a == 0){
                    if(n == 3)                  // if three neighbors, respawn
                        ary[i][k].b = 1;
                }
            }
        }

        // redraw
        for(int i = 0; i < rows; i++){

            for(int k = 0; k < cols; k++){
                if(ary[i][k].b == 0){
                    fill_rectangle(10*i,10*k,10,10,color::white);
                }else{
                    fill_rectangle(i*10,k*10,10,10, color::black);
                }
             }

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

        } // end while
  }
};

squares createWindow(){
    int row, col;
    cout << "Enter the number of rows followed by the number of columns desired." << endl; 
    cin >> row >> col;
    make_window(row*10,col*10);

    for(int i = 0; i < row; i++){
        move_to(0,i*10);
        draw_to(col*10,i*10);
    }

    for(int i = 0; i < col; i++){
        move_to(i*10, 0);
        draw_to(i*10,row*10);
    }

    squares sq(row, col);
    return sq;
}


void main(){

    squares sq = createWindow();
    sq.placeSquares();
        sq.redraw();

}
\$\endgroup\$
6
\$\begingroup\$

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;

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;
       }
    }
    
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.