Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

What I'm trying to do is very straightforward and easy, but I am wondering whether there is a more elegant solution.

Basically I have a game and I want the difficulty to increase the moment the score gets between 100 - 500, then increase once again when the score is between 500 - 1500, then again when the score is between 1500 - 3000. In total, the difficulty should increase 3 times.

I need to check:

  1. Whether I am in one of those ranges.
  2. Whether I already increased the score when I got in that range.

Here's what I've got.

bool Increased100500 = false;
bool Increased5001500 = false;
bool Increased15003000 = false;

void IncreaseScore()
{
    if (Score >= 100 && Score <= 500 && !Increased100500)
    {
        Increase();
        Increased100500 = true;
    } 
    else if (Score >= 500 && Score < 1500 && !Increased5001500)
    {
        Increase();
        Increased5001500 = true;
    } 
    else if (Score >= 1500 && !Increased15003000)
    {
        Increase();
        Increased15003000 = true;
    } 
}

LE: I should have mentioned that the method above is called multiple times per frame.

share|improve this question
5  
Since it ill be called multiple time/ frames. I think it's best to create events. You can create an level Up event and call it inside the Increase score event. –  jean Sep 3 at 21:07
    
@jean please avoid answering questions in comments. –  ANeves Sep 4 at 10:03

8 Answers 8

up vote 16 down vote accepted

Be careful with your inequalities. You've listed 500 twice: once as if (Score <= 500) and again as else if (Score >= 500). That's confusing, as the first condition takes precedence.

I think that you would be better off with one state variable representing the current difficulty level. The code would also be simpler if you determine NewDifficulty using a level-triggered rather than edge-triggered mechanism.

int Difficulty;

void IncreaseScore()
{
    int NewDifficulty = (Score <   100) ? 0
                      : (Score <=  500) ? 1
                      : (Score <= 1500) ? 2
                                        : 3;
    // One-way ratchet: difficulty can only increase, even if the
    // score subsequently drops.
    if (NewDifficulty < Difficulty)
    {
        Difficulty = NewDifficulty;
    }
}

If the score can never decrease in this game, then you can simplify it further:

void IncreaseScore()
{
    Difficulty = (Score <   100) ? 0
               : (Score <=  500) ? 1
               : (Score <= 1500) ? 2
                                 : 3;
}
share|improve this answer
3  
If there are a fixed set of valid Difficultly values, using an enum would be better than int. This will ensure the difficulty doesn't mistakenly get set to -100. Also, this function may change the the difficulty, but it never increases the score. UpdateDifficultly() would be a more descriptive name. –  unholysampler Sep 3 at 21:31
    
Hm. That's much cleaner, but the last snippet doesn't work in my case. I still need to keep a 'NewDifficulty' or 'OldDifficulty' variable somewhere and increase the score only when NewDifficulty != OldDifficulty (i.e. only when my difficulty changes and only then). By the way, <= should have been < in the first if, correct. My typo. –  user16547 Sep 3 at 21:40
1  
@unholysampler: An enum does not actually prevent any value from being entered. It makes it less likely, because there is a name, though. –  Magus Sep 3 at 21:41
    
@Magus I'm too used to Java enums. You are right, in C#, it just makes it harder to do it wrong. –  unholysampler Sep 3 at 21:53
1  
@Almo The ternary operator in PHP has a broken design. In all other sane languages, including C#, it chains reasonably well. –  200_success Sep 4 at 18:12

Just a quick note about naming:

bool Increased100500 = false;
bool Increased5001500 = false;
bool Increased15003000 = false;

Please, don't do that. I mean, yeah, private fields should be camelCase in the first place, but that chunk of numbers is fairly annoying, and it encodes your game logic into an identifier.

If you wanted to change your levels, let alone making them configurable, then what's 100500 going to tell the maintainer?

Consider using names like this instead:

var increasedLevel1 = false;
var increasedLevel2 = false;
var increasedLevel3 = false;

That's still encoding 3 difficulty levels into the identifiers, but at least the thresholds aren't part of the variables' names, so it's a start.

share|improve this answer
6  
"If you wanted to change your levels, let alone making them configurable, then what's 100500 going to tell the maintainer?" Great point. +1 –  user16547 Sep 3 at 21:34
    
Or you could just keep a single variable denoting the last score at which you increased the difficulty. –  CompuChip Sep 4 at 12:36

You don't talk about if the score can decrease or if it is possible for the score to jump over one of the difficulty ranges. If neither of these things are possible, the logic to set the difficulty can become very simple by using an enum.

enum Difficultly { Normal, Hard, Harder, Hardest }

class State
{
  private Difficultly _currentDifficultly = Difficultly.Normal;
  private int _score = 0;

  public void UpdateDifficulty()
  {
    switch(_currentDifficulty)
    {
      case Normal:
        if (_score  >= 100)
        {
          _currentDifficulty = Difficulty.Hard;
        }
        break;
      case Hard:
        if (_score  >= 500)
        {
          _currentDifficulty = Difficulty.Harder;
        }
        break;
      //...
    }
  }
}

However, as you can see, this gets very repetitive very fast. An alternative would be to construct a table that defines the data points.

Dictionary<Difficulty, Tuple<int, Difficulty>> _difficultyTransitions = new Dictionary<Difficulty, Tuple<int, Difficulty>>()
{
  { Difficulty.Normal, new Tuple<int, Difficulty>(100, Difficulty.Hard)},
  { Difficulty.Hard, new Tuple<int, Difficulty>(500, Difficulty.Harder)},
  { Difficulty.Harder, new Tuple<int, Difficulty>(1500, Difficulty.Hardest)},
  { Difficulty.Hardest, new Tuple<int, Difficulty>(int.MaxValue, Difficulty.Hardest)},
};

public void UpdateDifficulty()
{
  var transition = difficultyTransitions[_currentDifficulty];
  if (_score >= transition.Item1)
  {
    _currentDifficulty = transition.Item2;
  }
}
share|improve this answer

If Score cannot jump suddenly in the levels, for example after Score=50 it cannot jump to Score=550, then you could simplify by using half-ranges in the if statements, so instead of Score >= X && Score <= X + C you could use just Score >= X, like this:

void IncreaseScore()
{
    if (Score >= 100 && !Increased100500)
    {
        Increase();
        Increased100500 = true;
    } 
    else if (Score >= 500 && !Increased5001500)
    {
        Increase();
        Increased5001500 = true;
    } 
    else if (Score >= 1500 && !Increased15003000)
    {
        Increase();
        Increased15003000 = true;
    } 
}

But this is just treating the symptoms, not the real problem. It's not good that you need 3 state variables Increased100500, Increased5001500, and Increased15003000, and the hardcoded constants 100, 500, 1500 to keep track of this.

It would be better to refactor somehow without using those boolean variables at all. For example:

private const int[] thresholds = {100, 500, 1500};
private const HashSet<int> passedThresholds = new HashSet<int>();

void IncreaseScore()
{
    for (int threshold : thresholds)
    {
        if (Score >= threshold && !passedThresholds.Contains(threshold))
        {
            passedThresholds.add(threshold);
            Increase();
            break;
        }
    }
}

The behavior of this code is equivalent, and you don't need so many boolean variables to track the state. The increaseScore method doesn't need to know the exact values of the thresholds either, which is a good thing. And you can easily change the thresholds, simply in the declaration of thresholds, and without editing variable names.

share|improve this answer
    
I think I misread your answer. The purpose of those flags is to only increase the score once when in the specified ranges. If you don't have any Increased100500 (etc) flag, it will keep calling Increase() whenever you get in that particular branch. –  user16547 Sep 3 at 20:49
    
I want the score increased once when it's in [100, 500], then one more time in [500, 1500], then one last time in [1500, 3000]. IncreaseScore() is called multiple times per frame, by the way. I should have mentioned that. –  user16547 Sep 3 at 20:50
2  
@user16547 I think maybe you should add some of the surrounding code for extra context. –  RubberDuck Sep 3 at 20:53
    
@user16547 I updated my answer, though I'm still not 100% sure that's what you really wanted –  janos Sep 3 at 20:59
    
To avoid recalling the increase dificult method just because you are still in a range you must call it inside your increase score method and know before and after score. You are responding to an event, not a state –  jean Sep 3 at 21:10

A riff on @200_success answer

// value is the base-score for that level
public enum Difficulty {
    Beginner = 0,
    Easy = 100,
    Medium = 500,
    Hard = 1500
}

void IncreaseScore() // yeah, needs a different name
{
    Difficulty NewDifficulty = (Score <   Difficulty.Easy) ? Difficulty.Beginner
                      : (Score <=  Difficulty.Medium) ? Difficulty.Easy
                      : (Score <= Difficulty.Hard) ? Difficulty.Medium
                                        : Difficulty.Hard;
    // One-way ratchet: difficulty can only increase, even if the
    // score subsequently drops.
    if (NewDifficulty < CurrentDifficulty)
    {
        CurrentDifficulty = NewDifficulty;
    }
}
  • Defines levels for use everywhere.
  • Encapsulates the actual values associated w/ the levels
  • Useful throughout the application where ever the score-level needs to be used
  • Explicitly defines an initial state
  • Eliminates all magic numbers as seen in several other answers
  • No need for intermediate ints - which were essentially redefining the difficulty values.
share|improve this answer

I am not fond of the hardcoding since that will make it difficult to change your score thresholds later.This rules out Enum's as well. I liked Janos's second answer best, but that means storing a copy of the thresholds in a hash. In addition it will continue to loop through every threshold after the max difficulty is reached just to do nothing.

The following uses one list, handles level jumping, and once max difficulty is reached it short circuits as quickly as possible.

Initial variable declarations are only to illustrate type/expectation. Score, Difficulty, and the threshold list should be wrapped up nicely somewhere else in a static Singleton managing game state. And the thresholds need to come out of a configuration store somewhere so you don't need to recompile your game to change your mind later.

The key to this approach is that the difficulty is the index into the threshold list to determine the next increase. I did assume that you wouldn't be going backwards like the other solutions. Once difficulty level is out of bounds you know you have nothing left to check and are at max level.

int currentScore = 0; 
int currentDifficulty = 0;
List<int> difficultyThresholds = new List<int>() { 100, 500, 1500 };

void DifficultySlider()
{
   if (currentDifficulty >= difficultyThresholds.Count)
   {
      return;
   }

   while (currentScore >= difficultyThresholds[currentDifficulty])
   {
      currentDifficulty += 1;
      if (currentDifficulty >= difficultyThresholds.Count)
      {
         return;
      }
   }
}
share|improve this answer

Checking in reverse order will catch any possible diffuculty jumps (ie from easy straight to insane) as well as any possible negative changes in score.

enum Difficulty { Easy = 0, Medium, Hard, Insane, Deathwish }
Difficulty currentDifficulty;

void UpdateDifficulty(int _score)
{
    if(_score >= 3000)
    {
        currentDifficulty = Difficulty.Deathwish;
    }
    else if(_score >= 1500)
    {
        currentDifficulty = Difficulty.Insane;
    }
    else if(_score >= 1000)
    {
        currentDifficulty = Difficulty.Hard;
    }
    else if(_score >= 500)
    {
        currentDifficulty = Difficulty.Medium;
    }
    else
    {
        currentDifficulty = Difficulty.Easy;
    }
}
share|improve this answer

I don't use C# but took this as a chance to learn some of it... Tested it first on Progress 4GL and then made the C# version (quite ugly one).

My idea was to use modulo to find out if it's needed to increase the difficulty:

score % thresholds[x] < score

That way there's no need to hardcode the IF-clauses (only need to hardcode the thresholds).

Ideone link: http://ideone.com/JM9Nj1

using System;

public class Test
{
    public static void Main()
    {
        // your code goes here
        int[] thresholds = new int[4] {100, 500, 1000, 1500};
        int difficulty = 0;
        int score = 0;

        for (score = 0; score <= 3000; score++) {
            for (int x = 0; x < thresholds.Length; x++)
            {
                if (score % thresholds[x] < score && difficulty < x + 1) { 
                    difficulty = x + 1;
                    Console.WriteLine("Score: " + score + " Threshold: " + thresholds[x] + " Difficulty: " + difficulty + " Increased difficulty!");
                    break; 
                }
            }
        }
    }
}
share|improve this answer

We're looking for long answers that provide some explanation and context. Don't just give a one-line answer; explain why your answer is right, ideally with citations. Answers that don't include explanations may be removed.

2  
Welcome to Code Review. You seem to be proposing an independent solution, but for your answer to constitute a code review, you'll need to explain how your version improves on the original code. –  200_success Sep 4 at 9:26
    
I don't think using modulo is helping here... I believe it can screw up the results. –  Simon André Forsberg Sep 4 at 10:08

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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