3
\$\begingroup\$

I've been playing around a bit with a case of State Pattern but including as well ranges in order to get into a specific state.

So the definition is as simple as this:

There is a Steak that has a state based on its temperature, the temperature can be modified, which changes the state.

A Steak has 6 cooking states based on its temperature:

  • Raw (temp <= 100)
  • Rare (temp > 100 && temp <= 110)
  • MediumRare (temp > 110 && temp <= 120)
  • Medium (temp > 120 && temp <= 130)
  • MediumWell (temp > 130 && temp <= 140)
  • WellDone (temp > 140)

I am stuck trying to refactor the rules that dictate if the state must change or not. I'm aiming for a fully object-oriented solution, avoiding if branching conditions (if it's possible)

I've got to a solution, without using if branching conditions, but feels a bit complicated and overengineered...

Consuming code:

static void Main(string[] args)
{
    var steakRules = new SteakRules();

    steakRules.Add(new RawRule());
    steakRules.Add(new RareRule());
    steakRules.Add(new MediumRareRule());
    steakRules.Add(new MediumRule());
    steakRules.Add(new MediumWellRule());
    steakRules.Add(new WellDoneRule());

    var steak = new Steak(steakRules); //Temp 0. This stake is Raw and cannot be eaten.

    steak.AddTemperature(50); //Temp 50. This stake is Raw and cannot be eaten.
    steak.AddTemperature(55); //Temp 105. This stake is Rare and can be eaten.
    steak.AddTemperature(20); //Temp 125. This stake is Medium and can be eaten.
    steak.AddTemperature(40); //Temp 165. This stake is Well Done and can be eaten.
}

ISteakState

public interface ISteakState
{
    bool CanEat();
    ISteakState AddTemperature(int temp);
}

Steak

public class Steak
{
    private ISteakState _state;

    public Steak(SteakRules steakRules)
    {
        _state = new Raw(0, steakRules);
    }

    public void AddTemperature(int temp)
    {
        _state = _state.AddTemperature(temp);
    }

    public override string ToString()
    {
        var canBeEaten = _state.CanEat() ? "can be eaten." : "cannot be eaten.";
        return $"This stake is {_state} and {canBeEaten}";
    }
}

StakeState

public abstract class SteakState : ISteakState
{
    private int _temp;
    private SteakRules _steakRules;

    protected SteakState(int temp, SteakRules steakRules)
    {
        _temp = temp;
        _steakRules = steakRules;
    }

    public ISteakState AddTemperature(int temp)
    {
        return _steakRules.GetState(_temp += temp);
    }

    public abstract bool CanEat();
}

Raw State

public class Raw : SteakState
{
    public Raw(int temp, SteakRules steakRules) 
        : base(temp, steakRules) { }

    public override bool CanEat() => false;

    public override string ToString() => "Raw";
}

Medium State

public class Medium : SteakState
{
    public Medium(int temp, SteakRules steakRules) 
        : base(temp, steakRules) { }

    public override bool CanEat() => true;

    public override string ToString() => "Medium";
}

WellDone State

public class WellDone : SteakState
{
    public WellDone(int temp, SteakRules steakRules) 
        : base(temp, steakRules) { }

    public override bool CanEat() => true;

    public override string ToString() => "Well Done";
}

SteakRules

public class SteakRules
{
    private IList<ISteakRule> _steakRules = new List<ISteakRule>();

    public void Add(ISteakRule rule)
    {
        _steakRules.Add(rule);
    }

    public ISteakState GetState(int temp)
    {
        return _steakRules
            .First(rule => rule.Predicate(temp))
            .GetState(temp)(this);
    }
}

ISteakRule

public interface ISteakRule
{
    bool Predicate(int temp);
    Func<SteakRules, ISteakState> GetState(int temp);
}

RawRule

public class RawRule : ISteakRule
{
    public bool Predicate(int temp) => temp <= 100;

    public Func<SteakRules, ISteakState> GetState(int temp) => 
        (steakRules) => new Raw(temp, steakRules);
}

MediumRule

public class MediumRule : ISteakRule
{
    public bool Predicate(int temp) => temp > 120 && temp <= 130;

    public Func<SteakRules, ISteakState> GetState(int temp) => 
        (steakRules) => new Medium(temp, steakRules);
}

WellDoneRule

public class WellDoneRule : ISteakRule
{
    public bool Predicate(int temp) => temp > 140;

    public Func<SteakRules, ISteakState> GetState(int temp) => 
        (steakRules) => new WellDone(temp, steakRules);
}

This solution has no if branching conditions, classes follow SOLID principles (do they really?) and it is very extensible. However, the design is too complicated and some dependencies there seem to be out of place.

What would your suggestion be?

\$\endgroup\$
  • 2
    \$\begingroup\$ The same or not, on Code Review there is virtually no such thing as unnecessary code. The more the better so, it'd be nice if you added all of it. We cannot review what we cannot see. You'll get a much better feedback if you include everything and when people actually can even run it. \$\endgroup\$ – t3chb0t Apr 20 '19 at 14:09
  • 2
    \$\begingroup\$ Hey, thanks for your feedback. I'll edit my question including the latest design I came up with. I really appreciate your comments. \$\endgroup\$ – Facundo La Rocca Apr 20 '19 at 16:39
3
\$\begingroup\$

I have to dissapoint you because this is not a state-pattern what you have written. This is some strange hybrid that I don't know what to call.

When you take a look at the State Design Pattern then you can find there the following checklist:

  • Define a "context" class to present a single interface to the outside world.
  • Define a State abstract base class.
  • Represent the different "states" of the state machine as derived classes of the State base class.
  • Define state-specific behavior in the appropriate State derived classes.
  • Maintain a pointer to the current "state" in the "context" class. To change the state of the state machine, change the current "state" pointer.

Your code fails the last two of them. The SteakStates instead of returning the next state uses a lookup class. This is wrong because each state should encapsulate the logic that leads to the next state which is usually unique to this state. If it isn't, then it doesn't make sense to use the state-pattern because it's cheaper to solve it in another way. Probably similar to your current solution but with fewer classes.


I have upgraded your code according to the above state-pattern rules. Here's an example. Don't take it to serious, especially the async/await. I did it only for demonstration purposes and intentionally didn't use the Async suffix to make it more readable.


You start with the Raw state and let the while loop cook the stake until it's done. It can also be any other state that you start with.

static async Task Main(string[] args)
{
    var steak = new Steak
    {
        Current = new Raw()
    };

    while (await steak.Current.Next(steak) != null)
    {

    }
}

Each state has a max temperature and a method to the next state:

public interface ISteakState
{
    int MaxTemperature { get; }

    Task<ISteakState> Next(Steak context);
}

The said context is Steak in this case. For cooking purposes it also maintains the previous state.

// Context
public class Steak
{
    private ISteakState _current;
    public ISteakState Previous { get; private set; }
    public ISteakState Current
    {
        get => _current; set
        {
            Previous = _current;
            _current = value;
        }
    }
}

SteakState has a helper Cook method that for the sake of this demonstation prints the time it takes to cook the stake.

public abstract class SteakState : ISteakState
{
    private int _maxTemperature;

    protected SteakState(int maxTemperature)
    {
        _maxTemperature = maxTemperature;
    }

    public int MaxTemperature => _maxTemperature;

    public abstract Task<ISteakState> Next(Steak context);

    protected async Task Cook(int minTemperature)
    {
        var cookDelay = (MaxTemperature - minTemperature);
        Console.WriteLine($"Cooking for {cookDelay / 10} minutes.");
        await Task.Delay(cookDelay * 100);
        Console.WriteLine($"Steak is now {GetType().Name}");
    }
}

Each of the states sets it's max temperature and returns the next state. This is where you configure how a steak should be cooked. It also updates the Current state of the Steak.

public class Raw : SteakState
{
    public Raw() : base(50) { }

    public override async Task<ISteakState> Next(Steak context)
    {
        Console.WriteLine($"Starting cooking...");
        await Cook(context.Previous?.MaxTemperature ?? 0);
        return context.Current = new Rare();
    }
}

public class Rare : SteakState
{
    public Rare() : base(105) { }

    public override async Task<ISteakState> Next(Steak context)
    {
        await Cook(context.Previous?.MaxTemperature ?? 0);
        return context.Current = new Medium();
    }
}

public class Medium : SteakState
{
    public Medium() : base(125) { }

    public override async Task<ISteakState> Next(Steak context)
    {
        await Cook(context.Previous?.MaxTemperature ?? 0);
        return context.Current = new WellDone();
    }
}

public class WellDone : SteakState
{
    public WellDone() : base(165) { }

    public override async Task<ISteakState> Next(Steak context)
    {
        await Cook(context.Previous?.MaxTemperature ?? 0);
        return default;
    }
}

The last state doesn't return any state and the while loop stops there.


This is not 100% clean-code as there are still some repetitions but it should be enough to show the idea.

| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ Hey!! Thanks a lot for the time you took to explain and to expose, I really appreciate it!. Your answer is amazing, that was what I was looking for. Just to clarify, This was example was taken from a real case where we had to move from a finite state machine to states that vary depending upon ranges. Thanks. \$\endgroup\$ – Facundo La Rocca Apr 22 '19 at 12:47
  • \$\begingroup\$ @FacundoLaRocca cool ;-) I find that this example is perfect to show the state-pattern. Much better than in many books or something completely abstract without absolutely any relation to the real world. \$\endgroup\$ – t3chb0t Apr 23 '19 at 8:13
3
\$\begingroup\$

I'm not sure that eliminating if statements is a worthwhile exercise. _steakRules.First(rule => rule.Predicate(temp)) is just a weakly disguised conditional test anyway.

Your state transition rules are too liberal. According to your implementation, if you cool down a well done steak back to room temperature (steak.AddTemperature(-80)), then it reverts to a raw state!

Also, some raw steaks are edible.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Hey, thanks for your answer. I know this is not how real cooking works!!! You cant go from well done to raw in reality! My point was just to achieve the requirements without breaking any SOLID principles and using a fully object-oriented approach. \$\endgroup\$ – Facundo La Rocca Apr 20 '19 at 18:46
3
\$\begingroup\$

You are correct that your current solution is over engineered. This is because you define one class per object of your program. Several of these classes behave exactly the same. Therefore their classes can be merged into one.

A SteakRule consists of:

  • Name
  • Edible
  • Minimum temperature
  • Maximum temperature

If one of the temperatures is optional you can pass int.MinValue or int.MaxValue for them.

A rule does not know about other rules. It is an independent object.

There is no need to define a specialized MediumRule since it fits nicely into the general SteakRule.

Don't confuse "steak" and "stake", by the way.

After merging duplicate code and removing the boilerplate interfaces, the code becomes:

using System.Collections.Generic;
using System.Linq;
using Microsoft.VisualStudio.TestTools.UnitTesting;

namespace Tests
{
    [TestClass]
    public class SteakTest
    {
        [TestMethod]
        public void Test()
        {
            var steakRules = new List<SteakRule>
                {
                    new SteakRule("Raw", false, int.MinValue, 100),
                    new SteakRule("Rare", true, 101, 120),
                    new SteakRule("MediumRare", true, 111, 120),
                    new SteakRule("Medium", true, 121, 130),
                    new SteakRule("MediumWell", true, 131, 140),
                    new SteakRule("WellDone", true, 141, 200),
                    new SteakRule("Burned", false, 201, int.MaxValue)
                };

            var steak = new Steak(steakRules);

            Assert.AreEqual(0, steak.Temperature);
            Assert.AreEqual("This steak is Raw and cannot be eaten.", steak.ToString());

            steak.AddTemperature(50);

            Assert.AreEqual(50, steak.Temperature);
            Assert.AreEqual("This steak is Raw and cannot be eaten.", steak.ToString());

            steak.AddTemperature(55);

            Assert.AreEqual(105, steak.Temperature);
            Assert.AreEqual("This steak is Rare and can be eaten.", steak.ToString());

            steak.AddTemperature(20);

            Assert.AreEqual(125, steak.Temperature);
            Assert.AreEqual("This steak is Medium and can be eaten.", steak.ToString());

            steak.AddTemperature(40);

            Assert.AreEqual(165, steak.Temperature);
            Assert.AreEqual("This steak is WellDone and can be eaten.", steak.ToString());

            steak.AddTemperature(40);

            Assert.AreEqual(205, steak.Temperature);
            Assert.AreEqual("This steak is Burned and cannot be eaten.", steak.ToString());
        }

        public class Steak
        {
            private readonly IReadOnlyList<SteakRule> _rules;

            public Steak(IReadOnlyList<SteakRule> rules)
            {
                _rules = rules;
            }

            public int Temperature { get; private set; }

            public void AddTemperature(int temp)
                => Temperature += temp;

            public string State
                => _rules.First(rule => rule.Applies(Temperature)).Name;

            public bool CanEat()
                => _rules.First(rule => rule.Applies(Temperature)).Edible;

            public override string ToString()
            {
                var canBeEaten = CanEat() ? "can be eaten" : "cannot be eaten";
                return $"This steak is {State} and {canBeEaten}.";
            }
        }

        public class SteakRule
        {
            public string Name { get; }
            public bool Edible { get; }
            public int MinTemperature { get; }
            public int MaxTemperature { get; }

            public SteakRule(string name, bool edible, int minTemperature, int maxTemperature)
            {
                Name = name;
                Edible = edible;
                MinTemperature = minTemperature;
                MaxTemperature = maxTemperature;
            }

            public bool Applies(int temp)
                => MinTemperature <= temp && temp <= MaxTemperature;
        }
    }
}
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Hi, I really appreciate the time you spent to clarify, expose and explain the point. Your answer is clean and smart. I chose the previous one case it kept within state pattern. Thanks a lot! \$\endgroup\$ – Facundo La Rocca Apr 22 '19 at 12:48

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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