1
\$\begingroup\$

I've written this code to get input from a file and count the repetition of number of that file and give output to another file. How can I make this code better?

#include <iostream>
#include <cmath>
#include <fstream>

using namespace std;

int main()
{
    ofstream outputFile("outPutData.txt");
    ifstream theFile ("inputData.txt");
    int MaxRange= 50;

    int myint[100]={0};

    int mycompare[90]={0};

    int mycount[90] = {0};

    int i = 0, j = 0, k = 0, sum = 0;

    for(j=0;j <= MaxRange;j++){
        mycompare[j] = j;
    }

    do
    {
        theFile>>myint[i];

        for(j=0;j<=MaxRange;j++)
            for(k=0;k<=MaxRange;k++)
            {

                j = k;
                if(myint[i] == mycompare[j])
                    mycount[k] = mycount[k]+1;
            }
        i++;

    }
    while((myint[i-1] >=0) && (myint[i-1] <= MaxRange));

    cout<< "The Number\t\t Frequency"<<endl;
    cout<<"------\t\t------"<<endl;


    for(k=0; k <=MaxRange ;k++)
    {

        if(mycount[k] != 0)
        {
            outputFile<<k<<"  "<<mycount[k]<<endl;
            cout<< k<<"\t\t"<<mycount[k]<<endl;

            sum = sum + mycount[k];
        }
    }

    cout<<"Total input from file  = "<<sum<<endl;

    return 0;
}

If I have the series of number in my input file like this:

9
9
9
6
6
5
3
7
9
1
2

It gives the output:

The Number  Frequency
---------   --------
1             1
2             1
3             1
5             1
6             2
7             1
9             4
\$\endgroup\$
3
  • 1
    \$\begingroup\$ The first thing I would suggest to do is to improve the formatting of the code. Unneeded spaces like after the #includes you need 1 space so why use 3? It isn't that big a code. No need for the spaces between variable declaration either. You aren't consistent in your style. A for loop contained another for loop but you didn't use braces but in case of if you used braces. Also MaxRange doesn't change so why not make that a macro? \$\endgroup\$ Jul 28, 2013 at 15:27
  • \$\begingroup\$ @AseemBansal : I have edited my code & reduced the spaces. If I have the maximum value greater than 50 than it won't run because of the 'while condition' below. That's why I declared it at the beginning in case I need to change the value. \$\endgroup\$
    – aries0152
    Jul 28, 2013 at 15:48
  • \$\begingroup\$ @aries0152: During runtime, you mean? Still doesn't seem right. If you mean before runtime, then it can still be a const. Plus, you wouldn't want to accidentally alter that value somewhere else. \$\endgroup\$
    – Jamal
    Jul 28, 2013 at 17:20

3 Answers 3

4
\$\begingroup\$

Please do not do this:

using namespace std;

I know every stupid shitty book about C++ does this. But don't. It works fine if your code is 10 lines long but anything larger it becomes a problem. So it is a really bad habit to start and breaking habits is hard so don't start it. Do yourself a favour and get into the habit of not using it. See Why is “using namespace std;” considered bad practice? for details.

This looks like it should be a constant:

int MaxRange= 50;

You should make it const. Then you can also use it as the size of the arrays below.

Declare one variable per line:

int i = 0, j = 0, k = 0, sum = 0;

The is one corner case where it does not do what you expect. But that's not the real reason for not doing this. Readability. And declare the variable as close to the point of use as possible. No point in declaring a variable that is not used until the bottom line of a program. Also loop variables should be declared as part of the loop so there affect does not bleed beyond the loop.

for(int loop = 0; loop < 5; ++loop)
{
}
// loop not valid here.

Also use better names. If you have a loop that covers multiple lines. Try and find all uses of the variable i. Your editor is going to hit a bunch of words in comments and other things. Which is a real pain when you are trying to fix code.

Why do you only initialize half of the mycompare. It may be valid but a comment on why would definately be in order.

int MaxRange= 50;
int mycompare[90]={0};
for(j=0;j <= MaxRange;j++){
    mycompare[j] = j;
}

After reading the code it seems that mycompare is not really needed.
Even in your loop to print out the values you don't use it.

This is an anti pattern for reading from a stream. What happens if the read (operator>>) fails?

do
{
    theFile>>myint[i];
    // STUFF
}
while((myint[i-1] >=0) && (myint[i-1] <= MaxRange));

The correct patter for reading from a file is:
Note: You will find that nearly all programming languages work this way. It is a VERY common pattern. Put the read as part of the loop conditional to validate the read worked.

while(theFile>>myint[i])
{
    // Read worked so do work.
}

You can combine your other conditions:

while((theFile>>myint[i]) && (myint[i] >=0) && (myint[i] <= MaxRange))
{
    // Read worked and value in the correct range.
}

Try to always use {} around block statements. It will save your butt one day.

    for(j=0;j<=MaxRange;j++)
        for(k=0;k<=MaxRange;k++)

Prefer

    for(j=0;j<=MaxRange;j++)
    {
        for(k=0;k<=MaxRange;k++)
        {
        }
    }

Not sure what the outer loop is doing here.

    for(j=0;j<=MaxRange;j++)              // This line seems redundant.
        for(k=0;k<=MaxRange;k++)
        {
            j = k;                        // because of this line.
            if(myint[i] == mycompare[j])
                mycount[k] = mycount[k]+1;
        }

You may as well remove the outer loop.

Prefer not to use std::endl to mark end of line. Perfer '\n'. The difference is that std::endl puts the '\n' character on the stream then flushes the stream. If you don't need the flush do not use it. It makes the stream much more efficient not to use it.

cout<< "The Number\t\t Frequency"<<endl;
cout<<"------\t\t------"<<endl;

In C++ the return in main() is optional.
If you only return 0 then I would encourage you not put the return in the code. It is an indication that the program can not logically fail.

return 0;

Conclusion:

You need to learn how to use the built in types. This problem becomes trivial to do by using the std::map.

int main()
{
    std::map<int,int>   data;
    int  tmp;

    // Read the data
    ifstream theFile ("inputData.txt");
    while(theFile >> tmp)
    {
        ++data[tmp];
    }

    // Write the data 
    for(auto loop = data.begin(); loop != data.end();++loop)
    {
         std::cout << loop->first << "\t\t" << loop->second << "\n";
    }
}
\$\endgroup\$
5
  • \$\begingroup\$ Thanks for the explanation. Esp. for the conclusion. This code is nice and simple. Is there anyway that it count the total number of input from the file? \$\endgroup\$
    – aries0152
    Jul 28, 2013 at 17:02
  • \$\begingroup\$ Shouldn't the loop in the conclusion use .cbegin() and .cend() since you're just printing? Solid answer otherwise. Will vote as soon as I get them all back. \$\endgroup\$
    – Jamal
    Jul 28, 2013 at 17:08
  • \$\begingroup\$ Bah, I think you posted this answer right after I started mine, which is completely redundant by now. +1 anyway, good answer. \$\endgroup\$
    – Lstor
    Jul 28, 2013 at 17:10
  • \$\begingroup\$ @Jamal: Yes. That would be better. \$\endgroup\$ Jul 28, 2013 at 17:10
  • \$\begingroup\$ @Lstor: You still made some needed points. :-) I won't even bother to answer now (it'll be all the same stuff), but that's okay. \$\endgroup\$
    – Jamal
    Jul 28, 2013 at 17:13
2
\$\begingroup\$

Short version:

  • Sort your #includes alphabetically.
  • Don't use using namespace std.
  • Use meaningful variable names. Never prefix a variable with my -- it is never meaningful.
  • Use logical code grouping. Having empty lines between variable declarations doesn't make sense.
  • Declare variables as late as possible.
  • Why is the output file stream called outputFile (which is an okay name), but the input file stream is called theFile (which is not an okay name)? I suggest using simply output and input.
  • Some variables start with a capital letter, others do not. Be consistent. It's common to use lower-case for variable names.
  • You should almost never use arrays. Normally you want to use std::vector instead.
  • When you want to increase a variable by the value of another, prefer +=. For example, sum += mycount[k] is more readable than sum = sum + mycount[k].
  • Don't use std::endl unless you really need to. std::endl flushes the output as well as add a newline. If you just want a newline, do << '\n' instead.
  • You can use std::map to keep track of the numbers.
  • I don't really understand what you use mycompare for.
  • Use whitespace between operators and other places where it makes sense, to increase readability.
  • Either keep block statements on a single line, or use braces.

Explanations:

  • Declare variables as late as possible.

Declaring a variable as late as possible keeps the declaration closer to the relevant context. It might also be more efficient in cases where you never need to instantiate the variable at all. for loops should look like this, unless you really need to reuse the index variable later:

for (int i = 0; i < whatever; ++i) {
    // ...
}

Reuse the name i in the next for-loop. You can put the opening brace on a line by itself if you want to.

  • Prefer std::vector over arrays.

std::vector can grow, can be range-checked and has a lot of other advantages. Normally when you think "array", use std::vector. To access the variable at index i, use numbers.at(i) (where numbers is a vector). Using at(i) instead of [i] will cause an error (exception) if you go out of range -- and you want that.

  • Using std::map instead.

A std::map is an associative array (also known as a dictionary). You associate a key with a certain value. You can later get the value stored at a certain key. Like this:

std::map<int, int> numberCounts;
while (condition) {
    int number;
    input >> number;

    numberCounts[number] += 1; // Increase the count of `number` by one.
}

(You can also write ++numberCounts[number] if you prefer. Also, you should put the input reading as condition, but that's not the point here.)

Note that key/value pairs will be created when and only when you need it. This means that the snippet above will work for (almost) any number. But, even if number is 1,000,000 (one million), it will not create room for one million key/value pairs. This is in contrast with your solution. Your solution creates room for all numbers up to 90, regardless of whether or not they are read in the file. But if your file reads a number that is too high, your program will not count it.

\$\endgroup\$
0
\$\begingroup\$

By consistent style I meant something like this. If you use braces when a statement like if, for etc. contains single statement then use braces for if, for etc. containing single statements everywhere.

#include <iostream>
#include <cmath>
#include <fstream>

using namespace std;

int main()
{
    ofstream outputFile("outPutData.txt");
    ifstream theFile ("inputData.txt");

    int MaxRange = 50;
    int myint[100] = {0};
    int mycompare[90] = {0};
    int mycount[90] = {0};
    int i = 0, j = 0, k = 0, sum = 0;

    for (j = 0; j <= MaxRange; j++){
        mycompare[j] = j;
    }

    do
    {
        theFile>>myint[i];

        for(j = 0; j <= MaxRange; j++){
            for(k = 0; k <= MaxRange; k++)
            {
                j = k;
                if(myint[i] == mycompare[j]){
                    mycount[k]++;
                }
            }
        }
        i++;
    }while((myint[i-1] >=0) && (myint[i-1] <= MaxRange));

    cout<< "The Number\t\t Frequency"<<endl;
    cout<<"------\t\t------"<<endl;


    for(k=0; k <=MaxRange ;k++)
    {
        if(mycount[k] != 0)
        {
            outputFile<<k<<"  "<<mycount[k]<<endl;
            cout<< k<<"\t\t"<<mycount[k]<<endl;

            sum += mycount[k];
        }
    }

    cout<<"Total input from file  = "<<sum<<endl;

    return 0;
}

I changed some statement like sum = sum + mycount[k]; to sum += mycount[k] for readability.

I just commented on basic things like style. I'll leave the review to someone who knows C++.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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