11
\$\begingroup\$

Can you please review this code to determine if it's good or can be improved?

#include "stdafx.h"
#include <iostream>
#include <string>

using namespace std;

int _tmain(int argc, _TCHAR* argv[])
{

    int h = 20;
    int bl = 1;

    while (h != 0) {
        for (int i = 0; i != h; i++) {
            cout << " ";
        }
        for (int i = 0; i != bl; i++) {
            cout << "*";
        }
        cout << endl;
        h = h - 1;
        bl = bl + 2;
    }
    return 0;
}
\$\endgroup\$
12
\$\begingroup\$

First, it isn't too good of an idea to use using namespace std; as discussed here. There are times and places to use it, but you should know about this.

Second, it is obvious you are using VC++ (which isn't bad, but isn't exactly the same as C++). You should write your main like this instead:

int main()

I'm not sure what the h and bl names are supposed to mean (height and baseline?), use descriptive names.

This can be a for loop:

while (h != 0) {

Write it like this:

for (int height = 20; height > 0; --height) {
    for (int i = 0; i != height; i++) {
        cout << " ";
    }
    for (int i = 0; i != bl; i++) {
        cout << "*";
    }
    cout << endl;
    bl = bl + 2;
}

In fact, you can even put the bl variable in the loop too:

for (int height = 20, baseline = 1; height > 0; --height, baseline += 2) {
    for (int i = 0; i != height; i++) {
        cout << " ";
    }
    for (int i = 0; i != baseline; i++) {
        cout << "*";
    }
    cout << endl;
}

Use the prefix ++ and -- operators whenever possible because the postfix operators have worse performance due to the need for them to create a copy of the object to return before incrementing the value.

for (int i = 0; i < baseline; ++i) {

Also, in a loop like this, you typically do i < baseline, not i != baseline.

If you want to remove the inner loops entirely, you can rewrite the entire program to just this:

for (int height = 20, baseline = 1; height > 0; --height, baseline += 2) {
    std::cout << std::string(height, ' ')
              << std::string(baseline, '*')
              << std::endl;
}
\$\endgroup\$
  • 3
    \$\begingroup\$ Your comment about the increment/decrement operators is only partly correct. Yes, when applying them to an iterator object, the pre-inc/decr will avoid a copy. For a native type like int, this is irrelevant. \$\endgroup\$ – glampert Apr 20 '15 at 18:37
  • \$\begingroup\$ @glampert Oh, I didn't know that. \$\endgroup\$ – Hosch250 Apr 20 '15 at 18:38
  • \$\begingroup\$ I think this question has better answers than the one you've pointed to. Also, I do prefer to use pre-increment whenever possible to keep it consistent. \$\endgroup\$ – glampert Apr 20 '15 at 18:42
  • \$\begingroup\$ why use a for loop? While loops are better, at least you can prove while loops. \$\endgroup\$ – Vincent Apr 21 '15 at 6:41
  • \$\begingroup\$ I don't see how a while loop is better than a for loop - especially when you are iterating over a given range. \$\endgroup\$ – Hosch250 Apr 21 '15 at 15:42
10
\$\begingroup\$

Here are some things that I noticed that may help you improve your code.

Strive for portability where practical

Sometimes it's necessary to write non-portable code that is specific to a particular operating system or environment, but this is not one of those times. By doing two simple things -- omitting "stdafx.h" and using main() instead of non-standard _tmain() your code will still compile and run correctly under Windows, but also under any other operating system which supports C++. Since it costs essentially nothing and gains a great deal (not least because more Code Review reviewers will be able to try your code) it makes sense to use portable constructs where you can.

Only #include required headers

This program uses std::cout and so it needs <iostream> but makes no use of std::string and so it does not need <string>. Make sure you know the difference between a plain old C-style string constant such as "*" and a std::string.

Eliminate return 0 at the end of main

When a C++ program reaches the end of main the compiler will automatically generate code to return 0, so there is no reason to put return 0; explicitly at the end of main. This has been the case in C++ since the 1990 standard and in C since the 1999 standard.

Don't abuse using namespace std

Putting using namespace std at the top of every program is a bad habit that you'd do well to avoid. It isn't necessarily wrong to use, but be aware of when you absolutely shouldn't do it (such as in header files).

Learn and use C++ idioms

In C++ one would usually write --h rather than h=h-1. Similarly instead of a conditional expression h != 0 one would typically write simply h since it's functionally identical in C++ or C.

Prefer using for rather than while

The loop that you have is more idiomatically expressed as a for loop rather than as a while loop.

for (int h = 20, bl = 1; h; --h, bl+=2) {
    for (int i = 0; i != h; i++) {
        cout << " ";
    }
    for (int i = 0; i != bl; i++) {
        cout << "*";
    }
    cout << endl;
}

We do this because then it is clear that h and bl are only defined within the for loop and because it is now clear that they are altered each iteration.

Prefer to decrement to zero

On many machine architectures, zero is treated specially. There are often shorter instructions for loading or testing for zero, so for many machines, it's more efficient (in terms of both code space and execution time) to decrement rather than increment through a loop:

for (int i = h; i; --i) {
    cout << " ";
}

This is often (somewhat derisively) called a micro-optimization, but if you get into the habit of writing loops this way, by the time you are working on large, major projects in which the cumulative effects add up to "software that's too slow", you'll not have to relearn how to code simple loops.

Know the difference between --i and i--

In a short simple program such as this one, it makes no difference, but over your programming career, you will use both --i and i--. The difference is that the pre-decrement version (--i) decrements and then returns the value, while the post-decrement version (i--) saves the original value, decrements and then returns the original undecremented value. Modern compilers are smart enough to understand when you're not actually saving the value and will optimize them both to the same exact code (as with this case), but again, it's useful to know the difference.

Omit argc and argv unless you use them

This code doesn't actually use argc or argv so it would be better to omit them. This gives a very good clue to the reader of your program's source code that it takes no command-line parameters.

Use named constants where useful

The only constants here are 20 and 1 so for such a simple program it doesn't make much difference but it makes sense to get into the habit of naming constants. That way, in a larger program, when you have five constants, all with different uses but with the value 20 it will be possible to easily change one constant without having to hunt through the program for all instances of 20 and wondering which constant was meant.

Applying these tips

Here's a version of the code with those tips applied:

#include <iostream>

int main() 
{
    const int PYRAMID_HEIGHT = 20;
    const int PYRAMID_TOP_WIDTH = 1;
    for (int h = PYRAMID_HEIGHT, bl = PYRAMID_TOP_WIDTH; 
             h; --h, bl+=2) 
    {
        for (int i = h; i ; --i) {
            std::cout << " ";
        }
        for (int i = bl; i ; --i) {
            std::cout << "*";
        }
        std::cout << std::endl;
    }
}
\$\endgroup\$
7
\$\begingroup\$

In addition to @Hosch250's review:

Don't use stdafx.h for small programs

The precompiled headers are meant to save build (compile) time by processing infrequently changed header files into a more compact form. The savings is insignificant for small programs (the small programs appear to build in the same amount of time or less). The build time becomes long when there are over 100 files or files with lots of includes (such as Windows.h).

Declare main with no parameters

If you are not going to use command line parameters, declare the main function as:

void main()

or

void main(void)

Prefer named identifiers, not magic number

When other people read your code they see the number 20 and wonder what is the significance.
If you use something like:

  const unsigned PYRAMID_HEIGHT = 20U;

the value will be more apparent in the code.

Signed int versus unsigned int

The int type can be negative. It should be used when values are allow to be negative. The unsigned int should be used for values (such as counts) which are never negative.

In your program, you should be using unsigned int because quantity of lines or columns will not be negative.

This will help the compiler do checking for you before your program executes.

List the std items you are using. Rather than performing the global:

  using namespace std;

Tell the world which std:: functions you are using:

  using std::cout;
  using std::endl;
  using std::string;

This will reduce the conflicts of any symbols that you create that already exist in the std namespace.

Prefer returning EXIT_SUCCESS and EXIT_FAILURE

The identifiers EXIT_SUCCESS and EXIT_FAILURE are standard return values to the operating system. They may not be 0 or 1. Also, they are more readable.

There are some cases where 0 is the successful value, other cases it is not.

\$\endgroup\$
  • \$\begingroup\$ In Visual C++, you need to have the stdafx.h header or nothing compiles. \$\endgroup\$ – Hosch250 Apr 20 '15 at 20:48
  • 2
    \$\begingroup\$ Wrong. I turn off stdafx.h in all my Visual Studio projects. All of them compile, including the GUI programs. Go into the project properties, under Configuration properties --> C/C++ --> Precompiled Header. Change "Precompiled Header" to "Not Using Precompiled Headers". Also, when creating new projects, create empty project; don't use MS crap. \$\endgroup\$ – Thomas Matthews Apr 20 '15 at 21:39
  • \$\begingroup\$ Bjarne Stroustroup and many other smart people now agree: unsigned ints are generally a bad idea (skip to 42m40s). An unsigned int would prevent the value from going negative, but only by wrapping around to 2^32-1, which causes even weirder bugs. Java, for example, abandons unsigned types altogether. \$\endgroup\$ – 200_success Apr 21 '15 at 5:14

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.