Printing out a diamond with a user-inputted size

Question

I made a program that prints out a diamond with n lines. n is equal to whatever the user inputted. I know I overuse the ternary or conditional operator, but in my opinion, it makes the code more compact, and 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.

#include <iostream>

int main() {

    int lineNum = 1, lines,  stars = 1, spaces = 0;

    std::cin >> lines;

    // if lines is even, split it equally, if not, split in half and add 1
    while (lineNum <= ((lines % 2 == 0) ? (lines / 2) : (lines / 2) + 1)) {

        for (int c = (((lines % 2 == 0) ? (lines / 2) : (lines / 2) + 1) - lineNum); c != 0; c--)
            std::cout << " ";

        for (int c = 0; c < stars; c++)
            std::cout << "*";

        std::cout << "\n";

        lineNum++;
        stars += 2;

    }

    stars = (lines % 2 == 0) ? stars - 2 : stars - 4;
    spaces = (lines % 2 == 0) ? spaces = 0 : spaces = 1;

    while (lineNum <= lines) {

        for (int c = 0; c < spaces; c++)
            std::cout << " ";

        for (int c = stars; c != 0; c--)
            std::cout << "*";

        std::cout << "\n";
        stars -= 2;
        spaces++;
        lineNum++;

    }


}

Answer 1

I see a number of things that could help you improve your code.

Don't disregard readability

Readability of code is always of use and always of interest, even if you're the only one who ever sees the code. Since, by definition, you're asking for a code review by posting here, even that assumption is clearly faulty, because every reviewer is going to have to read and understand your code to comment usefully on it. Even if you really were the only person to ever look at the code, if you decide to modify it some time later, readability will be important to you.

Decompose your program into functions

All of the logic here is in main in one rather long and dense chunk of code. It would be better to decompose this into separate functions.

Don't further clutter trinary operators

The code currently includes this line:

spaces = (lines % 2 == 0) ? spaces = 0 : spaces = 1;

The problem is that the spaces = 0 and spaces = 1 on the right side are wholly uneccesary. If you must use trinary operators, (and I question that in this case), use them correctly:

spaces = (lines % 2 == 0) ? 0 : 1;

Simpler, and better in my view, would be this:

spaces = lines & 1; 

Rethink your algorithm

The biggest improvements are often found in simply rethinking and reimplementing the basic algorithm. In this case, each line is 0 or more spaces, followed by 1 or more stars. For each increasing line in the first half, the number of spaces decrements by one and the number of stars increments by two. A little bit of examination with paper and pencil will quickly yield algebraic expressions for the number of spaces and number of stars for each line in the top half of an n-line diamond pattern. The bottom half is simply the reverse of the top half. This suggests a simple algorithm.

Putting it all together

Your original program, as posted, was 44 lines long. Using all of these hints, and with better readability, here it is in exactly half that many lines:

#include <iostream>

void multiprint(unsigned n, char ch) {
    for ( ; n; --n) 
        std::cout << ch;
}

void line(unsigned n, unsigned maxline) {
    unsigned half = (maxline-1)/2;
    if (n > half) n = maxline - n - 1;
    multiprint(half - n, ' ');
    multiprint(n*2 + 1, '*');
    std::cout << '\n';
}

int main() {
    int lines;
    std::cin >> lines;

    for (int i =0; i < lines; ++i)
        line(i, lines);
}

Answer 2

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);
}