Code critique?

[seth23]

I made a simple code generator, for no particular reason and is not made to be a perfect pseudo-random number and letter generator, but give some sort of code. If anyone can find anyway I could improve this, just so I can learn from my mistakes and learn better coding practice I would greatly appreciate it.

I tried to put it in classes, but after many attempts I came unlucky :\ I also narrated the code.

#include <iostream>
#include <cstdlib>
#include <ctime>
using namespace std;

int main () {
    int z, x, w, y;
    char l, k; // declares the variables.

    cout << "Begin" << endl; //Signal to let me know the key has been generated.
    cout << "-------"<<endl;

    srand(time(NULL));
    for (int e = 0; e < 5; e++) {
//Does the follwoing 5 timmes.
    for (int i = 0; i < 4; i++)
    {
    z = rand() % (99 - 10) + 10;
    x = rand() % (99 - 10) + 10;
    }
// makes 4 random numbers.
            w = (rand() % 26);
            l = (char) (w+ 65);

            y = (rand() % 26);
            k = (char) (y+ 65);

            cout << z << l <<  x  << k << endl;


    }
}

[erikn]

Why the loop in lines 16-20?
Every time it iterates, it overwrites the z and x from the previous iteration without ever having done anything with them.

[S6k9GNh0]

Don't use "using namespace std;", it's generally bad given the amount of compile time you recieve. A quick compile-time optimization is to specify specifically which parts of the namespace std you are using.

You *do not* have to put everything in classes. As a matter of fact, I would suggest you not put everything into classes. Not everything in code can soundly relate to an object. This is not Java. Making a class named "Utility" which implements a bunch of static functions that do random things is essentially over complicated and pointless yet I see all too often.

Comments like "// makes 4 random numbers" and "//Does the follwoing 5 timmes." are generally bad as they don't actually provide you with any information you shouldn't have already known. When someone slightly experience in a language reads code, they read it like a talkable language. If they come across some syntax they don't recognize they look it up. However, if the syntax is used in a such a way that isn't usual or might be confusing (such as a function being used for unobvious reasons), a comment helps boost understanding of the code several times.

On lines 14 - 20, the tabbing is strange. Try and be consistent, it helps code clarity.

And the for loop on lines 16-20 is strange. A comment to explain why that's there would be great. ;)

[Cubbi]

In addition to what computerquip said, do not create objects at the beginning of a function, as in your lines 7 and 8; they are useless there. Only create them when necessary (Sutter/Alexandrescu C++ Coding Standards chapter 18 "Declare variables as locally as possible")

Also, the multiple calls to endl are unnecessary (\n is sufficient in all cases here)

The magic numbers 65 and 26 should be replaced by something meaningful.


Just for variety, here's an example of how this program could look in modern C++:

#include <iostream>
#include <random>
int main ()
{
    std::random_device rd;
    std::mt19937 eng(rd());
    std::uniform_int_distribution<> digits(10, 99), letter('A', 'Z');

    std::cout << "Begin\n"
              << "-------\n";
    for (int e = 0; e < 5; ++e)
        std::cout << digits(eng) << static_cast<char>(letter(eng))
                  << digits(eng) << static_cast<char>(letter(eng)) << '\n';
}

tested with gcc 4.6.2, clang++ 3.0, and Visual Studio 2010
online demo (without hardware seed): http://ideone.com/yeA8f

[Caprico]

Also, the multiple calls to endl are unnecessary

Could you please explain this? I mean, does it make a difference in efficiency or speed or memory?

[Cubbi]

does it make a difference in efficiency or speed or memory?
Certainly: endl calls flush which calls pubsync which suspends the program and calls the OS.

[Caprico]

Ooooh... didn't know that... I thought all it did was a call to insert newline character... Thanks! :D

[seth23]

Wow! Great stuff! Ok, I am using code::blocks as my compiler what is the significance in not declaring the namespace at the top? And the second for loop I forgot to remove I don't know why I didn't remove it. @Computerquip I already knew what they mean and I didn't put the comments on there until I posted it to here. This was just to show what it did to the whole group. But about the endl, I wanted it to print on a new line every time? Is there a more modern or more resourceful way of doing this? Every time I tried to run it using std:: on the
lines with endl they would give error messages?

You are totally right about using the std:: it makes it compile a lot quicker... 5.27 to 4 seconds.

[S6k9GNh0]

seth23, std::endl also flushes the stream among other things. It's easier to send a message as a whole (no endl) and then flush at the end (std::flush) to make sure the message is printed.

However, given the linear fashion of how console logging goes, what you did is just fine. Also, C++ streams are known to be very heavy and alternative methods of logging and output are often used when in a more resource intensive application.

About the error message, be sure you're using std::endl.

#include <iostream>

int main()
{
    std::cout << "This is a test" << std::endl;
}

[Cubbi]

It's easier to send a message as a whole (no endl) and then flush at the end (std::flush)

explicit flushing (either endl or flush) is almost never needed in beginner programs. You won't (usually) see them in Stroustrup's code examples: http://www2.research.att.com/~bs/3rd_code.html . One place it is needed, though, is right before the controversial system("pause").

[seth23]

Ok, thanks for the help you guys. I am needing a little clarification however. Ok, I don't see a lot of difference in the compile time in Code::Blocks when I use using namespace std; compared too std::cout etc. It is a lot easier just to predefine the whole namespace of the program than to tag each little section that needs to be named.

And another thing is, what is the "preferred" method of making a new line? "\n"? I know not to do the system("pause) I heard that is very unnecessary and sloppy. Pretty much any pause I do is the old C

#include <iostream>
#include <conio.h>
using namespace std;
int main ()
{
    cout <<"Preferred pausing method";
     getch();
}

}

If you know any better way I would appreciate it.

[S6k9GNh0]

#include <iostream>
// #include <conio.h> //This is a big no no...
//using namespace std;
int main ()
{
    using std::cout;
    using std::endl;
    using std::cin;

    cout <<"Preferred pausing method" << endl;
    cin.get();
}

}

There are other reasons outside of just compile bencharks. It clumps the namespace and it often reduces code clarity. Maybe others?

[1vRz3TCk]

Ok, I don't see a lot of difference in the compile time in Code::Blocks when I use using namespace std; compared too std::cout etc. It is a lot easier just to predefine the whole namespace of the program than to tag each little section that needs to be named.

Compile time should be the least of your worries, you need to know the differences between using Declarations and using directive if you don't use fully qualified names.
see: http://www.cplusplus.com/forum/beginner/9181/#msg42419

[seth23]

Ah, that makes sense. Thanks.

[LB]

The comment on line 15 is confusing, at first I thought it was referring to what is directly below it (almost all comments refer to what is to their left or to what is below them). Then I realised it was talking about the loop above it...