5
\$\begingroup\$

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();
        }
    }
}
\$\endgroup\$

1 Answer 1

4
\$\begingroup\$

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.

\$\endgroup\$
10
  • \$\begingroup\$ 1) Will deleting IVendingMachine, restrict the a consumer from DI into consuming class? 2) I will remove all public properties & get the values of private fields via public methods, so: Why declare VendingMachine or any of its methods abstract? If i declare the method MakeChoice abstract then I can not have an implementation of it in VendingMachine, and I can not think of any abstract methods to create or a reason to create a concrete subclass. 3) Maybe you are suggesting abstract class and a concrete imp to show the interviewer i am thinking about extensibility? 4)Remove writlines to main? \$\endgroup\$ Commented Mar 2, 2022 at 14:52
  • \$\begingroup\$ Thanks for the feedback, I have upvoted, I will prepare a revised version based on your observations. But first could you answer my 4 questions in my other comment to help me understand? Appreciate it. \$\endgroup\$ Commented Mar 2, 2022 at 14:54
  • \$\begingroup\$ 1) Will deleting IVendingMachine, restrict the a consumer from DI into consuming class? DI - you mean dependency injecttion, yes? To be clear passing parameters is DI. Perhaps 90%+ of DI you'll ever write will be using method parameters. Allowing random setting of variables directly may be DI technically but is contrary to good programming in general. IVendingMachine has no DI provisions at all. The one dependency, List<Product>, has no where to inject it. Making internal state properties public does not count! Make a constructor with an appropriate parameter. \$\endgroup\$
    – radarbob
    Commented Mar 3, 2022 at 5:11
  • \$\begingroup\$ 2) ... I assumed subclassing. Everyone seems to be declaring a (C#-keyword) interface when sublcassing even though it is wrong. A (C#-keyword) interface is to give classes some behavior not inherent in their own types or super types. If you want a Cat to behave like a Dog then have class Cat : IDog. Now you have a Cat that could be on Britian's Got Talent. The VendingMachine class is self defining. The class' public members is its interface. It is an interface. If other not-vending-machine types do not need VendingMachine behavior then IVendingMachine is pointless. \$\endgroup\$
    – radarbob
    Commented Mar 3, 2022 at 5:14
  • \$\begingroup\$ 3) Maybe you are suggesting abstract class and a concrete imp to show the interviewer i am thinking about extensibility? An abstract class is particularly intended for extension by inheritance. If the interviewers said "keep extension in mind", well, that's too vague. Good OO design generally enables potential extension with focused classes, using inheritance and composition appropriately, interfaces that fully expresss object behavior - and does not make everything public hoping the consumer-programmer can read your mind. \$\endgroup\$
    – radarbob
    Commented Mar 3, 2022 at 5:14

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.