1

This is my code for pset1 mario problem. It works but it looks really messy and inefficient. I think I could use less variables and use the while loops better but I'm not sure where to begin. I'd be happy if someone could point out where and why my code is inefficient so I can fix it

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

int main(void)
{
    int pyramidHeight;

    printf("How tall is the pyramid?: ");
    pyramidHeight = GetInt();

    while(pyramidHeight <= 0 || pyramidHeight > 23)
    {
        printf("Please enter a number between 1 and 23: ");
        pyramidHeight = GetInt();
    }

    int spaces = pyramidHeight - 1; //amount of spaces(should end up at 0)
    int spacesHolder = pyramidHeight -1; //place holder for holding spaces
    int hashes = 2; //how many hashes to print
    int hashesHolder = 2; //counts how many hashes we're supposed to print next
    int finalHashes = pyramidHeight + 2; //number of hashes it should end up at (It should be pyramidHeight + 1 but when I put 1 it ends too early)

    while((hashes != finalHashes)) //run this while final amount of hashes has not been printed
    {
        while(spaces != 0)
        {
            printf(" ");
            spaces--;
        }

        spacesHolder--; //print -1 space next line
        spaces = spacesHolder; 

        while((hashes != 0))
        {
            printf("#");
            hashes--;
        }

        hashesHolder++; //print +1 hash next line
        hashes = hashesHolder; 

        printf("\n"); //move to next line

    }

}

EDIT: UPDATED CODE BELOW

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

int main(void)
{

    printf("How tall is the pyramid?: ");
    int pyramidHeight = GetInt(); //get height of pyramid from user

    while(pyramidHeight <= 0 || pyramidHeight > 23) //if the user inputs a number less than or equal to 0 or greater than 23, prompt for a number again
    {
        printf("Please enter a number between 1 and 23: ");
        pyramidHeight = GetInt();
    }

    int space = pyramidHeight - 1; //initial amount of spaces to print
    int hash = 2; //initial amount of hashes to print

    for(int currentHeight = 0; currentHeight != pyramidHeight; currentHeight++) //while current pyramid height is less than final height
    {
        for(int spaceCurrent = space; spaceCurrent != 0; spaceCurrent--) //spaceCurrent = number of spaces currently being printed
        {
            printf(" ");
        }

        space--; //print one less space next time

        for(int hashCurrent = 0; hash != hashCurrent; hashCurrent++) //hashCurrent = number of hashes currently being printed
        {
            printf("#");
        }

        hash++; //print one more hash next time

        printf("\n"); // move to next line
    }


}

2 Answers 2

1

You can possibly use two nested for loops instead of while loops to make the code more understandable and presentable. Just a suggestion!

1

Declaring variables prior to initializing them can create cluttered code and is often a bad idea unless you require them to have a larger scope, even if you do require a larger scope you can tweak your code in order to not have initialize them early.

I prefer using for loops instead of while loops, but that's entirely up to you as long as your code is concise and readable, which to me it is except for all the variables.

6
  • Can you initialize a variable within the while loop? For example: While(int spaces = pyramidHeight - 1 != 0) { } Commented Oct 17, 2016 at 8:26
  • No it does not seem like so, as such a for loop would be better apt for the job. If you don't plan on switching to a for, you're declaring a lot of variables in one chunk, so it is difficult to see where each belongs. Although if you absolutely must declare variables prior to initializing them, try to move the declaration and initialization as close together as possible. Commented Oct 17, 2016 at 9:37
  • I just prefer using while loop since it makes more sense to me. I will try using a for loop and update the OP! Commented Oct 17, 2016 at 12:26
  • I try to think of the for loop as something I use when I know how many times the body will "loop", and the while loop when it has to run an unknown amount of times. Also, when you're decrementing (i.e., hashes--;) the variable used in the condition of a while loop inside the body, you're already doing what a for loop would, just using extra space. Commented Oct 17, 2016 at 13:49
  • Hey I updated the code on the OP! It was a good practice for using the for loop. I think it looks much more clean and presentable now Commented Oct 18, 2016 at 13:33

You must log in to answer this question.

Not the answer you're looking for? Browse other questions tagged .