An Opinion About My Code

[Mijojo]

(First of all i would just like to say that i'm new to the community and i'm also brazillian so i apologize for any mistakes i may commit grammatically)

This is the assignment:

5.23) Write a program that prints the following diamond shape. You may use output statements that print a single asterisk (*), a single blank or a single newline. Maximize your use of repetition (with nested for statements) and minimize the number of output statements.

That's how i did it:

Code: Select all

#include <iostream>

using namespace std;   

int main()
{
   int blanks_less = 4;
   int blanks_plus = 1;
   int astrks_less = 7;
   int astrks_plus = 1;

   for (int y = 1; y <= 9; y++)
   {
      if (y <= 4)
      {
         for (int x = 1; x <= blanks_less; x++)
            cout << ' ';
            
         blanks_less--;
         
         for (int a = 1; a <= astrks_plus; a++)
            cout << '*';

         astrks_plus += 2;

         cout << endl;
      }

      if (y == 5)
      {
         for (int a = 1; a <= 9; a++)
            cout <<'*';

         cout << endl;
      }

      if (y >= 6)
      {
         for (int x = 1; x <= blanks_plus; x++)
            cout << ' ';

         blanks_plus++;

         for (int a = 1; a <= astrks_less; a++)
            cout << '*';

         astrks_less -= 2;

         cout << endl;
      }
   }

   char wait;
   cin >> wait;
}


Then the next assignment:

5.24) Modify Exercise 5.23 to read an odd number in the range 1 to 19 to specify the number of rows in the diamond, then display a diamond of the appropriate size.

And i did it like this:

Code: Select all

#include <iostream>

using namespace std;   

int main()
{
   int rows;

   cout << "Enter the number of rows: ";
   cin >> rows;

   int blanks_less = rows - static_cast<int>(ceil(rows/2.0));
   int blanks_plus = 1;
   int astrks_less = rows - 2;
   int astrks_plus = 1;

   for (int y = 1; y <= rows; y++)
   {
      if (y <= static_cast<int>(floor(rows/2.0)))
      {
         for (int x = 1; x <= blanks_less; x++)
            cout << ' ';
            
         blanks_less--;
         
         for (int a = 1; a <= astrks_plus; a++)
            cout << '*';

         astrks_plus += 2;

         cout << endl;
      }

      if (y == static_cast<int>(ceil(rows/2.0)))
      {
         for (int a = 1; a <= rows; a++)
            cout <<'*';

         cout << endl;
      }

      if (y >= static_cast<int>(ceil(rows/2.0) + 1.0))
      {
         for (int x = 1; x <= blanks_plus; x++)
            cout << ' ';

         blanks_plus++;

         for (int a = 1; a <= astrks_less; a++)
            cout << '*';

         astrks_less -= 2;

         cout << endl;
      }
   }

   char wait;
   cin >> wait;
}


What you guys think about it? I mean, it did what it was supposed to do, but is it ugly? I mean, could i have made it cleaner, simpler? Thanks in advance.

[exomo]

I'll give you some comment on the second code, most of them can somehow be applied to the first one.

Code: Select all

int blanks_less = rows - static_cast<int>(ceil(rows/2.0));

If you want to use maths functions you should include the math.h header. But you don't need floating arithmetics here. You can just use an integer division:

Code: Select all

int blanks_less = rows / 2;

The conditions in you if statements can also use integer division:

Code: Select all

if (y <= rows / 2)

and the others alike. But I wouldn't use ifs anyway.
The logical structure of your program is ok, there's nothing where I would say "you can't do it that way", but still I would do some things different.
Instead of a single outer loop and the ifs, you could just use two loops, the first for increasing asterisks and decreasing spaces and the second for the other way.

Code: Select all

   int blanks_less = rows - static_cast<int>(ceil(rows/2.0));
   int blanks_plus = 1;
   int astrks_less = rows - 2;
   int astrks_plus = 1;

You don't need four variables, you can do it with two. You can use the same variable for increasing and decreasing.
Or you could use the single loop with ifs, but separate the logics (how many spaces...) from the actual output. Use one variable for asterisk and one for space count, increase and decrease them as required (inside the ifs) and place the print loops at the end of the outer loop.
Another thing you could do is place the output loops inside a function (if you are allowed to).
Just some suggestions, there are allways many ways to solve a problem and there is not always one perfect solution, so my solution is just different.

Code: Select all

#include <iostream>

using namespace std;

void printChars(char c, int n)
{
    for (int x = 1; x <= n; x++) cout << c;
}

int main()
{
   int rows;

   cout << "Enter the number of rows: ";
   cin >> rows;

   int blanks = rows / 2 + 1;
   int astrks = -1;

   // print the upper half of the diamond
   for (int y = 1; y <= rows/2+1; y++)
   {
         blanks--;
         printChars(' ', blanks);

         astrks += 2;
         printChars('*', astrks);

         cout << endl;
   }

   // print the lower half of the diamond
   for (int y = rows/2+2; y <=rows ; y++)
   {
         blanks++;
         printChars(' ', blanks);

         astrks -= 2;
         printChars('*', astrks);

         cout << endl;
   }
}

PS: I like minimizing code, so I made a even smaller version of the program that doesn't use the counting variables at all, a single outer loop, and only a single output statement in a function. This is not what I consider a good solution because it is hard read, but it works.

Code: Select all

#include <iostream>

using namespace std;

void printChars(char c, int n)
{
    for (int x = 1; x <= n; x++) cout << c;
}

int main()
{
   int rows;

   cout << "Enter the number of rows: ";
   cin >> rows;

   for (int y = 0; y < rows; y++)
   {
         printChars(' ', y<rows/2?rows/2-y:y-rows/2);
         printChars('*', y<rows/2?y*2+1:rows-(y-rows/2)*2);
         printChars('\n', 1);
   }
}

[Mijojo]

Thanks very much, Exomo. I appreciate.

[eyenrique]

You can do it in this way!
(im late i know but can help to someone else)

Code: Select all


#include <iostream>
using std::cout;
using std::endl;

int main(){

//nested loops to print 1st part shape
for(int ccounter=1,scounter=0;scounter<=4;scounter++,ccounter+=2){
        for(int nested_counter=4;nested_counter>=scounter;nested_counter--){
                cout<<' ';
        }//end for
        for(int nested_counter=1;nested_counter<=ccounter;nested_counter++){
                cout<<'@';
        }//end for
cout<<endl;
}//end for

//nested loops to print 2nd part shape
for(int ccounter=7,scounter=3;scounter>=0;scounter--,ccounter-=2){
        for(int nested_counter=4;nested_counter>=scounter;nested_counter--){
                cout<<' ';
        }//end for
        for(int nested_counter=1;nested_counter<=ccounter;nested_counter++){
                cout<<'@';
        }//end for
cout<<endl;
}//end for

return 0; //indicate successful termination
}//end main