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 genera开发者_开发百科l 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.

---

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.

---

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.

---

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.

---

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

---

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.

---

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.

---

  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 :)

---

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];
      }
   }
}

---

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.