Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

I am self studying C++ from Robert Lafore's OOP in C++. I am solving the first exercise questions of chapter-3 loop and decisions. I've written three different codes using different approaches. I want to get feedback over my codes like a teacher is supposed to give feedback to his student. I want to get feedback over each code individually and then want to know which solution is overall better or more efficient.

The exercise is:

Assume that you want to generate a table of multiples of any given number. Write a program that allows the user to enter the number and then generates the table, formatting it into 10 columns and 20 lines. Interaction with the program should look like this (only the first three lines are shown):

    Enter a number: 7
    7     14     21     28     35     42     49     56     63     70
   77     84     91     98    105    112    119    126    133    140
  147    154    161    168    175    182    189    196    203    210

Using a for loop inside while loop: I've written solution for user-defined number of columns and rows. The most obvious solution was to keep track of current position in the table. The number at a particular position should original_num * pos. And after every maxcolth position print a newline.

#include<iostream>
#include<iomanip>    // For setw()
#include<conio.h>    // For getch()
using namespace std;
int main()
{
    int number;
    cout << "Enter the number whose table you want: ";
    cin >> number;
    int maxrows; int maxcols;
    cout << "\nEnter the number of rows you want: "; cin >> maxrows;
    cout << "\nEnter the number of columns : "; 
    cin >> maxcols;
    int pos=1; //starting position

    while( pos<=maxrows*maxcols )
    {
        for(int col=1; col<=maxcols; col++, pos++)
        {
            cout<< setw(6) << pos*number << "  ";
        }
        cout<<"\n";
    }
    getch();
}  

The inner for loop finishes after maxcol number of repeatitions. The outer while loop puts a newline and repeats again.

Using a single for loop with an if statement inside: Another idea was to force myself to use only one loop because, in principle, the second loop should be redundant as we need to increment only pos to find the value at that position. There must be some way to tell the computer to print a newline after every maxcol number of executions -- this is acheived by the if statement.

#include<iostream>
#include<iomanip>
#include<conio.h>
using namespace std;
int main()
{
    int number;
    cout << "Enter the number whose table you want: ";
    cin >> number;
    int maxrows; int maxcols;
    cout << "\nEnter the number of rows you want: "; cin >> maxrows;
    cout << "\nEnter the number of columns : "; cin >> maxcols;
    int row; int col; //Current row and current col

    for(int pos=1; pos<=(maxrows*maxcols); pos++)
    {
        cout << setw(6) <<number*pos << "  ";
        if((pos%maxcols)==0)
        {
          cout << "\n"; 
        }

    }
    getch();    
}

The problem is the if statement doesn't get executed most of the times. For 10 maxcols if gets executed only 10% times. It mostly wastes cpu time. In the previous version every statement does some important function every single time it is executed.

Keep track of current row and column: The idea was to locate the current position by knowing the current row and column of that position. It is quite similar to the first solution but there is an important advantage that I'll mention after the code.

#include<iostream>
#include<iomanip>
#include<conio.h>
using namespace std;
int main()
{
    int number;
    cout << "Enter the number whose table you want: ";
    cin >> number;
    int maxrows; int maxcols;
    cout << "\nEnter the number of rows you want: "; cin >> maxrows;
    cout << "\n Enter the number of columns : "; cin >> maxcols;

    for(int row=0; row<=(maxrows-1); row++)
    {
        for(int col=1; col<=maxcols; col++)
        {
            cout << setw(6) << number*(col + row*maxcols); // (col + row*maxcols) represent the current position
        }
        cout << "\n";
    }
    getch();
}

Variable row is not the current row but number of rows above the current row. row*maxcols is the position of first number of current row. col is simply the current column. The beauty of this approach is that the way current position is determined allows us to print the multiplication table in vertically increasing manner too. As per the exercise the numbers 7, 14, 21 ... are printed horizontally -- what if I wanted to print them vertically and jump to next column after a particular number of rows? The way console window is designed we can't print above the current printed line -- we can't go on printing 7, newline , 14, newline..., then jump to next column and print at the initial top row. Here is the solution:

#include<iostream>
#include<iomanip>    // For setw()
#include<conio.h>    // For getch()
using namespace std;
int main()
{
    int number;
    cout << "Enter the number whose table you want: ";
    cin >> number;
    int maxrows; int maxcols;
    cout << "\nEnter the number of rows you want: "; cin >> maxrows;
    cout << "\n Enter the number of columns : "; 
    cin >> maxcols;



    for(int row=1; row<=maxrows; row++ )
    {       
        cout << "\n";
        for(int col=0; col<maxcols; col++)
        {
            cout << setw(6)<< number*(row + maxrows*col) << "  ";
        }   


    }
    getch();    
}

row is the current row now and col is the number of columns to the left of current position. (row + maxrows*col) represent the current position.


Question: - Please study every code individually and tell if the idea used could be implemented better. Could the number of variables used deceased further? Could the algorithm be improved further? Could the code be more efficient/optimised further?

  • In the second version I removed one loop and instead used an if statement? Which is better? Use two loops or one loop plus an if statement?

  • Overall which version is more professional/good/efficient? Which one would be faster at run time?

  • Bonus optional question: I am new to programming and this is the first book which I've started reading seriously. Is the way I am solving exercises ok for a starter? I'm not asking for opinion based answer. Just a suggestion.

share|improve this question
4  
Please don't use old libraries like conio.h, they are not supported by any modern compiler. Also refrain from using using namespace std for reasons highlighted here: stackoverflow.com/questions/1452721/… – Max Payne Mar 6 at 12:45
1  
@Tim didn't know about conio.h. Which library is used nowadays for getche? About using namespcae std -- I read that answer before but don't understand it. I don't even know why cout and cin are declared in a namespace while in Turbo C++ they were declared in iostream.h. Untill I need another namespace like using namespace foo I don't see any harm in using namespace std – user31782 Mar 6 at 12:56
4  
Stop using turbo c++ now! People who are using Turbo C++ now are no different from people who use stone to make fire! There are plenty of good compilers like g++, clang++, etc. If you are on linux, g++ comes pre installed. If you are on windows, you can install min-gw, or you can download Code::blocks or Visual Studio! Turbo C++ is a verry old compiler. – Max Payne Mar 6 at 13:01
    
@Tim I am using dev C++. It uses MinGW GCC 4.6.2 32-bit compiler. It somehow still allows me to use conio.h. I use both TurboC++ and DevC++. I tried downloading modern ides like code blocks Visual C++ etc. But they , for some reason, do not compile at all. So I deleted them. I'll stop using TurboC++ when I'll find any harm in it. Except from namespace and header files without .h all my programs are same both in DevC++ and Turbo C++. Even Lafore's book with Turbo C++ and Visual C++(latest edition) use same programs. – user31782 Mar 6 at 13:05

I see a number of things that could help you improve your program.

Don't abuse using namespace std

Putting using namespace std within your program is generally a bad habit that you'd do well to avoid.

Strive for portable code

Several of the features of this code are either platform-specific or compiler-specific or both. Specifically, #include <conio.h> and getch() are non-standard and not portable. Instead consider portable replacements. For instance you could use std::getchar() instead.

Use whitespace to improve readability

Lines like this:

for(int row=0; row<=(maxrows-1); row++)

become much easier to read with a little bit of whitespace:

for(int row = 0; row <= maxrows - 1; row++)

Prefer ++i to i++ in loops

There is not a big difference for most uses, but if you don't need to save the pre-incremented value, make it simple for both the reader and the compiler and say ++i. This also will help when you start using iterators which often only implement the prefix ++ operator.

Use language idioms

The classic C++ (and C) method of iteration through N things is like this this:

for(int i=0; i < N; ++i) { /* do something */ }

Note that counting starts from 0 and that the < operator is used. The current code (last variant) uses this instead:

for(int row=0; row<=(maxrows-1); row++) {
    for(int col=1; col<=maxcols; col++) {
        // do stuff
    }
}

Think about signed versus unsigned

What should the program do with a negative number for maxrows? I don't know and probably neither do you, since it doesn't really make sense. For that reason, it would make more sense for maxrows and maxcols to be declared as unsigned.

Think carefully about the problem

Each time the program prints, it's printing the next multiple of number. Rather than multiply each time, why not simply add? I'd write it like this:

int accum = number;
for(unsigned row = 0; row < maxrows; ++row) {
    for(unsigned col = 0; col < maxcols; ++col) {
        std::cout << std::setw(6) << accum;
        accum += number;
    }
    std::cout << "\n";
}

Don't optimize blindly

First, we should keep in mind what Donald Knuth has said about optimization:

The real problem is that programmers have spent far too much time worrying about efficiency in the wrong places and at the wrong times; premature optimization is the root of all evil (or at least most of it) in programming.

Your particular program probably spends 99% of its time doing I/O, so the performance differences among the variations you present is probably best approximated as zero.

We should keep in mind yet another famous quote from Knuth,

Let us change our traditional attitude to the construction of programs: Instead of imagining that our main task is to instruct a computer what to do, let us concentrate rather on explaining to human beings what we want a computer to do.

For that reason, it seems to me that the last version (with nested for loops) is probably better because it is easier for humans to understand the code. I'd write it as shown above.

Result

For the column-first version, we can still do it this way with constant adders each loop iteration. That would look like this:

#include <iostream>
#include <iomanip>

int main()
{
    int number;
    std::cout << "Enter the number whose table you want: ";
    std::cin >> number;
    unsigned maxrows; 
    unsigned maxcols;
    std::cout << "\nEnter the number of rows you want: "; 
    std::cin >> maxrows;
    std::cout << "\nEnter the number of columns : "; 
    std::cin >> maxcols;

    const int colincr = number * maxrows;
    const int rowdecr = number * (maxrows * maxcols - 1);
    int accum = number;
    for(unsigned row = 0; row < maxrows; ++row) {
        for(unsigned col = 0; col < maxcols; ++col) {
            std::cout << std::setw(6) << accum;
            accum += colincr;
        }
        accum -= rowdecr;
        std::cout << '\n';
    }
}

Example:

Enter the number whose table you want: 7

Enter the number of rows you want: 5

Enter the number of columns : 8
     7    42    77   112   147   182   217   252
    14    49    84   119   154   189   224   259
    21    56    91   126   161   196   231   266
    28    63    98   133   168   203   238   273
    35    70   105   140   175   210   245   280
share|improve this answer
    
I've some doubt on Prefer ++i to i++ in loops section. AFAIK we never need to save pre-incremented value in for loop. Because the incrementation is always done at the end of the loop. ++i or i++ both will function same in every case of for loop. – user31782 Mar 6 at 14:16
2  
Yes, exactly because you don't need to save the value use ++i instead. The difference is that ++i increments and then returns that incremented value (typically needing only a single register) while i++ increments but returns the original, unincremented value (typically requiring two registers). Not a big deal, as I've said, but why not get into good habits early? – Edward Mar 6 at 14:20
    
@user31782 If you do some search over the net, you'll find most of the questions have already been answered on other SE sites: Regarding ++i and i++ : Stackoverflow – Max Payne Mar 6 at 14:25
1  
Sure. For each item in a row, add number*maxrows, and for each new row subtract number*(maxrows*maxcols-1). Both are "loop invariants" and can be calculated once before the loop begins. – Edward Mar 6 at 14:53
2  
I've added to my answer to show exactly what I mean. – Edward Mar 7 at 12:45

No of times condition gets checked:

  • In all your examples except second one, condition was checked maxrows*maxcols times.

  • In second example, checking is done 2*maxrows*maxcols times. While this is not a significant drawback, it certainly is a bit inefficient.

Possible Improvements:

  • As highlighted by other answers, strive for readable code. Optimisation is very important, but mainly if it causes significant changes to efficiency. Its better to put our concentration in improving algorithm and the big picture, rather than keep on tinkering with tiny details. That being said, adopting good habits, which provide us with legible and fairly efficient code, is in my opinion the way to go. Micro-optimisations often lead to cryptic code.

  • Use type-modifiers like unsigned where-ever applicable. For example, the iteration variables are positive, so change their type to unsigned int.
    Perhaps you should not use unsigned for sum and num variables (refer code below), because number entered could be negative.

  • Lastly, although it doesn't make any significant changes, but addition is done more efficiently in computers than multiplication, so you can use repeated addition instead of multiplying everytime (note the sum += num) :

    #include <iostream>
    #include <iomanip>
    
    int main() {
    
        int num;
        std::cout << "Enter no: ";
        std::cin >> num;
    
        unsigned int row = 20, col = 10;
    
        int sum = 0;
        num < 0     
    
        for( unsigned int i = 0; i < row; ++i) {
    
            for( unsigned int j = 0; j < col; ++j) {
                sum += num;
                std::cout << std::setw(6) << sum;
            }
            std::cout << '\n';
        }
    
        return 0;
    }
    

Use standard C++

  • Instead of using non-standard getch() from conio.h, use cin.get() from iostream

  • Do not use using namespace std;. Refer link.
    If you want to use cin and cout, prepend them with their namespace, followed by scope resolution operator:
    std::cout << " "; and std::cin >> x;

share|improve this answer
    
How does condition checking affect performance? Would the second example be two times slower than the rest one's? Does processing of a condition checking statement take more time than processing of usual statements? E.g. does if(i==0) take more time than i=i+1? – user31782 Mar 6 at 13:59
    
You need not micro optimize! For most of your code, you shouldn't worry for optimising unless it is absolutely critical. Often times, you can use tons of micro-optimizations, but your code would become unreadable mess (see edward answer). – Max Payne Mar 6 at 14:17
    
But I am still curious about it. If nothing, It'd add some knowledge. – user31782 Mar 6 at 14:19

In a sense, you have already reviewed your own code. You have weighed the pros and cons of various approaches, and even challenged yourself to consider how to transpose the output. It's no surprise that your question has been well received on Code Review.

Be careful to look at the big picture. None of these solutions is bad code, even if some of these solutions are better than others. Even when we say that something is "inefficient", it's just a matter of craftsmanship rather than a real performance issue, because any computation will be dwarfed by the I/O and program initialization overhead. Producing maintainable code is more important than saving a few CPU cycles.


The most idiomatic way to do something n times in C/C++ is

for (int i = 0; i < n; i++) {
    …
}

It can also be done with for (int i = 1; i <= n; i++), but only if you have a good reason to start at 1. A third way is while (n--) { … }, if you don't need to preserve the original value of n and don't need to count upwards. Any other loop structure, such as

for(int row=0; row<=(maxrows-1); row++)

… would be unconventional and likely to lead to off-by-one errors.

It's best to avoid recomputing invariants such as maxrows * maxcols in the loop condition.


I'm not a fan of the first two approaches.

The first approach has pos initialized on one line, tested on a second line, and incremented in a third place. That makes it hard to follow.

The second approach tries to perform a two-dimensional task using one linear loop. I have some mild concerns about computing maxrows*maxcols and (pos%maxcols)==0 every time you print a number. You also left the declarations int row; int col; for unused variables there; compiling with warnings enabled should have warned you about that.

The third approach, with two nested for-loops, is the most straightforward way to make a grid. However, the implementation is marred by the unconventional loop counting, as mentioned above.

I think that the most elegant solution would be to decouple the grid-counting loops from the content-generating calculation:

int n = 0;
for (int row = 0; row < maxrows; row++) {
    for (int col = 0; col < maxcols; col++) {
        std::cout << std::setw(7) << (n += multiple);
    }
    std::cout << "\n";
}

You can also use that technique to produce a transposed table, though it's less elegant.

int stride = maxrows * multiple;
for (int row = 1; row <= maxrows; row++) {
    for (int col = 1, n = row * multiple; col <= maxcols; col++, n += stride) {
        std::cout << std::setw(7) << n;
    }
    std::cout << "\n";
}
share|improve this answer
    
Thanks for the constructive edits and the answer. All answers are good so I can't choose to accept one over others. – user31782 Mar 7 at 7:22

There is an excellent way to print tables using the conditional (ternary) operator.

For example, to print a weekly calendar, the C version is:

date=1 ; while(date <= monthLength)
         printf("%2d%c",date++,(date-offset) % 7 ? ' ' : '\n');

It works by appending a linefeed when (date-offset)%7 is zero, otherwise a space between table columns.

offset is chosen so the weekends are right for each month.

share|improve this answer
    
I don't know enough about C++ to implement this using cout. – Arif Burhan Mar 6 at 17:47
3  
Regardless, that still doesn't look very readable, so an if may be preferred. – Jamal Mar 6 at 17:58
    
Telling a beginner to write C while learning C++ is a bad idea. This is not an answer, it's an alternative implementation at best. This does not review the code given by the OP. – Mast Mar 15 at 11:22
    
Hi @Mast, this will work fine in C++ too, it's just not idiomatic. But that's not the point. The point is recommending the ternary operator, and an example to illustrate the point. It's not a great answer but it's an answer. – janos Mar 15 at 11:45
    
@janos You're right about the ternary operator being a valid recommendation, my bad. I'm aware provided code will work in C++, but it's a bad thing to recommend when a beginner is learning C++. OP is on the right track, I can imagine this answer being most confusing for a beginner. For instructions on how to ternary on cout (you shouldn't): see this SO answer. – Mast Mar 15 at 13:25

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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