Problem
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
Here is the code for review:
using System;
using System.Collections.Generic;
using System.Linq;
namespace VendingMachineV2_StatePattern
{
public interface IVendingMachine
{
List<Product> Products { get; }
int CashInserted { get; }
int ProductChosen { get; set; }
void ChangeState(IState state);
void MakeChoice(int productId);
void InsertMoney(int amount);
(Product, int) DispenseItem();
int RefundMoney();
void ResetMachine();
}
public interface IState
{
(Product, int) DoWork();
}
public class VendingMachine : IVendingMachine
{
private IState _state;
public List<Product> Products { get;}
public int CashInserted { get; private set; }
public int ProductChosen { get; set; }
public VendingMachine()
{
CashInserted = 0;
Products = PopulateProducts();
_state = new AwaitingChoice(this);
}
public void ChangeState(IState state)
{
_state = state;
}
public void MakeChoice(int productId)
{
ProductChosen = productId;
_state.DoWork();
}
public void InsertMoney(int amount)
{
CashInserted += amount;
_state.DoWork();
}
public (Product, int) DispenseItem()
{
return _state.DoWork();
}
public int RefundMoney()
{
_state = new Canceling(this);
var (product, refund) = _state.DoWork();
CashInserted = 0;
return refund;
}
public void ResetMachine()
{
CashInserted = 0;
ProductChosen = 0;
}
private static List<Product> PopulateProducts()
{
return new List<Product>()
{
new Product(1, "Guinness", 3),
new Product(2, "Tayto", 2)
};
}
}
public class AwaitingChoice : IState
{
private readonly IVendingMachine _vendingMachine;
public AwaitingChoice(IVendingMachine vendingMachine)
{
_vendingMachine = vendingMachine;
_vendingMachine.ResetMachine();
Console.WriteLine($"Available products: {string.Join(",", _vendingMachine.Products.Select(x => $"Id: {x.Id}, Name: {x.Name}, Price: {x.Price}").ToArray())}");
}
public (Product, int) DoWork()
{
_vendingMachine.ChangeState(new AwaitingMoney(_vendingMachine));
return (null, 0);
}
}
public class AwaitingMoney : IState
{
private readonly IVendingMachine _vendingMachine;
private readonly Product _productChosen;
public AwaitingMoney(IVendingMachine vendingMachine)
{
_vendingMachine = vendingMachine;
_productChosen = _vendingMachine.Products.FirstOrDefault(x => x.Id == _vendingMachine.ProductChosen);
Console.WriteLine($"You have chosen {_productChosen.Name}, price is {_productChosen.Price}, please insert coins.");
}
public (Product, int) DoWork()
{
if (_vendingMachine.CashInserted < _productChosen.Price)
{
Console.WriteLine($"Price is {_productChosen.Price}, you have paid {_vendingMachine.CashInserted}, please insert more coins.");
}
else
{
_vendingMachine.ChangeState(new Dispensing(_vendingMachine));
}
return (null, 0);
}
}
public class Dispensing : IState
{
private readonly IVendingMachine _vendingMachine;
private readonly Product _productChosen;
public Dispensing (IVendingMachine vendingMachine)
{
_vendingMachine = vendingMachine;
_productChosen = _vendingMachine.Products.FirstOrDefault(x => x.Id == _vendingMachine.ProductChosen);
Console.WriteLine($"You have paid, dispensing {_productChosen.Name}, please wait...");
}
public (Product, int) DoWork()
{
var returningChange = _vendingMachine.CashInserted > _productChosen.Price
? _vendingMachine.CashInserted - _productChosen.Price
: 0;
Console.WriteLine($"Here is your product {_productChosen.Name}, returning change {returningChange}.");
_vendingMachine.ChangeState(new AwaitingChoice(_vendingMachine));
return (_productChosen, returningChange);
}
}
public class Canceling : IState
{
private readonly IVendingMachine _vendingMachine;
public Canceling(IVendingMachine vendingMachine)
{
_vendingMachine = vendingMachine;
Console.WriteLine("Cancelling, please wait...");
}
public (Product, int) DoWork()
{
var refund = _vendingMachine.CashInserted;
Console.WriteLine($"Here is your refund of {refund}...");
_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)
{
IVendingMachine vendingMachine = new VendingMachine();
//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
vendingMachine.RefundMoney();
Console.ReadLine();
}
}
}
Solution
VendingMachine internals should not be public
This fundamentally subverts the idea of a state machine; that the vending machine’s internal state is changed by a proscribed, orderly series of transactions/events. Why bother when I can set all the properties directly.
Delete IVendingMachine
A well designed class exposes functionality and hides state. In other words a class’ interface exposes functionality and hides state. However the formal interface C# construct is forcing required state to be public.
The public members of a class is an interface even if it does not implement a interface definition. We will design with inheritance, polymorphism, etc. such that clients must implent their particular behavior of the desired interface; this does not require a C#-keyword interface definition. With abstract classes/methods, overriding, et.al. we are still “coding to interface not implementation.”
Define an abstract VendingMachine
Now properties can be protected and thus inherited and public methods declared abstract, requiring subclass implementation. Any temporal coupling needs – like calling PopulateProducts – can be controlled using a template method.
VendingMachine constructor should require products list
Client programmer needs to know what is required to instantiate a valid object. This will inhance its usability as a library. Also, the vended products will naturally vary widely.
Creating an inline List(Product) is OK but no time contraint excuses creating that inside VendingMachine. I might have argued “it’s just a first, rough draft” if, and only if, there was a constructor overload as an implicit acknowledgement that I know I am egregiously violating “an obvious case for composition” (or “dependency injection” if you want to impress others) . e.g.:
public VendingMachine(List<Product> inventory){
inventory ?? Throw new ArgumentNullException("constructor parameter is null");
}
console.WriteLine() – Not in business model!
I wonder if the interviewers said anything about it. Seems like it would not be trivial to refactor it out.
Consumers of the library can list products
This begs for a custom Products collection, if only to impliment this one method, given the time constraints. Collections need a place for the same OO goodness we intend to put in any other class. The expedient List<someClass> means validation, searching, comparing, output formatting, etc., etc., etc. is all done differently and redundantly by every client and even multiple methods of a given client. AwaitingChoice() is a case in point.
Intended to speak to the spirit of a custom collection, not say what specific parameter and return types, and implementation, must be.
public class Products {
private List<Product> products {get; set;}
public Products (List<Product> stuff) {
products = stuff ?? new List<Product>();
}
public string Inventory() {
return this.ToString(); // ultra trivial if Product.ToString is overridden
}
public bool Add(product another) {
// +another.Count if existing, else add new product
}
public string Dispense(string selection) {
//Do we have that product and count >= 1 ?
}
}
I wouldn’t expect this necessarily, given your time constraint.