I'd like to have this code criticized please

[skn]

This was a simple program I wrote to allow me to decide between multiple choices and also give it a personal touch.

I'd like to know if I have some bad habits, ways to improve it and such. I also want to know how my commenting skills are, and if I'd need more or less of them.

#include <iostream>
using std::cout;
using std::cin;
using std::endl;

#include <cstdlib>
using std::rand; // used to get a random number from seed
using std::srand; // get a seed for random
using std::string;

#include <ctime>
using std::time; // used to get amount of secounds for the seed

#include <sstream>
using std::stringstream; // used to convert from string to number


//start main
int main()
{
    int random_number; // For choosing a random "object"
    int object_amount; // the amount of "objects"

    string input; // used for input

    string objects[21]; // All the different "objects"

    cout << "Welcome to the random choice program!";
    while(true)
    {
        cout << "Please enter how many factors[2-20]" << endl;

        // loops to make sure that the user enters an accepted input
        while(true)
        {
            getline(cin,input);
            stringstream myStream(input);
            if(myStream >> object_amount && object_amount > 1 && object_amount < 21) // only accepts a number between 2 - 20
                break;
            cout << "Invalid input! Write a number between 2 and 20!" << endl;
        }//end loop

        srand(time(0));
        random_number = rand() % object_amount + 1;

        // goes trough the amount of objects and name each one
        for(int i = 1; i <= object_amount ; i++ )
        {
            cout << "Please write down the name of object nr" << i << ": ";
            getline(cin, objects[i]); cout << endl;
        }//end loop

        cout << "The choice is: " << objects[random_number] << endl << endl;

        cout << "Would you like to restart? ";
        getline(cin,input);

        //if user asks no then the program exists
        if(input[0] == 'n' || input[0] == 'N')
            break;
    }// end loop

    return 0; // successfull excecution
}//end main 

[hamsterman]

rand, srand, time and any other c functions are not in std namespace. It would be better to explicitly include <string>. Now <sstream> is including it, but if you ever remove <sstream> you should get an error and you may not see where it came from.
There is little point to include <sstream>. Same can be done with cin, with a little bit of code added. But then, I guess there is no harm here.
In C++, arrays start from 0. Since you start your for loops from 1, you're just ignoring the first element. That is why you declared an array of 20 elements as "objects[21]". This is wasteful and slightly confusing. You should fix that.
You're commenting a lot more than you need to. Most of the code explains itself. Unless you're doing something weird, it would be best if you only described each function in a couple of lines above it.
Also, you only need to call srand once. Here it does no harm. If the loop was fast, you might get not random results though.

[skn]

rand, srand, time and any other c functions are not in std namespace

^- I have a problem understanding this one, what exactly do you mean by that? From the C++ book i use he declares the namespace's for rand\srand and time the way I do and also includes those libraries.

--

So I should just have the array from 20, and instead using a + 1 and - 1 where it's needed? Such as this:

for(int i = 0; i <= object_amount - 1 ; i++ )
{
        cout << "Please write down the name of object nr" << i + 1 << ": ";
        getline(cin, objects[i]); cout << endl;
}//end loop 

--

I use the sstream for the input loop to make sure it's a number, as it turns into an unlimited loop if someone accidently give a non integer value with cin. I also read(might have been here can't remember) that this is a suggested form for input instead of using cin.


Thank's for the constructive criticism

[1vRz3TCk]

hamsterman wrote:

rand, srand, time and any other c functions are not in std namespace.

Svein Kristian Nykaas wrote:

I have a problem understanding this one, what exactly do you mean by that?

hamsterman is not entirely correct in this. The C++ standard says that the contents of the C headers shall be put in C++ header along the lines of name.h becomes cname. It also says that, in the C++ headers, declaration and definitions (except macros in C) are within the namespace scope of namespace std. So your book is correct.

-------------

So I should just have the array from 20, and instead using a + 1 and - 1 where it's needed? Such as this:

for(int i = 0; i <= object_amount - 1 ; i++ )
{
        cout << "Please write down the name of object nr" << i + 1 << ": ";
        getline(cin, objects[i]); cout << endl;
}//end loop  

If you have object_amount as the number of elements in a array:

for(int i = 0; i < object_amount ;  i++ )
{
        cout << "Please write down the name of object nr" << i + 1 << ": ";
        getline(cin, objects[i]); cout << endl;
}//end loop  

[skn]

Object_amount is a user input\choice. Which would be between 2-20, however unless I remove 1 from it(with the 20 array) i will always gain an higher value then planned. For instance:

user input = 2
then the for loop is supposed to loop 2 times, however unless i remove 1 from object_amount it would loop 3 times.

[alhypo]

Object_amount is a user input\choice. Which would be between 2-20, however unless I remove 1 from it(with the 20 array) i will always gain an higher value then planned. For instance:

user input = 2
then the for loop is supposed to loop 2 times, however unless i remove 1 from object_amount it would loop 3 times.

Notice that CodeMonkey replaced the less-than-or-equal operator with the less-than operator in your loop condition statement. Those two loops are functionally the same, but using a single operator and not subtracting 1 is much more concise.

[1vRz3TCk]

Note the less than rather than the less than or equal to.

Edit: Very slow typing on my part or not refreshing. :0)

[skn]

Ah of course my bad haha xD. Didn't even notice that. Thanks =).

Now onto the comments, is it good practice to use comments to indicate the end of a loop like I do? Or should I drop that?

[1vRz3TCk]

On small loops where you can clearly see the starting point, it is probably not worth it. If you have a large loop with other indentation due to other constructs, it can be worth putting a comment on it.

i.e.

        // Someocode here
            }
        }
    } // it may be worth commenting what block is being closed by the }
}

[skn]

Ah I see, so using it for the main loop could be good. but for the smaller few lines one I have thrown in all over it's not needed. I see, thanks :).

[zb0S216C]

I would incorporate some sort of thread sleeping function such as Sleep( ) (<Windows.h>) within any loop that uses many clock-cycles. Sleep( ) suspends the execution of the current process thread to allow the CPU to execute other threads. This effectively cuts down the the amount of clock-cycles (CPU usage) your program uses.

_Wazzak

[skn]

How would I know how many clock-cycles a loop would run trough? And how can I know what argument to use with Sleep?

I'm pretty new to programming and only learning trough the c++ book, no school or anything.

[zb0S216C]

Svein Kristian Nykaas wrote:

How would I know how many clock-cycles a loop would run trough? (sic)

Unless there's a function within your operating system's API, there's no true way of knowing.

Svein Kristian Nykaas wrote:

And how can I know what argument to use with Sleep? (sic)

Sleep( ) requires a single argument which specifies the length of time (in milliseconds) to suspend the process thread.

Svein Kristian Nykaas wrote:

only learning trough the c++ book, no school or anything. (sic)

Same here.


_Wazzak

[skn]

So not doing anything wrong by for instance, using 50 as an argument? As it depends on a need-to need type of thing?

Thanks for that code, quite useful to stop and pause the code for x-amount of seconds.

[zb0S216C]

Even 50ms is too long; 1ms would do the job. However, the time given depends on how much time you want to give to the CPU for the other threads.

_Wazzak

[ne555]

Framework wrote:

I would incorporate some sort of thread sleeping function such as Sleep( ) (...) This effectively cuts down the the amount of clock-cycles (CPU usage) your program uses.

¿Why would you do that?

[zb0S216C]

ne555 wrote:

¿Why would you do that?

So it's not using too many CPU clock-cycles, that's why; it's called greed otherwise.

_Wazzak

[1vRz3TCk]

Framework, I can't quite see where you are coming from on this.

[zb0S216C]

See here: http://siber.cankaya.edu.tr/OperatingSystems/ceng328/node116.html
The images aren't exactly pukka^[what?], but the text is what you want.

_Wazzak

[ne555]

Let the scheduler take care of that, don't worry about others.
Your program needs those precious cycles to accomplish its task.

Considering Svein Kristian Nykaas's code, ¿where will you put those sleep calls?