Code review request for efficiency

Brand new coder here. This code works, but I would love some feedback on inefficiencies and/or bad habit avoidance. Thanks for taking the time!

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
#include <iostream>
#include <cmath>

using namespace std;

int main()
{
    //Asking for an odd number to make an even shaped diamond
    cout << "This program creates a diamond shape out of *s.\nPlease enter an odd number for the height of the diamond:\n";
    int rows;
    cin >> rows;

    //Check for odd number
    while ((rows % 2) == 0)
    {
        cout << "\nThat is not an odd number, please try again:\n";
        cin >> rows;
    }

    double rowCount = round(rows / 2.0);
    int stars = 0;
    
    //Top half of diamond
    //Moves down through the rows
    for (int count = 1; count <= rowCount; count++)
    {
        //Enters leading spaces prior to first start
        for (int spaces = 1; spaces <= (rowCount - count); spaces++)
        {
            cout << " ";
        }

        //Enters stars and spaces between them
        while (stars != (2 * count - 1))
        {
            if ((stars == 0) || (stars == (2 * count - 2)))
            {
                cout << "*";
            }
            else
            {
                cout << " ";
            }
            stars++;
        }
        stars = 0; //Reset this counter for a new row of stars
        cout << "\n";
    }

    rowCount--; //The middle row was accounted for in the top half

    //Bottom half of diamond
    //Moves down through the rows
    for (int count = rowCount; count >= 1; count--)
    {
        //Enters leading spaces prior to first start
        for (int spaces = 1; spaces <= (rowCount - (count-1)); spaces++)
        {
            cout << " ";
        }

        //Enters stars and spaces between them
        while (stars != (2 * count - 1))
        {
            if ((stars == 0) || (stars == (2 * count - 2)))
            {
                cout << "*";
            }
            else
            {
                cout << " ";
            }
            stars++;
        }
        stars = 0; //Reset this counter for a new row of stars
        cout << "\n";
    }
}
using namespace std; bad habit

efficiency can be improved, at the cost of being readable, and there isnt any point in speeding up nearly instant code that has no real value (its homework, you learned from it, but it does not need to run 5000 times a second does it?) after you have coded it.

one example of speed is to generate an output string rather than cout over and over, and dump the string at the end.
if you did that ^^ a second example is you can make top and bottom half at the same time in a single loop, doing 1/2 the work. you can do that into an output string because you can write anywhere inside it at any time, but the console is one line at a time no going back (well, with standard tools, there are ways) so you can't do that as cleanly.
but I ask again, do you really care? Nothing you have is directly inefficient (bad code) in any major way (maybe small things if you want to get really deep on it).

little thing:
consider
1
2
3
4
5
rowCount--; //The middle row was accounted for in the top half

    //Bottom half of diamond
    //Moves down through the rows
    for (int count = rowCount-1; count >= 1; count--)


issue:
double rowCount = round(rows / 2.0);
why is rowcout a double ?! I see no reason and if int, round becomes pointless as well.

top and bottom code are so close you could have made them a function, but having already said you could make that one loop instead of 2, the redundancy goes away, so meh.
Last edited on
Great feedback. Exactly what I was looking for. Thanks!
Lets make a table (for 7 rows):
row P M
 0  3 -
 1  2 1
 2  1 3
 3  0 5
 4  1 3
 5  2 1
 6  3 -

where
P: space before first star
M: space between stars

Is it possible to calculate P from row?
Is it possible to calculate M from row?

Yes. That is why one can process all rows with one loop:
1
2
3
4
5
6
7
for ( int row = 0; row < rows; ++row ) {
  int p = ...
  int m = ...
  // print p spaces and *
  if ( m > 0 ) // print m spaces and *
  std::cout << '\n';
}



The <iomanip> offers std::setw.
See http://www.cplusplus.com/reference/iomanip/setw/
With that: std::cout << std::setw( p + 1 ) << '*';

Does that print p spaces "without a loop"? Yes and no.
We do not see a loop, but the implementation of ostream::operator<< has one in it.


Note:
1
2
cout << '*'; // prints one char, calls operator<<(char)
cout << "*"; // prints C-string {'*', 0}, calls operator<<(char*) 

The double-quote version loops through an array.
Last edited on
Topic archived. No new replies allowed.