HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMajor

OO coffee dispenser

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
dispensercoffeestackoverflow

Problem

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

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 Cups = new Dictionary
{
[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

Solution

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 Cups = new Dictionary
{
    [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
{
    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?

  • If you're setting the Amount from 0 to 0 no one will even notice



  • 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.

Code Snippets

person = new Person(firstName, lastName);

var choice = GetUserAction();
while (choice != EXIT)
{
    if (choice == GET_COFFEE)
    {
        if (person?.Cup?.Empty == false)
        {
public enum CoffeeKind
{
    Black = 1,
    Cappuccino = 2,
    Espresso = 3
}
public enum DrinkType
{
    FilterCoffee,
    Cappuccino,
    Espresso
}
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
};

Context

StackExchange Code Review Q#118647, answer score: 26

Revisions (0)

No revisions yet.