1
\$\begingroup\$

I'm in my first-semester coding class and it's using c++. I recently finished this project to create a typing tutor and received full credit, but I want to find ways and opinions to optimize it as my goal is to improve. And full credit with no professor comments won't help me.

The goal is to have a randomly generated five-length string that can include a wild character [0-9] 10% of the time and another wild [%-!] any non alphanum symbol the other 10% of the time. The remaining 80% is upper and lowercase letters. (using ascii)...

You gain 500 points by typing correctly in under 10 seconds and lose points based off of passing the 10-second mark or typing a wrong character. The penalty for the wrong character is the abs(the difference between requested ascii and input ascii)...

I'm not looking for anyone to rewrite it, just some areas or redundancies and tips on how I can shorten the code.

#include <iostream>
#include <chrono>
#include <ctime>
#include <cmath>
#include <string>
using namespace std;

string generate(const int length);
string interpret(string generated);
int scoreOffset(string inputString,string randoString);
void withinInterval(int offset, int time);
void intervalFail(int offset, int time);

int main()
{
    int totalScore = 2000;
    int offset,totalTime;
    srand(time(0));
    string randoString;
    string inputString;
    string interpreted;

    while(totalScore > 0 && totalScore < 5000)
    {
        randoString = generate(5);
        interpreted = interpret(randoString);
        cout << "Your current points " << totalScore << ", just type -> " << randoString << ": ";
        chrono::steady_clock::time_point begin = chrono::steady_clock::now();
        cin >> inputString;
        chrono::steady_clock::time_point end= chrono::steady_clock::now();
        totalTime = chrono::duration_cast<chrono::milliseconds>(end - begin).count();


        if (totalTime <= 10000)
        {
            offset = scoreOffset(interpreted,inputString);
            if(offset == 0)
            {
                totalScore += 500;
                withinInterval(offset,totalTime);
            }
            else
            {
                totalScore -= offset;
                withinInterval(offset,totalTime);
            }
        }

        else
        {
            offset = scoreOffset(interpreted,inputString);
            totalScore -= (totalTime-10000);
            if(offset == 0)
            {
                totalScore += 500;
                intervalFail(offset,totalTime);
            }
            else
            {
                totalScore -= offset;
                intervalFail(offset,totalTime);
            }
        }
    }

    if(totalScore < 0)
        cout << "You lose..." << endl;
    else
        cout << "You win..." << endl;

}

string generate(const int length)
{
    string str = "";

    for(int i=0; i<length; i++)
    {
        int randomElement = (rand() % 100 + 1);
        if (randomElement <= 40)
            str += (rand() % 26 + 65);
        else if (randomElement <= 80)
            str += (rand() % 26 + 97);
        else if (randomElement <= 90)
        {
            str += "[%-!]";
        }
        else
        {
            str+= "[0-9]";
        }
    }
    return str;
}

string interpret(string generated)
{
    int length = generated.length();
    string str;
    for(int i=0; i<length; i++)
        {
            if(generated[i] == '[' && generated[i+1] == '0')
            {
                str += '0';
                i+=4;
            }
            else if(generated[i] == '[' && generated[i+1] == '%')
            {
                str += '*';
                i+=4;
            }
            else
                str += generated[i];
        }
    return str;
}


int scoreOffset(string randoString,string inputString)
{
    int offset = 0;
    int length = randoString.length();
    for(int i = 0; i<length;i++)
    {
        if(isupper(randoString[i]))
        {
            if(isupper(inputString[i]))
                offset += 0;
            else
                offset += abs((char(inputString[i])) - (char(randoString[i])));
        }
        else if(islower(randoString[i]))
        {
            if(islower(inputString[i]))
                offset += 0;
            else
                offset += abs((char(inputString[i])) - (char(randoString[i])));
        }
        else if(isdigit(randoString[i]))
        {
            if(isdigit(inputString[i]))
                offset += 0;
            else
                offset += abs((char(inputString[i])) - (char('0')));

        }
        else if(isalnum(randoString[i]) == false)
        {
            if(isalnum(inputString[i]) == false)
                offset += 0;
            else
                offset += abs((char(inputString[i])) - int(char('*')));
        }
    }
    return offset;
}

void withinInterval(int offset,int time)
{
    cout << time << " milliseconds, you made it within the interval of 10,000..." << endl;
    if(offset != 0)
        cout << "String offset is " << offset << ", your total penalty is " << offset << endl;
}

void intervalFail(int offset, int time)
{
    cout << time << " milliseconds, you *failed* it within the interval of 10,000..." << endl;
    if(offset != 0)
        cout << "String offset is " << offset << ", your total penalty is " << offset << endl;
}
\$\endgroup\$
1

1 Answer 1

4
\$\begingroup\$

Random Numbers

The old random number generator:

srand()
rand()

Have been replaced by C++ versions (these are from the old C library). They are known to be not very random and have several issues that make them hard to use correctly.

You think this would give you an even distribution of numbers in the range 1..100 ?

 (rand() % 100 + 1)

Not quite. Because the maximum random number is MAX_RAND (lets assume 32768 for this discussion but see your documentation). Basically the first 68 numbers have a slightly higher probability than the last 21 numbers.

What you should have done was:

 int r = rand();
 while(r > 32700) {
     r = rand();
 }
 int result = r % 100 + 1;

Or you could have divided by MAX_RAND to get a number in the range [0-1).

 double result = rand() / MAX_RAND;

But even better than all this would have been to use the new random number generator.

std::default_random_engine         generator;
std::uniform_int_distribution<int> distribution(1, 100);
int                                result = distribution(generator);

Dry up your code

Don't repeat yourself (DRY)

Some of the code is overly verbose. I think you can tighten some of this down.

    if (totalTime <= 10000)
    {
        offset = scoreOffset(interpreted,inputString);
        if(offset == 0)
        {
            totalScore += 500;
            withinInterval(offset,totalTime);
        }
        else
        {
            totalScore -= offset;
            withinInterval(offset,totalTime);
        }
    }

    else
    {
        offset = scoreOffset(interpreted,inputString);
        totalScore -= (totalTime-10000);
        if(offset == 0)
        {
            totalScore += 500;
            intervalFail(offset,totalTime);
        }
        else
        {
            totalScore -= offset;
            intervalFail(offset,totalTime);
        }
    }
}

Can be written like this:

    offset = scoreOffset(interpreted,inputString);

    if (totalTime <= 10000)
    {
        if(offset == 0)
        {
            totalScore += 500;
        }
        else
        {
            totalScore -= offset;
        }
        withinInterval(offset,totalTime);
    }
    else
    {
        totalScore -= (totalTime-10000);
        if(offset == 0)
        {
            totalScore += 500;
        }
        else
        {
            totalScore -= offset;
        }
        intervalFail(offset,totalTime);
    }
}

Notice how I have pulled common elements out of if blocks to a higher scope. Now that we have done this it becomes easier to read and leads to more clean up.

    offset = scoreOffset(interpreted,inputString);

    if (totalTime <= 10000)
    {
        totalScope = totalScore + (offset == 0) ? 500 : -offset;
        withinInterval(offset,totalTime);
    }
    else
    {
        totalScore -= (totalTime-10000);
        totalScope = totalScore + (offset == 0) ? 500 : -offset;
        intervalFail(offset,totalTime);
    }
}

OK. One more tidy up.

    offset = scoreOffset(interpreted,inputString);
    totalScope = totalScore + (offset == 0) ? 500 : -offset;

    if (totalTime <= 10000)
    {
        withinInterval(offset, totalTime);
    }
    else
    {
        totalScore -= (totalTime - 10000);
        intervalFail(offset, totalTime);
    }
}

Always use braces

    if (randomElement <= 40)
        str += (rand() % 26 + 65);
    else if (randomElement <= 80)
        str += (rand() % 26 + 97);

There is technically nothing wrong with this code. But missing braces can potentially lead to very hard to find bugs (especially when there are C macros around). So just to be on the safe side always use braces around subblocks (its a good habit to get into).

    if (randomElement <= 40) {        // I have started using this style for sub-blocks.
        str += (rand() % 26 + 65);    // But use what you like.
    }
    else if (randomElement <= 80) {
        str += (rand() % 26 + 97);
    }

Another Dry situation

These two look identical. I am sure you can factor out the identical parts into their own function.

void withinInterval(int offset,int time)
{
    cout << time << " milliseconds, you made it within the interval of 10,000..." << endl;
    if(offset != 0)
        cout << "String offset is " << offset << ", your total penalty is " << offset << endl;
}

void intervalFail(int offset, int time)
{
    cout << time << " milliseconds, you *failed* it within the interval of 10,000..." << endl;
    if(offset != 0)
        cout << "String offset is " << offset << ", your total penalty is " << offset << endl;
}
\$\endgroup\$
1
  • \$\begingroup\$ It would be better to use discrete distribution to choose from three kinds, then use uniform to choose one from one kind. May be you meant that, but your example got me a little bit confused. \$\endgroup\$ Commented Mar 22, 2018 at 19:18

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.