Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

This picture inspired a contest between a few of my friends to rewrite this code in more proper OO style.

coffee mug with code

This is what I have come up with. Any thoughts:

public enum CoffeeKind
{
    Black = 1,
    Cappuccino,
    Espresso
}

public class Coffee
{
    //Measured in celcius
    public int Temprature { get; set; } = 30;
    public bool HasMilk { get; set; }
    public int Sugar { get; set; }
    public CoffeeKind Kind { get; set; }
    public bool Cold => Temprature < 24;
}

public enum CupSize
{
    Small = 1,
    Medium,
    Large,
    ExtraLarge
}

public class Cup
{
    public static Cup SMALL = new Cup(CupSize.Small);
    public static Cup MEDIUM = new Cup(CupSize.Medium);
    public static Cup LARGE = new Cup(CupSize.Large);
    public static Cup EXTRA_LARGE = new Cup(CupSize.ExtraLarge);

    public static Dictionary<CupSize, Cup> Cups = new Dictionary<CupSize, Cup>
    {
        [CupSize.Small] = SMALL,
        [CupSize.Medium] = MEDIUM,
        [CupSize.Large] = LARGE,
        [CupSize.ExtraLarge] = EXTRA_LARGE
    };

    public CupSize Size { get; }
    public int Amount { get; set; }
    public bool Empty => Amount == 0;
    public Coffee Coffee { get; set; }

    private Cup() : this(CupSize.Medium) { }
    private Cup(CupSize size)
    {
        Size = size;
    }

    public void Fill(CoffeeKind kind)
    {
        if (Empty)
        {
            Coffee = new Coffee { Kind = kind };
            Amount = SetAmountBySize(Size);
        }
    }

    private static int SetAmountBySize(CupSize size)
    {
        switch (size)
        {
            case CupSize.Small:
                return 100;
            case CupSize.Large:
                return 300;
            case CupSize.ExtraLarge:
                return 400;
            case CupSize.Medium:
            default:
                return 200;
        }
    }
}

public class Person
{
    public string FirstName { get; set; }
    public string LastName { get; set; }
    public string Name => $"{FirstName} {LastName}";
    public Cup Cup { get; set; }

    public Person() { }
    public Person(string firstName, string lastName)
    {
        FirstName = firstName;
        LastName = lastName;
    }

    public void GetCoffee(CupSize size, CoffeeKind kind)
    {
        Cup = Cup.Cups[size];
        Cup.Fill(kind);
    }

    public void Drink()
    {
        if (Cup?.Empty == false && Cup?.Coffee?.Cold == false)
        {
            Cup.Amount = 0;
        }
    }

    public void Drink(int amount)
    {
        if (Cup?.Empty == false && Cup?.Coffee?.Cold == false)
        {
            Cup.Amount -= amount;
        }
    }
}

class Program
{
    const int EXIT = -1;
    const int GET_COFFEE = 1;
    const int DRINK_COFFEE = 2;
    const int DRINK_AMOUNT = 3;

    //TODO change these to use values of enum - or just change the calls to them
    const int SMALL = 1;
    const int MEDIUM = 2;
    const int LARGE = 3;
    const int EXTRA_LARGE = 4;

    const int BLACK = 1;
    const int CAPPUCCINO = 2;
    const int ESPRESSO = 3;

    static Person person;
    static void Main(string[] args)
    {
        Console.WriteLine("Enter first name:");
        string firstName = Console.ReadLine();
        Console.WriteLine("Enter last name:");
        string lastName = Console.ReadLine();

        person = new Person(firstName, lastName);

        var choice = GetUserAction();
        while (choice != EXIT)
        {
            if (choice == GET_COFFEE)
            {
                if (person?.Cup?.Empty == false)
                {
                    Console.WriteLine("Old coffee will be thrown out");
                }
                int cupSize = GetCupSize();
                int coffeeKind = GetCoffeKind();

                person.GetCoffee((CupSize)cupSize, (CoffeeKind)coffeeKind);
                Console.WriteLine($"Got a {person.Cup.Size.ToString()} size {person.Cup.Coffee.Kind.ToString()} coffee");
            }
            else if (choice == DRINK_COFFEE)
            {
                if (person?.Cup?.Empty == false)
                {
                    person.Drink();
                }
                else
                {
                    Console.WriteLine("Can not drink without coffee!");
                }
            }

            choice = GetUserAction();
        }
    }

    private static int GetCoffeKind()
    {
        Console.WriteLine($"{BLACK}: Black");
        Console.WriteLine($"{CAPPUCCINO}: Cappuccino");
        Console.WriteLine($"{ESPRESSO}: Espresso");

        var result = int.Parse(Console.ReadLine());

        if (result < BLACK || result > ESPRESSO)
        {
            return GetCoffeKind();
        }

        return result;
    }

    private static int GetCupSize()
    {
        Console.WriteLine($"{SMALL}: {nameof(SMALL)}");
        Console.WriteLine($"{MEDIUM}: Medium");
        Console.WriteLine($"{LARGE}: Large");
        Console.WriteLine($"{EXTRA_LARGE}: Extra Large");
        var result = int.Parse(Console.ReadLine());

        if (result < SMALL || result > EXTRA_LARGE)
        {
            return GetCupSize();
        }

        return result;
    }

    private static int GetUserAction()
    {
        Console.WriteLine("What would you like to do:");
        Console.WriteLine($"{GET_COFFEE}: Get coffee");
        // You can't drink coffee without a cup
        if (person?.Cup != null)
        {
            Console.WriteLine($"{DRINK_COFFEE}: Drink coffee");
            Console.WriteLine($"{DRINK_AMOUNT}: Drink some coffee");
        }
        Console.WriteLine($"{EXIT}: Exit");
        return int.Parse(Console.ReadLine());
    }
}
share|improve this question
    
Comments are not for extended discussion; this conversation has been moved to chat. – Mat's Mug Feb 3 at 14:57
3  
In writing this code, please ensure that the implementation is compatible with the correct and agreed related standards for protocol support which are documented here. – Ben Feb 3 at 19:30
    
It's such a letdown that you chose to write this in C# instead of Java. I mean, I usually like C# better, but you should choose the most appropriate tool for the job – Anonymous Penguin Feb 4 at 3:22
up vote 26 down vote accepted

You're abusing the null propagation operator. E.g.

person = new Person(firstName, lastName);

var choice = GetUserAction();
while (choice != EXIT)
{
    if (choice == GET_COFFEE)
    {
        if (person?.Cup?.Empty == false)
        {

There is no way that person can be null. After being created, you never set it to anything else. Funnily enough, I thought it was going to be my most used feature of C#6 but I almost never use it.


C# generally never uses SHOUT_CASE. Your constants should be PascalCase.


If you're going to set an explicit value for one enum member, I'd say you should do it for them all:

public enum CoffeeKind
{
    Black = 1,
    Cappuccino = 2,
    Espresso = 3
}

I'd argue that your model is a bit off here. Espresso is black coffee i.e. it doesn't have milk in it.

public enum DrinkType
{
    FilterCoffee,
    Cappuccino,
    Espresso
}

I think that in general this isn't an enum. What if I want a decaff coffee? I think you need a drink class that can be composed.


I don't see the point in these fields:

public static Cup SMALL = new Cup(CupSize.Small);
public static Cup MEDIUM = new Cup(CupSize.Medium);
public static Cup LARGE = new Cup(CupSize.Large);
public static Cup EXTRA_LARGE = new Cup(CupSize.ExtraLarge);

You already have an enum - make sure you have implemented equality correctly and let people create their own mug cup.


As above:

public static Dictionary<CupSize, Cup> Cups = new Dictionary<CupSize, Cup>
{
    [CupSize.Small] = SMALL,
    [CupSize.Medium] = MEDIUM,
    [CupSize.Large] = LARGE,
    [CupSize.ExtraLarge] = EXTRA_LARGE
};

If you want to know about Cups you need to have a Cupboard:

public class Cupboard : IRepository<Cup>
{
    public Cup GetCupForPerson(Person p)
    {
        // ...
    }
}

You like your coffee cold ;)

public class Coffee
{
    //Measured in celcius
    public int Temprature { get; set; } = 30;

This is an odd method:

public void Drink()
{
    if (Cup?.Empty == false && Cup?.Coffee?.Cold == false)
    {
        Cup.Amount = 0;
    }
}

Why does it matter if the cup is empty?

  1. If you're setting the Amount from 0 to 0 no one will even notice
  2. I regularly get distracted and attempt to drink from an empty cup (sometimes even yesterday's)

You should either TryDrink if the action might not occur. Or, if you think your person (like me) is going to commit to drinking whatever is in that cup then you should always let them do it.


Watch out for typos: GetCoffeKind should be GetCoffeeKind.

share|improve this answer
2  
"You like your coffee cold" lol. Nice answer! – Phrancis Feb 2 at 17:51
1  
@Max - an IRepository might be a bit much but I don't think it's the responsibility of the cup to know what cup sizes are available. I'd say 60C for coffee temp which is backed up by this study which I can't believe is real. – RobH Feb 3 at 8:36
1  
codereview.stackexchange.com/questions/118647/… - I may just drop the temperature. A cup should know what size it is though, correct? So maybe a CupSize struct with name and amount? – Mord Zuber Feb 3 at 8:47
1  
@Max - I'd say a cup should know its capacity yes and you can encode that as a set of predefined sizes a collection of immutable structs sounds like it could be a good choice. – RobH Feb 3 at 9:37
1  
@IsmaelMiguel the way to choose temperature is too wait for v2 ;) mug == cup (based on JavaScript equality). Drinking coffee out of a rubber duck is illegal under the Rome statute – Mord Zuber Feb 3 at 13:18

Some quick remarks

int GetCupSize(), int GetUserAction() and int GetCoffeKind()

Using int.TryParse() would be better so any invalid entered character won't throw an exception.

int SetAmountBySize(CupSize size)

You don't really set an amount here. Try to find a better name.

General

Instead of if (someBoolean == false) you should use the more idiomatic way like
if (!someBoolean).

share|improve this answer
    
Good catch on the SetAmountBySize. Will change the prefix from set to get. The booleanVar == false is because I'm using the Elvis operator so it it's a nullable – Mord Zuber Feb 2 at 15:47
    
Would you say !(bool)person?.Cup?.Empty is better then person?.Cup?.Empty == false? if so why? – Mord Zuber Feb 2 at 15:55
    
No, not really ;-) but nevertheless person wouln't be null (in main) – Heslacher Feb 2 at 15:59
    
Thank you for the feedback, I've fixed all the null checks. – Mord Zuber Feb 2 at 20:19

I'd say that there's a flaw in the logic of the code in the cup :). It should be more like the following:

Cup coffeeCup = new Cup(size);

while(true)
{
    if(cup.IsEmpty)
    {
        cup.Refill(coffeeType);
    }

    cup.Drink();
}

That being said, regarding your code, I'd just remove the following lines:

public static Cup SMALL = new Cup(CupSize.Small);
public static Cup MEDIUM = new Cup(CupSize.Medium);
public static Cup LARGE = new Cup(CupSize.Large);
public static Cup EXTRA_LARGE = new Cup(CupSize.ExtraLarge);

as they don't add anything of much relevance IMO.

share|improve this answer
    
That being said, regarding your code, I'd just remove the following lines: These are just convenience methods to get a quick pre-made cup of that size – Mord Zuber Feb 2 at 20:20
2  
@Max yeah, my point is that it adds nothing of much relevance. I'd prefer to go with the new Cup(size) instruction or with some factory methods like MakeCup(size) or MakeSmallCup() (or MakeMediumCup() and so on). – Gentian Kasa Feb 3 at 7:19
    
Thank you for that feedback. I'll play around some with all the feedback I've gotten and make a follow-up post(question) – Mord Zuber Feb 3 at 7:22

My comment:

OO-wise this is a spilled cup. It's all over the place. You've got everything except a CupOfCoffee. The "dispenser" in the title can only be inferred w/in the spillage.

Max: @radarbob would you like to elaborate please?

static void Main(string[] args){
    Person theDeveloper = new Person();
    Dispenser coffeeMaker = new Dispenser();

    CoffeeOrder whatIWant = GetUserAction();
    CupOfCoffee myCuppa = CoffeeMaker.Brew(whatIWant);
    theDeveloper.Drink(myCuppa);
}

Object Oriented, enumerated

  • Structure

  • Encapsulation

  • Inversion of Control

  • Modeling the domain. Per the OP there is a dispenser and a cup-of-coffee.

  • Single Responsibility Principle - yes, it applies to methods.

  • No properties have been harmed in the making of this code.

    • Alan Kay said, paraphrasing, "If you're setting state directly, not using methods, you're doing it wrong."
  • OO is fractal - Don't quit applying design just because:

    • "It's not much code"
    • "The method is small, we can put more code here"
    • Look! A class. That's OO! That's SOLID! Let the code barf commence.
  • Reality, Interrupted:

    • //TODO change these... we have over 700 of these in our production code base. Code is like quick drying cement when released. it's solid (pun intended).
    • This was just a quickly thrown together
      • Been there, done that. And I've personally seen this manifest as the difference between spending 2 days in formal test and a 3 month test-fail-fix cycle of hell.
share|improve this answer
    
Thank you. In my current code base, the program is the dispenser. But your example has definitely enlightened me. I may simply create a separate project so as to not be side tracked by what I have already written – Mord Zuber Feb 3 at 15:49
    
"OO is fractal - Don't quit applying design just because:". This was just a quickly thrown together first attempt. All the feedback I have received will help me rewrite it better. – Mord Zuber Feb 3 at 15:51
2  
I've learnt more about OO from asking about this silly little toy program than from any of the lectures/books that I have tried before :) – Mord Zuber Feb 3 at 16:14

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.