4
\$\begingroup\$

I have this problem where I've to print a number pyramid by taking input from the user.

        1
      2 3 2
    3 4 5 4 3
  4 5 6 7 6 5 4
5 6 7 8 9 8 7 6 5

I've written the following code and then output is correct. However, I was wondering if there are any other optimized way to print this pyramid, since mine includes 4 loops.

#include<iostream>
using namespace std;
int main()
{
    int a;
    cin>>a;
    for(int i=1;i<=a;i++)
    {
        for(int j=a-i;j>=0;j--)
        {
            cout<<"  ";
        }
        for(int k=i;k<=(i+i-1);k++)
        {
            cout<<(k)<<" ";
        }
        for(int l=i+i-2;l>=i;l--)
        {
            cout<<l<<" ";
        }

        cout<<endl;
    }

    return 0;
}
\$\endgroup\$
3
  • \$\begingroup\$ Whilst I dislike recursive code and have always avoided writing it, maybe this is one of those rare instances where recursion would be the best way to code it. \$\endgroup\$ Dec 7 '21 at 16:42
  • 4
    \$\begingroup\$ I can't think of any reason to use recursion here. \$\endgroup\$
    – ggorlen
    Dec 7 '21 at 18:08
  • \$\begingroup\$ What should the program do for input a>5, when two digit numbers start to show up? \$\endgroup\$
    – slepic
    Dec 8 '21 at 13:18
8
\$\begingroup\$

Don’t write using namespace std;.

You can, however, in a CPP file (not H file) or inside a function put individual using std::string; etc. (See SF.7.)

⧺SL.io.50 Don't use endl.


Your first inner loop is just printing a-i spaces, right? Use the width modifier to ostream output to just do that with one command.

A single space should be a character, not a string. It will be more efficient.

cout << setw(a-i) << ' ';

Prefer prefix increment/decrement.

It doesn't affect the efficiency for plain int when not using the value of the expression, but more generally the prefix for is efficient in-place modification and the postfix causes a copy to be made. Seeing a postfix ++ in a for loop or whenever the return value is not used is a code review issue — it's wasted brain power and a distraction to figure out "Oh, it doesn't matter this time."


Looping n times: The C-style for loop is awkward and more complex than simply saying what you want. With C++20 you can use std::ranges::views::iota. In older versions where this is not included with the compiler, you can use other libraries with your program or supply your own simple counter as part of your program.

\$\endgroup\$
2
  • \$\begingroup\$ yes, the first inner loop is used for printing spaces, other than use of width modifier, are there anyother modifications that i can use in this code to make it more efficient? \$\endgroup\$
    – Manya Garg
    Dec 7 '21 at 16:09
  • 1
    \$\begingroup\$ I edited to remove the multiple version tags - that slightly invalidates your answer, but I think that's easily fixable. \$\endgroup\$ Dec 7 '21 at 17:19
3
\$\begingroup\$

Comments

However, I was wondering if there are any other optimized way to print this pyramid, since mine includes 4 loops.

Probably a lot of ways to optimize the code. But nothing worth spending time over. The compiler will do a great job here.

I would not worry about the number of loops, that is not a big issue. It's the number of nested loops that is going to make code bad or have high complexity. You only have two layers of nesting so not that much to worry about.

Style Issues

Don't do this:

using namespace std;

Bad habit that will cause problems in the future.
Be specific in what you want to use.


My biggest issues with your style is readability:

    for(int i=1;i<=a;i++)

You don't need to squash everything together as tight as possible. Put in some horizontal white space to make it readable.

    for(int i=1 ; i <= a; ++i)   // Side note prefer ++i to i++

And:

    cout<<(k)<<" ";   // My poor eyes are bleeding trying to read that.

Don't use std::endl.

        cout<<endl;

Forcing a flush of the stream is rarely (if ever) needed to be done manually like this. Simply use the "\n" to add a newline.


Main does not need a return if it is always going to succeed

    return 0;

Better Code

You could have a better naming convention for your variables.
Yes everybody and their grandfather used to use i, j, k for their loop names. But have you ever tried to find all instances of the variable i in even a moderately sized piece of code with comments? You get so many false positives that it's a pain.

Use readable and meaningful names for your variables. Something that is easy to search for and spot in the code and also something the conveys meaning to the reader.


Check that the read worked.

    int a;
    cin>>a;

If I type Loki as input then a has some random value in it. What the rest of the code will do then is technically undefined as you have not set the value, so reading from it is undefined.


 for(int k=i;k<=(i+i-1);k++)

The max is (i+i-1) Is this not the same as 2*i-1 and you are using <= as the comparison so we could drop the -1 and use a less than.

 for(int k=i; k < 2*i; ++k)

The only reason you are using 2*i is that you start at i and count up. It seems like this would be easier to read by adding i to the output.

 for(int k=0; k < i; ++k)
     std::cout << (k + i) << " ";  // Start at i count up.

OK. The next trick is probably not the most efficient way of doing something. But it is a reasonable trick for small layout examples like this. I would not do it for anything more than simple text layouts unless you think about it and decide it's worth it.

    for(int j=a-i;j>=0;j--)
    {
        cout<<"  ";
    }
    // We can replace this by printing a dynamically created string.
    std::cout << std::string(' ', (a - i));

#include<iostream>

void printPryamid(int size)
{
    for(int row = 1; row <= size; ++row)
    {
        std::cout << std::string((size - row) *2, ' ');
        int loop;
        for(loop = 0; loop < row; ++loop)
        {
            std::cout << (row + loop) << " ";
        }
        for(--loop; loop > 0; --loop)
        {
            std::cout << (row + loop - 1) << " ";
        }

        std::cout << "\n";
    }
}

int main()
{
    int size;
    if (std::cin >> size)
    {
        printPryamid(size);
    }
    else
    {
        std::cout << "User Error on Input\n";
    }
}
\$\endgroup\$
9
  • 1
    \$\begingroup\$ “printPryamid” – Typo? \$\endgroup\$
    – Martin R
    Dec 8 '21 at 4:19
  • \$\begingroup\$ In this review, it's not always clear what's a quote from the question, and what's a suggested replacement. Please consider using > quoting for the code you're reviewing, to make that more obvious! \$\endgroup\$ Dec 8 '21 at 7:47
  • \$\begingroup\$ While i kind of agree with the "meaningful names" part, I doubt that "loop" conveys a better meaning for a counter, it would be like naming my fingers "hands" when I count things with my fingers. I also wonder why you prefer pre-incrementing in the loop declaration \$\endgroup\$
    – Kaddath
    Dec 8 '21 at 9:34
  • \$\begingroup\$ Out of curiosity prefer ++i to i++ why is that? \$\endgroup\$ Dec 8 '21 at 9:46
  • \$\begingroup\$ @Kaddath Loop variables don't need to be that descriptive you are in a loop. But what is more important is the ability to find a loop variable. Open your editor search for all the occurrences of i even in a small loop with any comments the number of false positives is excessive. \$\endgroup\$ Dec 8 '21 at 20:11
2
\$\begingroup\$

Don't ignore input errors like this:

cin>>a;
for(int i=1;i<=a;i++)

Always test that the input stream is still good before using the values we believe were supplied. For example:

std::cin >> a;
if (!std::cin) {
    std::cerr << "Not a valid number!\n";
    return EXIT_FAILURE;  // note: defined in <cstdlib> header
}
for (int i = 1;  i <= a;  ++i)
\$\endgroup\$
0
\$\begingroup\$

If you are absolutely fixated on reducing the number of loops (which I point out is at best premature optimization, and probably a misunderstanding of how complexity should be calculated), then I offer the following bad idea which will make your code more compact, but less readable, and less efficient:

  • Replace the first inner loop with a width or string as per other recommendations
  • Replace the remaining two inner loops with
for (int loop = 1 ; loop < row*2 ; ++loop)
    std::cout << row-1+std::min(loop,row-loop) << ' ';
  • And while we're about shrinking things, remove all the extraneous { and } where the C++ grammar does not require them, especially around single expressions; only we frail humans find them useful.

If you use these ideas don't say I didn't warn you.

\$\endgroup\$

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.