CODEWITHSUNDEEP
c++
B Smith
B Smith

Reputation: 89

Can this if-else statement be made cleaner

I am trying to improve a C++ assignment to make it more efficient. I am a beginner with the language (and programming in general too), so I am only using what I know so far (if, else). I have a function that converts scores into levels, so anything under 30 = 1, 30-49 = 2, 50-79 = 3 and so on...

Here is how I am doing it:

if (score1 <= 30) level1 = 1;
else if (score1 <= 49) level1 = 2;
else level1 = 3;

if (score2 <= 30) level2 = 1;
else if (score2 <= 49) level2 = 2;
else level2 = 3;

//etc...

Is there a better way to do this, as I am aware this will require a new line for every single score I have.

Upvotes: 8

Views: 905

Answers (9)

The Archetypal Paul
The Archetypal Paul

Reputation: 41749

This rather depends on what you mean by efficiency. You could keep the limits for each level in an array

int level_limits[] = {0, 30, 49, 79, [...]};

int getLevel(int score)
{
   int level;
   for (level = 0; level < N_LEVELS; ++level)
       if (level_limits[level] > score)
            return level;
   return level; // or whatever should happen when you exceed the score of the top level
 }
 ...

 level1 = getLevel(score1);
 level2 = getLevel(score2);

... or something like that.

Upvotes: 13

bitmask
bitmask

Reputation: 34618

If you have less levels than fingers on your hands, you should use Paul's suggestion. However, if the number of levels are bigger, you could use binary search if your level thresholds are fairly uniform (and strictly increasing). Then you can get close to logarithmic runtime (I mean, you were asking for efficiency).

This is a good compromise between the lookuptable-suggestion and the linear search, but again, it only pays if you have a lot of levels.

Oh, by the way, if there is a pattern in how the thresholds are chosen, you should use that instead of searching for the right level.

Upvotes: 1

D_K
D_K

Reputation: 1420

You can utilize modulo and a hash table in order to achieve some code style elegance. Pseudo code:

int getLevel(int num)
{
   hash_table = (30=>1, 49=>2, 79=>3);
   foreach threshold (keys hash_table) {
      if (num modulo (threshold + 1) == num) {
        return hash_table[threshold];
      }
   }
}

Upvotes: 1

vitaut
vitaut

Reputation: 55524

First of all factor out the code for computing the level into a separate function, say get_level:

level1 = get_level(score1);
level2 = get_level(score2);

You can implement get_level in different ways.

If the number of levels is small you can use the linear search:

const int bounds[] = {30, 49, 79}; // Add more level bounds here.

int get_level(int score)
{
    const size_t NUM_BOUNDS = sizeof(bounds) / sizeof(*bounds);
    for (size_t i = 0; i < NUM_BOUNDS; ++i)
        if (score <= bounds[i])
            return i + 1;
    return NUM_BOUNDS + 1;
}

Or, if you are an STL fan:

#include <algorithm>
#include <functional>

const int bounds[] = {30, 49, 79}; // Add more level bounds here.

int get_level(int score)
{
    return std::find_if(bounds,
        bounds + sizeof(bounds) / sizeof(*bounds),
        std::bind2nd(std::greater_equal<int>(), score)) - bounds + 1;
}

If you have many levels binary search may be more appropriate:

#include <algorithm>

const int bounds[] = {30, 49, 79}; // Add more level bounds here.

int get_level(int score)
{
    return std::lower_bound(bounds,
        bounds + sizeof(bounds) / sizeof(*bounds), score) - bounds + 1;
}

Or if you have relatively small number of levels then use if-else chain similar to your original version:

int get_level(int score)
{
    if (score <= 30)
        return 1;
    else if (score <= 49)
        return 2;
    else if (score <= 79)
        return 3;
    return 4;
}

Note that putting returns on separate lines can make your program easier to trace in the debugger.

Upvotes: 5

theReverseFlick
theReverseFlick

Reputation: 6044

  int getLevel(int score)
  {
      if(score<=30)
          return 1;
      int level=2,limit=49;
      while(score > limit)
      {
          limit+=30;
          level++;
      }
      return level;
  }  

  int score[]={35,25,67,56,78};
  int level[5];
  for(int i=0;i<5;i++)
      level[i]=getLevel(score[i]);

Cheers :)

Upvotes: 1

phkahler
phkahler

Reputation: 5767

Create a function where you pass in the score and it returns the level. Also, if there are going to be a lot of them you should create an array of scores and levels.

for (x=0;x < num_scores;x++)
{
   level[x] = get_level(score[x]);
}

something like that.

Upvotes: 5

Arun
Arun

Reputation: 20383

If the score has a "manageable" range, how about the following?

// Convert score to level. Valid scores are in the range [0-79]
int score2level( int score ) {

    static const int level_table[ 80 ] = {
        1, 1, 1, ...     // 31 "1"s for range 0-30
        2, 2, 2, ...     // 19 "2"s for range 31-49
        3, 3, 3, ...     // 30 "3"s for range 50-79
    };

    assert( (0 <= score) && (score <= 79) );

    return level_table[ score ];
 }

The motivation is to avoid conditional code (the if, elses) at the cost of a pre-populated table. May be its too much for 3 levels, but may help when the number of levels increase.

Upvotes: 2

Jay
Jay

Reputation: 14441

The absolute fastest way:

Create an array of 80 values, one for each possible score. Fill in the array with the level for each possible score.

Example: int score_array[80] = {1,1,1,1,...};

The following code gets you the level for each score:

level2 = score_array[score2];

This will compile down to one machine instruction. Doesn't get much faster.

Upvotes: 2

Kenny Cason
Kenny Cason

Reputation: 12328

No, In terms of efficiency it's already optimized.

On another stylistic note. I would recommend putting the conditions and statements on separate lines:

if (score1 <= 30) {
    level1 = 1;
} else if (score1 <= 49) {
    level1 = 2;
} else if (score1 <= 79) {
    level1 = 3;
}

and as the other answer suggest, another stylistic plus would be to extract out the common behavior into a function

Upvotes: 4

Related Questions