Can i make this code easier

[Ch1156]

Is there any way i can shorten this code, also any other tips on things i can do?

#include <iostream>
#include <string>
#include <ctime>
#include <random>
#include <fstream>

using namespace std;

void RPS(int &Time)
{
    switch(Time)
    {
        case 0:
            cout << "Paper" << endl;
            break;
        case 1:
            cout << "Rock" << endl;
            break;
        case 2:
            cout << "Scissors" << endl;
            break;
    }
}

void conditionTimer(int &choice, int &Time, int &score, bool &end)
{
    if((choice == 1) && (Time == 1))
    {
        cout << "You tied with the computer!" << endl;
        score -= 1;
    }
    else if((choice == 2) && (Time == 0))
    {
        cout << "You tied with the computer!" << endl;
        score -= 1;
    }
    else if((choice == 3) && (Time == 2))
    {
        cout << "You tied with the computer!" << endl;
        score -= 1;
    }


    if((choice != 1) && (Time != 1))
    {
        score += 1;
    }
    else if((choice != 2) && (Time != 0))
    {
        score += 1;
    }
    else if((choice != 3) && (Time != 2))
    {
        score += 1;
    }

    if(score == 12)
    {
        end = true;
    }
}

int main()
{
    int choice;
    ofstream fileOut("RPS.txt");
    ifstream fileIn("RPS.txt");
    bool end = false;
    int score = 0;
    fileIn >> score;
    time_t T;
    time(&T);
    srand(T);
    int Time;

    while((end != true) && (choice != -1))
    {
        for(int i = 0; i < 10; i++)
        {
            Time = rand() % 3;
        }

        cout << "1) rock" << endl;
        cout << "2) paper" << endl;
        cout << "3) scissors" << endl;
        cout << "Enter -1 to quit at any time" << endl;
        cin >> choice;

        cin.get();
        cout << "\nThe computer picked: ";
        RPS(Time);
        conditionTimer(choice, Time, score, end);
        cout << "\nYour current score is: " << score << endl;
        cout << "\n";
    }

    if(choice == -1)
    {
        end = true;
        fileOut << score;
    }
}

[Ch1156]

bump

[andywestken]

Well, I wouldn't start off worrying about making it shorter to start with. First I'd make it clearer.

#1 Some of your variable and function names could do with improving. Esp. "Time". It looks like the computers choice, rather than the time (Time already has a meaning). And the name conditionTimer() doesn't tell me what the function does.

#2 You could replace 1, 2, 3 (and 0, 1, 2) with enum values (if you're familar with them), or even consts or #defines. Then the code would read more easily. This will require you to subtract 1 from the user input, or add 1 when you generate the random value, but then you could treat the user and computer choices consistently. As it stands, the code is using different numbers for the user and computer, which is a bit confusing.

(This will allow you to simplify the check for a tie. And there may well be an opportunity to reduce the amount of code in "conditionTimer()" if you use clock arithmetic?)

#3 You could tighten up the scoping of some variables a bit (e.g. fileOut, choice)

(I would consider splitting main() up into function: e.g. loadScore(), playGame(), saveScore())

#4 I don't see the need for the little loop round rand(). It should be enough to call it just once.

#5 if(choice == -1) looks like it should be inside the while loop.

#6 While not actually required, it is good form to return 0; from main()

#7 int choice; should be initialized before its first use.

(It is good form to initialize all variables, ideally at point of first use.)

#8 void RPS(int &Time) should be void RPS(int Time) as Time is not an out or in/out parameter.

(And look for same issue elsewhere!)

#9 you could add code to check that a meaningful choice was entered (what happens if you enter a random letter?)

#10 I'd rework the way you display the choice a bit (to make it more cout friendly)

Aⁿd_y

[Incis B]

AndyWestken hit those points "Right on the nail" !

I would only add a few additional points;

#1 The ConditionalTimer loop seems to have a lot of "Fall Through" without being tested for or accounted for. Make sure all bases are covered and documented as they will be the ones to come back to haunt you later.

#2 Time, time, and t seem to be very scary names. They come close to being both Reserved Key Words, and so cryptic that they may cause confusion later.

First, pick names that are very descriptive. Such as, in this case,
i_Rnd_choice_time.
Note, I always place the TYPE as the first char of Vars.
This is also true for your var CHOICE, maybe something like:
i_Usr_choice,
so that it is (very) clear to you and to others who may read this, as to exactly what this var is, how it is being used, and it's type.

This is also true for RPS. Looking quickly at the code, I have NO idea what an RPS is, or is for - not without digging through the code to figure-it-out.

#3 You can always find ways to shorten code, but remember that it gets compiled into shorter code anyway by the compiler. This level- this high level- of code ( called C / C++) is for humans to read - so it doesn't need to be too tight - unless, of course, you are writing tight code for say an embedded system - for example.

Add comments. Comments will not help your code to run better or faster, but it will save you hours the next time you or someone else returns to fix or enhance this program.

#4 in Main() you init'l some vars, and write some code, then init'l some more vars. This makes reading and debugging a lot more difficult.

Group your vars to be init'lized together & at point of use.
int time,choice,score =0;
See Andy's #7 (above)

#5 ( same as Andy's) , looks like an ELSE condition of the WHILE.

#6 you have three conditions, with the same three results:
score += 1;
You may want to combine all these seperate conditions into a single condition.
else if((choice != 2) && (Time != 0))

IF ( [ ( A) && (B) ] OR [ (C) && (D) ] OR [ (E) && (F) ] )
THEN score += 1;

Because all three groups are closely related in function, then they can be logically grouped into, in this case, one statement, which is easier to read, and ( maybe) faster to run. Remember to comment its purpose.

#7 Always spend extra time on your DESIGN first. By spending 60% of the overall time on design development you reduce your debugging time to 15%.
(Otherwise you end-up spending as long or sometimes longer fixing your code than you did to write it in the first place.)

-Enjoy,

[Ch1156]

Ok i'll work on all of those points, thanks!, now i had another question i was wondering, does it matter that i put the functions before main? could any problems arise?

[andywestken]

does it matter that i put the functions before main? could any problems arise?

It makes no odds if you put the functions before or after main. In the latter case you'd need forward declarations, of course. But the compiler and linker will put the object code where they want, irrespective of where it's located in a file (or files).

[TheIdeasMan]

I prefer functions declarations before main, with definitions after. This makes it easier to read because I don't have to scroll past all the functions to see what main does. This has been the convention for a very long time - since the beginning of the C language in the 1970's.

Here is a link to some code I did to help someone else:

http://www.cplusplus.com/forum/beginner/84889/#msg455481

It shows how to exit out of a loop, that is less complex.

Hope all goes well.

[andywestken]

I have to admit that my current habit is to put the function before main. But then I always code the actual functionality in a function, or functions, so my main generally comes at the end. But I do put all non-inline utility functions after the main work function.

I got into this habit as "main" does not say what the function does. Putting the actual code in a function allows me to give it a name. (I didn't come up with this idea; I bumped into it somewhere or else)

#include <iostream>
#include <fstream>
#include <etc>

int utility();
void anotherUtility();

int stripBlanks(const char* filePath) {
    // todo

    // All the actual work is done here
    // so THIS is this is what I want to see first!
}

int utility() {
    // todo
}

void anotherUtility() {
    // todo
}

// main just deals with the command line args and then
// calls the actual work function
int main(int argc, char* argv[]) {
    int retCode = 0;

    if(2 != argc) {
        std::cerr << "stripblanks <filepath>\n";
    } else {
        const char* filePath = argv[1];
        retCode = stripBlank(filePath);
    }

    return retCode;
}

Aⁿd_y

[Chervil]

I dare say this is a matter of preference, but I usually follow the same pattern as TheIdeasMan.

My feeling is that until main() has been examined, there's no way of knowing whether any of the other functions are called at all, so there's no point in spending any time even glancing at them first.

[Chervil]

One comment regarding this function:

void RPS(int &Time)
{
    switch(Time)
    {
        case 0:
            cout << "Paper" << endl;
            break;
        case 1:
            cout << "Rock" << endl;
            break;
        case 2:
            cout << "Scissors" << endl;
            break;
    }
}

Although comments have already been made regarding meaningful names and so on, my primary thought was, why not just use an array of strings? Then instead of of a list of different cases, the code would simply read:

string items[] = { "Rock", "Paper", "Scissors" };

cout << items [CChoice] << endl; 

In fact doing it that way, function RPS() is not even needed.

I also agree with the suggestions to use enum, perhaps like this: enum choices {rock, paper, scissors, quit};
It does help a great deal in this case to improve readability, and thus makes the coding easier and more likely to be error-free.

[Incis B]

2cents worth,

I began programming a few years before "C" appeared, which, by-the-way helps to explain why I am still in the beginners forum, - so I tend to place all my code in the func()'s as are needed ( same as Andy), then I simply "understand" that main() goes at the end of the program rather than the top as a lot of other languages do.

I can see, however, some reasoning behind the idea of placing the def's at the beginning.
If you have a large application ( .gt. 100K lines of code)
and multiple files ( various .h, .dll. & .cpp ) with a makefile to make sense of the whole build process, - then having the def's at the top of each file would make the debugging process much easier - by seeing all the func def's up-front - (rather than searching for them inside the code).

-to each, his own.

edit: oops, Andy, I see that I am not using the same style as you do, but it looks like a good one, and one that I'll try using. Tnx !