5
\$\begingroup\$

The problem is to print an N*N array and fill it with numbers(starting from 1 at (0,0)) in the rectangular/circular/spiral fashion, going inwards, ending at the N*N value.

Also find and count the indices where the values are divisible by 11, in the circular fashion only.[(0,0) will always be an index]

What improvements can I make in the following code?

#include<iostream>
#include<cstdlib> 

class arr{
   int value;
   bool property;
public:
   arr(){
       property=false;
   }
   void put_value(int a,bool b){
       value = a;
       property = b;
   }
   int get_value(){
       return value;
   }
   bool get_property(){
        return property;
   }
};
int count = 1;
int counter(){
    return count++;
}
int c=0;
int t(){
   return c++;
}
class first{
  int a,b;
  bool flag;
  public: 
     first(){flag=false;}
     void put_value(int x, int y, bool m){
         flag = true;
         a = x ;
         b = y;
     }
    int get_property(){return flag;}
    int get_a(){return a;}
    int get_b(){return b;}
};
int fill(int a, int b,int dimension ,arr** array,first cls[],int temporary){
   int i=0,j=0,k=0,l=0;
   int f=t();
   int put;
     for(i = b ; i < dimension ; i++){
       if(array[a][i].get_property()==false){
         put = counter();
         array[a][i].put_value(put,true); 

       if(!(put%11))
         { cls[temporary].put_value(a,i,true);
              temporary++;
         }
         }
       }
     i--;
     for(j = a + 1 ; j < dimension  ; j++ ){
       if(array[j][i].get_property()==false){
         put = counter();
         array[j][i].put_value(put,true);


     if(!(put%11))
         { cls[temporary].put_value(j,i,true);
              temporary++;
         }
     }
     }
     j--;
     for(k = i - 1 ; k>=f; k-- ){
      if(array[j][k].get_property()==false){
         put = counter();
         array[j][k].put_value(put,true);


      if(!(put%11))
         { cls[temporary].put_value(j,k,true);
              temporary++;
         }
      }
      }
    i=k+1;
    for(l = j-1 ;l>=f ; l--){
       if(array[l][i].get_property()==false){
         put = counter();
         array[l][i].put_value(put,true);


       if(!(put%11))
         { cls[temporary].put_value(l,i,true); 
              temporary++;
         }
       } 
    }
    return temporary;
};
int main(){
    int dimension;
    std::cin>>dimension;
    arr** array = new arr*[dimension];
    for(int i=0;i<dimension;i++)
         array[i] = new arr[dimension];  
   int temp=dimension;
   first *kay = new first[dimension*dimension];
   int temporary = 0;
   for(int i=0;i<temp;i++){
     for(int j=0;j<temp;j++)
       {
          if(i==j){
             temporary = fill(i,j,temp,array,kay,temporary);
             temp--;
          }
       } 
   }
   for(int i=0;i<dimension;i++){
     for(int j=0;j<dimension;j++)
       {
       std::cout.width(5);
       std::cout<<array[i][j].get_value()<<"   ";
       } 
     std::cout<<"\n";
   }
   std::cout<<"Total Power points : "<<temporary+1<<"\n";
   std::cout<<"(0,0)"<<"\n";
 int q;  for(q = 0;q<temporary;q++)
 { std::cout<<"("<<kay[q].get_a()<<","<<kay[q].get_b()<<")\n";
}return 0;
}

A sample output : Run the program. Enter 5 as N.

1       2       3       4        5
16      17      18      19       6
15      24      25      20       7
14      23      22      21       8
13      12      11      10       9

Total Power points : 3 
(0,0)
(4,2)
(3,2)

For N=20

 1       2       3       4       5       6       7       8       9      10      11      12      13      14      15      16      17      18      19      20
76      77      78      79      80      81      82      83      84      85      86      87      88      89      90      91      92      93      94      21
75     144     145     146     147     148     149     150     151     152     153     154     155     156     157     158     159     160      95      22
74     143     204     205     206     207     208     209     210     211     212     213     214     215     216     217     218     161      96      23
73     142     203     256     257     258     259     260     261     262     263     264     265     266     267     268     219     162      97      24
72     141     202     255     300     301     302     303     304     305     306     307     308     309     310     269     220     163      98      25
71     140     201     254     299     336     337     338     339     340     341     342     343     344     311     270     221     164      99      26
70     139     200     253     298     335     364     365     366     367     368     369     370     345     312     271     222     165     100      27
69     138     199     252     297     334     363     384     385     386     387     388     371     346     313     272     223     166     101      28
68     137     198     251     296     333     362     383     396     397     398     389     372     347     314     273     224     167     102      29
67     136     197     250     295     332     361     382     395     400     399     390     373     348     315     274     225     168     103      30
66     135     196     249     294     331     360     381     394     393     392     391     374     349     316     275     226     169     104      31
65     134     195     248     293     330     359     380     379     378     377     376     375     350     317     276     227     170     105      32
64     133     194     247     292     329     358     357     356     355     354     353     352     351     318     277     228     171     106      33
63     132     193     246     291     328     327     326     325     324     323     322     321     320     319     278     229     172     107      34
62     131     192     245     290     289     288     287     286     285     284     283     282     281     280     279     230     173     108      35
61     130     191     244     243     242     241     240     239     238     237     236     235     234     233     232     231     174     109      36
60     129     190     189     188     187     186     185     184     183     182     181     180     179     178     177     176     175     110      37
59     128     127     126     125     124     123     122     121     120     119     118     117     116     115     114     113     112     111      38
58      57      56      55      54      53      52      51      50      49      48      47      46      45      44      43      42      41      40      39

Total Power points : 37
(0,0)
(0,10)
(2,19)
(13,19)
(19,14)
(19,3)
(11,0)
(1,1)
(1,12)
(6,18)
(17,18)
(18,8)
(14,1)
(3,1)
(2,11)
(7,17)
(17,16)
(17,5)
(9,2)
(3,7)
(5,16)
(16,16)
(16,5)
(7,3)
(4,11)
(11,15)
(15,8)
(8,4)
(5,12)
(14,14)
(12,5)
(6,10)
(13,12)
(8,6)
(11,12)
(8,8)
(9,8)
\$\endgroup\$

2 Answers 2

4
\$\begingroup\$

Global Variables
The code contains the variables count and c declared as global variables. Global variables are generally frowned upon.
1. As programs get more complex they are created in multiple files, global variables are very difficult to find when used in multiple files.
2. Using global variables makes it harder to maintain the code.
3. Using global variables makes it harder to debug any problems.
4. Using global variables may prevent adding external libraries.

Variable Initialization in Constructors
In both of the classes the variables are initialized within the body of the constructor. The more accepted way to do this is this way:

first::first()
: flag{false}
{
}

arr::arr()
: property{false}
{
}

Use Descriptive Variable Names
@D.Jurcau point about variable and function names is correct.

The function fill() has a fairly meaningful name, it is filling a matrix. The function t() isn't clear at all.

The code should always be as self documenting as possible. This means that the variable and function names should be as descriptive as possible. If you or someone needs to come back and modify the code in 2 or 3 years the code needs to be instantly understandable.

Variable names like i, j, and k can be used in for loops, but it would make more sense to name them so that the algorithm is clear.

DRY Code
DRY stands for Don't Repeat Yourself. This is another item where D.Jurcau is correct. One of the ways to correct this is to put code into functions, another would be to use loops, which is already done.

Reducing repetition makes the code easier to write, debug, and maintain. Use the standard library and container classes whenever possible to reduce code.

Use the Single Responsibility Principal
When designing functions and objects (classes and structs) reduce the complexity of the functions by dividing large complex functions into smaller functions. This is called the Single Responsibility Principal.

Both the fill() function and main() are too complex.

Each of the loops in the fill() function should probably be separate functions. It is also unclear what each loop is doing in the fill() function. Having each loop be a function would better document what each loop is doing.

The purpose of the main() function is to set up the environment to run the program, execute the program and catch any exceptions, all other code such as input, output and actual execution should be in other functions. The actual logic for running the program should be in another function.

Additional Topics to Research
Knowing good programming principals is a good thing, additional things to research would be SOLID and KISS.

\$\endgroup\$
3
\$\begingroup\$

Inappropriate names

The names you used make your code almost unreadable: what is a class called arr or first supposed to represent? What should a function called t do?

Wrong use of OOP

Your arr class seems only to be encapsulating two variables. Why did you choose to create a class with only trivial getters/setters instead on simply creating a struct with two fields?

The same seems to be the case for first, which also seems to request a boolean parameter in its constructor which is never actually used.

Duplicate code

Your main method, fill contains repeats the for block 4 times, with only minor changes Find a way to reduce this repetition.

Code formatting

Try and use a consistent formatting style. You're placing brackets all over, sometimes even mix multiple statements on the same line. White-space is also often times in short supply (variables and operators are tightened together) and a few times in excess.

\$\endgroup\$
1
  • \$\begingroup\$ I had blanked out how to create 2d dynamic structure in c++ when I sat down for solving the problem. I could have extended the arr class to implement first though. I will find a way to make fill more concise. \$\endgroup\$
    – piepi
    Commented Aug 1, 2016 at 13:07

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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