Issues

Your current model violates two principles:

• OCP: you cannot easily add new coffee types without touching multiple functions.
• SRP: the PrepareCoffee method not only creates a coffee but also contains logic for multiple recipies.

Let's fix that...

Fix

We start by creating a few abstractions that will allow us to create more concrete objects.

The first thing we need is an Ingredient:

abstract class Ingredient
{
 protected Ingredient(string name, int amount)
 {
 Name = name;
 Amount = Amount;
 }
 public string Name { get; }
 public int Amount { get; }
}

we now use this to derive concrete ingredients from it:

class Milk : Ingredient
{
 public Milk(int amount) : base("Milk", amount) { }
}
class Sugar : Ingredient
{
 public Sugar(int amount) : base("Sugar", amount) { }
}

we also use this to create another abstractions which is a Coffee:

abstract class Coffee : Ingredient
{
 public Coffee(string name, int amount) : base(name, amount) { }
}

and of course some concrete coffee types:

class Espresso : Coffee
{
 public Espresso(int amount) : base("Espresso", amount) { }
}
class Americano : Coffee
{
 public Americano(int amount) : base("Americano", amount) { }
}

But we need more. Ingredients need to be combined in to something even more concrete - a recipe.

class Recipe : IEnumerable<Ingredient>
{
 private readonly IEnumerable<Ingredient> _ingredients;
 public Recipe(int id, string name, params Ingredient[] ingredients)
 {
 Id = id;
 Name = name;
 _ingredients = ingredients;
 }
 public string Name { get; }
 public int Id { get; }
 public IEnumerator<Ingredient> GetEnumerator() => _ingredients.GetEnumerator();
 IEnumerator IEnumerable.GetEnumerator() => GetEnumerator();
}

A recipe can be for various things, we are interested only in drinks, so let's create it:

private class Drink
{
 public Drink(int id, string name, Recipe recipe)
 {
 Id = id;
 Name = name;
 Recipe = recipe;
 }
 public string Name { get; }
 public int Id { get; }
 public Recipe Recipe { get; set; }
}

This will allow us to create a menu:

var drinks = new Drink[]
{
 new Drink(1, "Espresso1", new Recipe(new Espresso(3), new Milk(2), new Sugar(1))),
 new Drink(2, "Black with Sugar", new Recipe(new Americano(3), new Sugar(2))),
}

Lastly we need customers...

class Customer
{
 public Customer(string name, Drink drink)
 {
 Name = name;
 Drink = drink;
 public string Name { get; }
 public Drink Drink { get; }
}

If a customer wants to buy something we can create a new one and assign a drink to him:

var customers = new Customer[]
{
 new Customer("Anthony", drinks.Single(x => x.Id == 1)),
 new Customer("Chris", drinks.Single(x => x.Id == 2)),
}

You have a textual representation of the recipes. You can achieve this by for example creating a new class specialized only in this:

class RecipeRenderer
{
 public string Render(Recipe recipe)
 {
 // create the string...
 }
}

With all these modules you can now add ingredients, drinks etc. without modifying anything. You just create new types and add a new item to the menu.

So how SOLID is this now?

• SRP: checked - each module does only one thing
• OCP: checked - can be extended without modifying any components
• LSP: checked - Derived types must be completely substitutable for their base types. - Coffee, Milk, Espresso all are Ingredient and can be used as such.
• ISP: does not apply
• DIP: checked - High Level Classes Recipe --> Abstraction Layer Ingredient --> Low Level Classes Milk, Sugar

You can of course add meals or flatware and create even more complex menus and services.

• 2
  \$\begingroup\$ Excellent answer. Only things I would change would be to have a factory create the drinks, and change the way drinks are selected by customers from "id" to an enum which shows more obviously what drink they're selecting. \$\endgroup\$

  404

  – 404

  11/03/2016 11:23:28

  Commented Nov 3, 2016 at 11:23
• \$\begingroup\$ Great answer, clarifies a lot of key concepts I was unaware of as a beginner... Exactly what I was looking for. \$\endgroup\$

  Nelson

  – Nelson

  11/03/2016 13:33:35

  Commented Nov 3, 2016 at 13:33
• 1
  \$\begingroup\$ Look! No interfaces of the keyword kind. Yes, dear reader, this adheres to code to interfaces, not implementation \$\endgroup\$

  radarbob

  – radarbob

  11/03/2016 18:05:50

  Commented Nov 3, 2016 at 18:05

I would probably create a separate objects for this project, such CoffeeDrinkers and Coffee and leave the responsibility for creating of coffee on Coffee (or CoffeeMachine) object. You can eliminate the "switch" based on person. The repetition of predefined persons is bad. If you would like to add more persons in future, you will have to do it on two places of your code.

Program.cs

class Program
{
 static void Main(string[] args)
 {
 CoffeeDrinker[] coffeeDrinkers = new[]
 { new CoffeeDrinker("Anthony", new Coffee(25, 2)),
 new CoffeeDrinker("Chris", new Coffee(10, 0)),
 new CoffeeDrinker("Christian", new Coffee(25, 2)) };
 foreach (var person in coffeeDrinkers)
 {
 Console.WriteLine(person.Name + ": " + person.Coffee.GetCoffee());
 }
 Console.ReadLine();
 }
}

CoffeeDrinker.cs

CoffeeDrinker has name and favourite coffee.

public class CoffeeDrinker
{
 public CoffeeDrinker(string name, Coffee coffee)
 {
 Name = name;
 Coffee = coffee;
 }
 public string Name { get; set; }
 public Coffee Coffee { get; set; }
}

Coffee.cs

You can replace replace string variables a, b, c by one StringBuilder if there is only strings concatenation.

public class Coffee
{
 public Coffee(int milkCount, int sugarCount)
 {
 MilkCount = milkCount;
 SugarCount = sugarCount;
 }
 public int MilkCount { get; set; }
 public int SugarCount { get; set; }
 public string GetCoffee()
 {
 StringBuilder sb = new StringBuilder();
 if (this.MilkCount > 0)
 {
 sb.Append("White");
 if (this.MilkCount <= 20)
 {
 sb.Append(" but not too white");
 }
 }
 else
 {
 sb.Append("Black");
 }
 if (this.SugarCount > 0)
 {
 if (this.SugarCount > 1)
 {
 sb.Append(", " + this.SugarCount + " sugars.");
 }
 else
 {
 sb.Append(", one sugar.");
 }
 }
 else
 {
 sb.Append(", no sugar.");
 }
 return sb.ToString();
 }
}

• 1
  \$\begingroup\$ One suggestion, if you make the parameters of the coffee constructor, optional, with default values of 0, you have much more flexibility by allowing a constructor with no parameters. \$\endgroup\$

  user33306

  – user33306

  11/02/2016 22:11:49

  Commented Nov 2, 2016 at 22:11
• \$\begingroup\$ This is a good start but there is more to be optimized. \$\endgroup\$

  t3chb0t

  – t3chb0t

  11/03/2016 08:25:00

  Commented Nov 3, 2016 at 8:25

Tweaking @Tomaa Paul Answer

This is an exercise in compare and contrast.

Think about "encapsulation", "least knowledge principle", "single responsibility principle"

Think about how client code cannot screw things up because it does not have access to all those properties.

Think about implications now that the client - Main - does not have to know intimate details of two different classes.

And how Coffee and CoffeeMaker are more portable - re-usable - because they know for themselves what and how to output.

Think about how you instantly know what MakeCoffee does. Why do you know what CustomMilk does without reading all the code? Why can we guess that is the only thing it does? Think about why OO principles should be applied into the depths of a class.

And finally, how .net framework is smart enough to use the ToString override.

class Program
{
 static void Main(string[] args)
 {
 CoffeeDrinker[] coffeeDrinkers = new[]
 { new CoffeeDrinker("Anthony", new Coffee(25, 2)),
 new CoffeeDrinker("Chris", new Coffee(10, 0)),
 new CoffeeDrinker("Christian", new Coffee(25, 2)) };
 foreach (var person in coffeeDrinkers)
 {
 Console.WriteLine(person);
 }
 Console.ReadLine();
 }
}
public class CoffeeDrinker
{
 public CoffeeDrinker(string name, Coffee coffee) {
 Name = name;
 Coffee = coffee;
 }
 protected string Name { get; set; }
 protected Coffee Favorite { get; set; }
 public override string ToString() {
 return string.Format("{0} likes {1}", Name, Favorite);
 } 
}
public class Coffee
{
 public Coffee(int milkCount, int sugarCount)
 {
 Milk = milkCount;
 Sugar = sugarCount;
 MakeCoffee();
 }
 ///<Summary>
 /// Teaspoons
 ///</Summary>
 protected int Milk { get; set; }
 ///<Summary>
 /// lumps
 ///</Summary>
 protected int Sugar { get; set; }
 protected string Customize = string.Empty;
 protected void MakeCoffee()
 {
 Customize = string.Format("{0} {1}", CustomMilk(), CustomSugar());
 }
 protected string CustomMilk() {
 StringBuilder creamy = new StringBuilder();
 if (this.Milk > 0) {
 creamy.Append("White");
 if (this.Milk <= 20) {
 creamy.Append(" but not too white");
 }
 }
 else {
 creamy.Append("Black");
 }
 return creamy.ToString();
 }
 protected string CustomSugar() {
 StringBuilder sweetness = new StringBuilder();
 if (this.SugarCount > 0) {
 if (this.Sugar > 1) {
 sweetness.Append(", " + this.Sugar + " sugars.");
 }
 else {
 sweetness.Append(", one sugar.");
 }
 }
 else {
 sweetness.Append(", no sugar.");
 }
 return sweetness.ToString();
 }
 public override string ToString(){
 return string.Format("Sugar:{0}, Milk:{1}, {2}", Sugar, Milk, Customize);
 }
}

P.S. I know you're gonna say, "Hay Bob. You shouldn't use StringBuilder now. Well, it made refactoring easier and quicker.

• 2
  \$\begingroup\$ Why shouldn't use use the StringBuilder? It's exactly what should be used when you create strings. \$\endgroup\$

  t3chb0t

  – t3chb0t

  11/03/2016 08:23:18

  Commented Nov 3, 2016 at 8:23
• 1
  \$\begingroup\$ For someone using the Coffee class, the parameters are milkCount and sugarCount which doesn't mean anything to them. The comments of what the amounts actually are are hidden away in the protected properties inside the class. Name the parameters teaspoonsOfMilk and lumpsOfSugar instead. \$\endgroup\$

  404

  – 404

  11/03/2016 11:57:25

  Commented Nov 3, 2016 at 11:57
• \$\begingroup\$ Having refactored each (local) SB has fewer concats,. to do. I was expecting someone to mention the conventional wisdom that simple string concatenation would be more better in this case. \$\endgroup\$

  radarbob

  – radarbob

  11/03/2016 17:24:30

  Commented Nov 3, 2016 at 17:24
• \$\begingroup\$ I commented these non-public properties so that "Milk", "Sugar" will otherwise suffice as names. When I comment class members I use XML comments (period)- and sometimes in-line. I have this delusion that someday our shop will generate code documentation as part of our Continuous Integration process. \$\endgroup\$

  radarbob

  – radarbob

  11/03/2016 17:35:19

  Commented Nov 3, 2016 at 17:35
• \$\begingroup\$ ... but yeah, milkCount should be something like milkAmount \$\endgroup\$

  radarbob

  – radarbob

  11/03/2016 17:47:51

  Commented Nov 3, 2016 at 17:47

• c#
• beginner