since I am the only one reading my code, readability doesn't matter to me. My friend challenged me to write one of these programs in under 50 lines of code, so that is why I am cramming everything together.
That is the wrong mindset and you are going about his challenge the wrong way. First, readability always matters. I don't remember who said this, but the goal of a program is to tell other people what you're telling the computer to do. And the way to shorten code isn't to arbitrarily cram lines together - it's to avoid code duplication. You have a lot of code duplication (which I'll get to) and it's quite easy to solve this problem in less than 50 lines.
Don't Repeat Yourself
The core of this problem is writing a line with some number of spaces and some number of stars. We have to do that lines
number of times, for different numbers of stars and spaces, but it's really the same subproblem. You currently have this same logic in two different places - and it's fairly awkwardly written both times (e.g. why are you incrementing down from stars
in the second loop?) So let's just put it in one function:
void printLine(int stars, int width) {
for (int i = 0; i < (width-stars)/2; ++i) {
std::cout << ' ';
}
for (int i = 0; i < stars; ++i) {
std::cout << '*';
}
std::cout << '\n';
}
This can even be abridged further by taking advantage of the std::string
constructor that produces a string consisting of a single repeating character:
void printLine(int stars, int width) {
std::cout << std::string((width-stars)/2, ' ')
<< std::string(stars, '*')
<< '\n';
}
Note that width
will actually be constant through all of our calls, so now all we really have to do is produce the correct sequence for stars
!
Loop Construction
Anytime you want to do something n
times, you shouldn't use a while
loop. You should put the incrementing logic in a for
loop - that makes it easier to tell at a glance what's going on. Compare:
for (; i < 10; ++i) { ... }
to
while (i < 10) {
// lots of code
++i;
}
Plus it's less error-prone. If you stick in a continue
at some point, the former will still work.
Furthermore, unless you have a strongly compelling reason... count upward. Counting down is more complex - avoid it unless necessary.
Undefined Behavior
This line is UB:
spaces = (lines % 2 == 0) ? spaces = 0 : spaces = 1;
You meant:
spaces = (lines % 2 == 0) ? 0 : 1;
or:
spaces = lines % 2;
Although with printLine()
that's not really necessary at all.
Putting it all together
So once we get lines
, we want to print lines
lines:
for (int i = 0; i < lines; ++i)
{
}
What is our total width? If lines
is odd, the max width is lines
. If lines is even, we stop one less. So let's save that off:
int max_width = lines % 2 == 0 ? lines - 1 : lines;
for (int i = 0; i < lines; ++i)
{
printLine(???, max_width);
}
Now we just need the number of stars. It starts at 2*i+1
for the first lines/2
lines, then we get one line of lines
stars (for odd stars), and then we go back down the pyramid. As a first go, we can just split it in 3:
for (int i = 0; i < lines/2; ++i) {
printLine(2*i+1, max_width);
}
if (lines % 2 == 1) {
printLine(max_width, max_width);
}
for (int i = lines/2-1; i >= 0; --i) { // here we have a compelling reason
printLine(2*i+1, max_width); // to descend
}
This is on the order of 25 lines and far easier to understand.
One Loop
Of course if we really want to reduce the line count, we can do it in one loop. I wouldn't recommend this, since it's harder to discern, although it's not too bad. The key is just to see at one point we flip from going up by 2s to down by 2s:
int max = lines % 2 == 0 ? lines - 1 : lines;
for (int i = 0; i < lines; ++i) {
printLine(i < lines/2 ? 2*i+1 : 2*(lines-i)-1, max);
}