1
\$\begingroup\$

I previously asked for a review for a Vending Machine implementing State Pattern in this linked post.

And received an answer from @radarbob. The code below is my revised attempt based on some of @radarbob's answer.

Notes / Changes from my original attempt:

  • VendingMachine internals no longer public
  • protected properties instead
  • Abstract VendingMachine base, with Concrete VendingMachine with virtual methods allowing for extensibility
  • Products/Inventory by composition
  • No Writelines in Domain

Original Question:

I was asked to implement a Vending Machine in a recent interview coding challenge. My attempt at a solution is based on the state pattern.

I would like to get some code reviews on this bearing in mind I would have about 40 minutes to implement this in the interview scenario. When reviewing I would like you to consider how I have done in relation to SOLID principles.

And also consider that the requirements from the interview are:

  • Create a vending machine library
  • Consumers of the library can list products
  • Ability to select product
  • After product selected, ability to insert coins
  • When enough coins are submitted, product and change dispensed
  • Ability to cancel
  • Dont worry about stock levels
  • Only one consumer at a time
  • Think about extensibility
public abstract class VendingMachineBase
{
    protected IState State;
    protected List<Product> Products;
    protected int MoneyInserted;
    protected int ProductChosen;

    protected VendingMachineBase(List<Product> products)
    {
        _ = products ?? throw new ArgumentNullException(nameof(products), "You must provide a product inventory");
        Products = products;
    }

    public void ChangeState(IState state)
    {
        State = state;
    }

    public List<Product> ListProducts()
    {
        return Products;
    }
}

public interface IState
{
    (Product, int) DoWork();
}

public class VendingMachine : VendingMachineBase
{
    public VendingMachine(List<Product> products) :base(products)
    {
        MoneyInserted = 0;
        State = new AwaitingChoice(this);
    }

    public virtual void MakeChoice(int productId)
    {
        ProductChosen = productId;
        State.DoWork();
    }

    public virtual int GetChosenProductId()
    {
        return ProductChosen;
    }

    public virtual void InsertMoney(int amount)
    {
        MoneyInserted += amount;
        State.DoWork();
    }

    public virtual int GetMoneyInsertedAmount()
    {
        return MoneyInserted;
    }

    public virtual (Product, int) DispenseItem()
    {
        return State.DoWork();
    }

    public virtual int RefundMoney()
    {
        State = new Canceling(this);
        var (_, refund) = State.DoWork();

        MoneyInserted = 0;

        return refund;
    }

    public virtual void ResetMachine()
    {
        MoneyInserted = 0;
        ProductChosen = 0;
    }
}

public class AwaitingChoice : IState
{
    private readonly VendingMachine _vendingMachine;
    public AwaitingChoice(VendingMachine vendingMachine)
    {
        _vendingMachine = vendingMachine;
        _vendingMachine.ResetMachine();
    }

    public (Product, int) DoWork()
    {
        _vendingMachine.ChangeState(new AwaitingMoney(_vendingMachine));
        return (null, 0);
    }
}

public class AwaitingMoney : IState
{
    private readonly VendingMachine _vendingMachine;
    private readonly Product _productChosen;
    public AwaitingMoney(VendingMachine vendingMachine)
    {
        _vendingMachine = vendingMachine;
        _productChosen = _vendingMachine.ListProducts().FirstOrDefault(x => x.Id == _vendingMachine.GetChosenProductId());
    }

    public (Product, int) DoWork()
    {
        if (_vendingMachine.GetMoneyInsertedAmount() >= _productChosen.Price)
        {
            _vendingMachine.ChangeState(new Dispensing(_vendingMachine));
        }

        return (null, 0);
    }
}

public class Dispensing : IState
{
    private readonly VendingMachine _vendingMachine;
    private readonly Product _productChosen;

    public Dispensing (VendingMachine vendingMachine)
    {
        _vendingMachine = vendingMachine;
        _productChosen = _vendingMachine.ListProducts().FirstOrDefault(x => x.Id == _vendingMachine.GetChosenProductId());
    }

    public (Product, int) DoWork()
    {
        var returningChange = _vendingMachine.GetMoneyInsertedAmount() > _productChosen.Price
            ? _vendingMachine.GetMoneyInsertedAmount() - _productChosen.Price
            : 0;

        _vendingMachine.ChangeState(new AwaitingChoice(_vendingMachine));

        return (_productChosen, returningChange);
    }
}

public class Canceling : IState
{
    private readonly VendingMachine _vendingMachine;

    public Canceling(VendingMachine vendingMachine)
    {
        _vendingMachine = vendingMachine;
    }

    public (Product, int) DoWork()
    {
        var refund = _vendingMachine.GetMoneyInsertedAmount();

        _vendingMachine.ChangeState(new AwaitingChoice(_vendingMachine));

        return (null, refund);
    }
}

public class Product
{
    public int Id { get; }
    public string Name { get; }

    public int Price { get; }

    public Product(int id, string name, int price)
    {
        Id = id;
        Name = name;
        Price = price;
    }
}

class Program
{
    static void Main(string[] args)
    {
        //worker installs vending machine and populates products
        var products =  new List<Product>()
        {
            new Product(1, "Guinness", 3),
            new Product(2, "Tayto", 2)
        };

        var vendingMachine = new VendingMachine(products);

        //Customer uses Vending Machine

        //Choose Product
        vendingMachine.MakeChoice(1);

        //Insert money
        vendingMachine.InsertMoney(1);

        //Not enough, insert more
        vendingMachine.InsertMoney(3);

        var (product, change) = vendingMachine.DispenseItem();
        Console.WriteLine($"Got my {product.Name}, change is {change}.");

        //Choose Product
        vendingMachine.MakeChoice(2);

        //Insert money
        vendingMachine.InsertMoney(1);

        //Ah, no coins left, I will cancel
        var refund = vendingMachine.RefundMoney();
        Console.WriteLine($"Got my refund {refund}.");

        Console.ReadLine();
    }
}
\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

I think it's more usual when using the state pattern for the state to have a set of well-defined methods on it rather than one general method. You can see that your return value for DoWork is barely used.

I'll try to show that using just the first transition: selecting a product. I haven't compiled the code, I've written it straight into CR so apologies for any errors on my part.

internal abstract class VendingMachineState
{
     protected readonly VendingMachine Machine;

     public VendingMachineState(VendingMachine machine)
     {
         Machine = machine;
     }

     public abstract void ChooseProduct(int productId);
}

internal class AwaitingChoiceState : VendingMachineState
{
    public AwaitingChoiceState(VendingMachine vendingMaching): base(vendingMachine) { }

    public override void ChooseProduct(int productId)
    {
        Machine.SetState(new ProductChosenState(Machine, productId));
    }
}

internal class ProductChosenState : VendingMachineState
{
    public int ProductId { get; }        

    public ProductChosenState(VendingMachine vendingMaching, int productId): base(vendingMachine)
    {
        ProductId = productId;
    }

    public override void ChooseProduct(int productId)
    {
        // You might want this to be a no-op, or change the void return type to something more meaningful. 
        throw new InvalidOperationException("Product already selected");
    }
}

Your VendingMachine then just forwards calls to the state:

public void ChooseProduct(int productId)
{
    _state.ChooseProduct(productId);
}

This achieves the decoupling you want in the vending machine. The next step would be allowing the ProductChosen state to accept money and ensuring that the AwaitingChoiceState doesn't allow it.


It might also be worth reconsidering what you consider the states of the machine. I think these exist:

  1. Initial - transitions to 2. when product is selected
  2. ProductChosen - transitions to 3. when enough money has been added
  3. ReadyToDispense - transitions to 1. when product is dispensed

Even the need for ReadyToDispense is debatable. I think it could work with just Initial and ProductChosen but the third state keeps each state's responsibility smaller. ReadyToDispense might also reject further input of money for example.

There's no need for a cancelling state for example. It's just a transition back to initial with a side-effect that money should be returned if it has already been added which is the individual state's responsibility.

\$\endgroup\$
6
  • \$\begingroup\$ Thank you. I did struggle with the idea that DoWork return value is under used. But I wondered about the alternative as you suggest. Abstract base VendingMachineState will define ChooseProduct, and ProductChosenState, DispensingState, CancellingState will all have to implement an irrelevant method ChooseProduct and throw an exception within it. All states will implement many unwanted methods in the end. So I made the choice to go with DoWork. Do you have any thoughts on my argument against all of these unused methods vs underused return value? I'd love to hear and learn what is best. Thanks \$\endgroup\$ Commented Mar 4, 2022 at 9:45
  • 1
    \$\begingroup\$ As with everything, it's a trade-off. The state pattern's big advantage is that everything is really explicit. To answer "what happens when I try to do x in state y" you just look at the y state class's x action method. It also affords the opportunity for simple validation - as I have demonstrated in ChooseProduct when a product is already chosen. The downside is verbosity. However, to me, clear and verbose code is not a problem ordinarily. Secondly, your approach is not extensible. What happens if another method needs a completely different return value? You can't cater for it. \$\endgroup\$
    – RobH
    Commented Mar 4, 2022 at 9:52
  • \$\begingroup\$ Just in case it's helpful: I've probably only seen 2 good uses of the state pattern in 10+ years of programming. When done right, it's really powerful but it is rather rigid. If you think the states/transitions are likely to change in the future it can be deeply frustrating to change. \$\endgroup\$
    – RobH
    Commented Mar 4, 2022 at 10:02
  • \$\begingroup\$ Thank you, some very good points to consider regarding verbosity and extensibility. Regarding your mention of use cases, there is a question that comes to mind- in your opinion is the Vending Machine even a good use case for the state pattern? \$\endgroup\$ Commented Mar 4, 2022 at 12:10
  • 1
    \$\begingroup\$ in your opinion is the Vending Machine even a good use case for the state pattern? Head First Design Patterns uses it. \$\endgroup\$
    – radarbob
    Commented Mar 5, 2022 at 3:52

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.