Printing pyramids in C (CS50)

Question

I'm taking the CS50 course, and we're asked to create a program which takes an input (the total height of the pyramid) and outputs a pyramid of that height.

i.e Here’s how the program might work if the user inputs 8 when prompted:

$ ./mario
Height: 8
       #  #
      ##  ##
     ###  ###
    ####  ####
   #####  #####
  ######  ######
 #######  #######
########  ########

Here is my code:

#include <cs50.h>
#include <stdio.h>

void print_hashes(int width, int empty_space);

int main(void)
{
    // Get height input between ranges 1 to 8 inclusive
    int height;
    do
    {
        height = get_int("Height: ");
    } 
    while (height < 1 || height > 8);
    // Loops through each row
    for (int row = 1; row <= height; row++)
    {
        // Prints the left part of the pyramid, we need to print empty spaces before printing the hashes, so
        // the width is equal to the height, amount of hashes printed is equal to the row, so amount of empty spaces is the width - row
        print_hashes(height, height - row);
        printf("  ");
        // Prints the right part of the pyramid, we don't need to print empty spaces before printing hashes, so
        // the width is equal to the amount of hashes, i.e the row and the amount of empty space is just 0
        print_hashes(row, 0);
        printf("\n");
    }
}

void print_hashes(int width, int empty_space)
{
    // Loops through the column in the row
    for (int column = 0; column < width; column++)
    {
        // Checks if there needs to be an empty space within that column, if so print a space and if not print a hash
        if (column < empty_space)
        {
            printf(" ");
        } 
        else
        {
            printf("#");
        }
    }
}

I would like some feedback on my code and how I could improve it; things like readability, efficiency, etc.

Is my code easily understandable? Am I overdoing the comments? The reason I put a lot of comments is because in my experience, when looking at code days/weeks after writing it, I tend to forget what it does. So I wrote explanations so that I can easily understand the code in the future.

How do I improve the style, and are there any code conventions I'm not following? Can I optimize this to make it a faster process? Or maybe there's a better way to do the same thing? Basically, how do I improve my code?

Answer 1

The code can be structured better, so the code is more self-documenting. Self-documenting code doesn't need as many comments, which makes it easier on everyone. Comments that aren't there can't be obsolete either.

• Take input (height)
• Output pyramid

Those are the only lines I want to see in main. The rest should be in functions. You started out nicely by creating a print_hashes function, but we can do better. For example, the entire do ... while can be in a function.

int ask_height(void)
{
    // Get height input between ranges 1 to 8 inclusive
    int height;
    do
    {
        height = get_int("Height: ");
    } 
    while (height < 1 || height > 8);
    return height;
}

Now, we can start the main program like this instead:

int main(void)
{
    int height = ask_height();

That turns 7 lines in main into 1 line in main. And since the function name is descriptive, the comments are almost obsolete.

We can do the same thing with the rest of the program.

void print_pyramid(int height)
{
    // Loops through each row
    for (int row = 1; row <= height; row++)
    {
        // Prints the left part of the pyramid, we need to print empty spaces before printing the hashes, so
        // the width is equal to the height, amount of hashes printed is equal to the row, so amount of empty spaces is the width - row
        print_hashes(height, height - row);
        printf("  ");
        // Prints the right part of the pyramid, we don't need to print empty spaces before printing hashes, so
        // the width is equal to the amount of hashes, i.e the row and the amount of empty space is just 0
        print_hashes(row, 0);
        printf("\n");
    }
}

And all of a sudden, we have nicely split-out code, each function has one and only 1 thing to do (you may want to read up on the SRP, the Single-responsibility principle).

Now your file starts like this:

#include <cs50.h>
#include <stdio.h>

int ask_height(void);
void print_hashes(int width, int empty_space);
void print_pyramid(int height);

int main(void)
{
    int height = ask_height();
    print_pyramid(height);
}

That's it. The rest is in functions. The function names can probably be further improved, but I'm quite awful at that myself. If you want to go a step further, you don't even need a height variable in main. This works too:

int main(void)
{
    print_pyramid(ask_height());
}

The rest is in functions, and functions can be re-used. Besides, should you ever want to call your program from an outside location, that's a lot easier now too.

Answer 2

A pyramid in geometry is a certain polyhedron i.e. a 3D structure. Your empty stripe in the middle turns it into a stepped pyramid.

This can be split into two triangles, as you do with print_hashes(). But while the function is declared as:

void print_hashes(int width, int empty_space)

it is called with height, not width

print_hashes(height, height - row);

This switch is somehow mentioned in the comment, but the code is confusing.

Triangle(s)

Instead of pyramids, rows/columns and hashes you maybe should be working with triangles. A pyramid (or triangle) of height 4 looks like:

123#...
12###..
1#####.
#######

It has 1+3+5+7 = 16 = 4*4 hashes (sum of odd numbers). Everything can be known in advance. The numbers of spaces to be inserted is 3,2,1,0, the number of hashes 1,3,5,7. The bottom layer is 4+3=7, i.e. base width = 2*height - 1.

Instead of looping:

for (int column = 0; column < width; column++)
    {
        // Checks if there needs to be an empty space within that column, if so print a space and if not print a hash
        if (column < empty_space)

you could prepare a whole line with string/array operators or memset() and print it at once at the end.

The one-by-one printing of every "pixel" is what is slowing the program down, not the loop itself.

You get along with no math and not much C at all - but at some price.

A single string (array of chars) that gets changed and printed as line (and represents a layer) is more flexible and much faster. You allocate it, fill it, and then in a loop apply the changes and print it. String (=memory) operations are very fast, and printf() is very slow.

---

These stairs in the middle are disruptive...but here is a solution that uses two (different, alas) simpler triangles:

#include <stdio.h>
#include <string.h>

// prints the left half-layer of a pyramid with height
// layer 1 is the top layer;  
void print_hashes(int height, int layer)
{
    char line[height+1];
    int empty = height - layer;
    memset(line,         ' ', empty);
    memset(line + empty, '#', layer);
    line[height] = '\0';
    printf("%s", line);
}
// right side 
void print_hashes_desc(int layer)
{
    char line[layer+1];
    memset(line, '#', layer);
    line[layer] = '\0';
    printf("%s", line);
}
int main(void)
{
    // Ask for height
    int height;
    do
    {
        scanf("%d", &height);
    } while (height < 1 || height > 8);
    
    for (int layer = 1; layer <= height; layer++)
    {
        // Left part
        print_hashes(height, layer);
        // Middle (stairs)
        printf("__");
        // Right part 
        print_hashes_desc(layer);
        printf("\n");
    }
}

Instead of memset() the string could also be filled with for-loop. The expression empty = height - layer is the only math involved.

Edit: just realized that the simpler _desc function does not need height. So much for copy-pasting...this is all not so obvious, without handy string operations...and with these stairs in the middle. In the first link they gave up on "print_repeated()"-function.

Output:

       #__#
      ##__##
     ###__###
    ####__####
   #####__#####
  ######__######
 #######__#######
########__########

Answer 3

printf is overkill for printing a single character - you could use putc instead.

However, printf provides some useful facilities that we can use to our advantage. Look at this for inspiration:

#include <stdio.h>

int main(void)
{
    const int width = 8;
    for (int i = 0;  i < width;  ++i) {
        printf("%*s%.*s\n", width-i, "", i, "################");
    }
}

See how we can use printf() to pad the empty string with spaces, or truncate a longer string to a given width.

You'll want to read the printf documentation to see how . and * are used in format strings, and think of a way to ensure you have enough # characters to print.