6
\$\begingroup\$

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?

\$\endgroup\$
3
  • \$\begingroup\$ Considering the purpose of the code, are you really interested in performance? I'd say the readability is a bigger concern. \$\endgroup\$ – Mast Jan 17 at 6:01
  • \$\begingroup\$ Yeah performance shouldn't be that big of an issue since this is so simple. It's just weird since when I tried it with 100 levels it took a while to print everything out, though maybe that's an issue with the IDE or my computer. What can you tell me about the readability? \$\endgroup\$ – xdxt Jan 17 at 6:28
  • \$\begingroup\$ You need more functions. I'll write something down later today. \$\endgroup\$ – Mast Jan 17 at 6:53
5
\$\begingroup\$

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.

\$\endgroup\$
4
  • \$\begingroup\$ I've done some reading on SRP, but it seems like the principles (as well as the SOLID principles for which it's a part of) applies for OOP only. Are these principles applicable to functions as well? I'm not very familiar with OOP atm. \$\endgroup\$ – xdxt Jan 17 at 12:39
  • \$\begingroup\$ @xdxt For classes it becomes extra important, but there's little reason not to apply it outside of OO as well. You'll find out later there are some situations where not turning everything into a function makes sense too, but in the vast majority of cases you want to keep your functions nice and simple. \$\endgroup\$ – Mast Jan 17 at 13:05
  • \$\begingroup\$ What are some examples in which "situations where not turning everything into a function makes sense"? I'm curious. I really like the SRP approach as my code looks much cleaner now, but I'm wondering whether there are cases at which this principle would do more harm than good. \$\endgroup\$ – xdxt Jan 17 at 13:11
  • \$\begingroup\$ @xdxt It's not something you should be worried about now. But you'll probably encounter a situation where it's simply not worth the effort or where splitting up will lead to so many mini-functions that are already relatively small. There is such a thing as taking it too far, experience will tell you when you've reached that point. Sometimes it's nicer to have a function that appears a bit bigger, but still makes sense in its context. main should always be as compact (good use of functions, as shown) as possible, after that it varies how rigid you want/should be. \$\endgroup\$ – Mast Jan 17 at 14:10
3
\$\begingroup\$

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:

       #__#
      ##__##
     ###__###
    ####__####
   #####__#####
  ######__######
 #######__#######
########__########
\$\endgroup\$
1
  • 2
    \$\begingroup\$ Ouch! Memset and pointer arithmetics are not the right tool to fomat a string. They depend on the low-level working of C strings and pointers. It is difficult to read and brings to advantage. Imho. \$\endgroup\$ – Florian F Jan 17 at 22:51
-2
\$\begingroup\$

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.

\$\endgroup\$
3
  • 1
    \$\begingroup\$ Please explain what this code is doing. \$\endgroup\$ – Peilonrayz Jan 17 at 14:16
  • \$\begingroup\$ Why? This isn't spoon-feeding; reading the man page and really understanding printf is far more important than having the code for a single exercise. \$\endgroup\$ – Toby Speight Jan 17 at 14:32
  • 6
    \$\begingroup\$ After reading your answer I don't feel enriched. I've not learnt anything, apart from "go RTFM" is still alive and kicking. \$\endgroup\$ – Peilonrayz Jan 17 at 14:35

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.