8
\$\begingroup\$

I am practicing my object oriented knowledge, so I created a very simple program that determines which is the lowest and highest number inside array.

This is my main method:

class Program
{
    static void Main()
    {
        Console.Write("How many numbers would you like to enter?: ");
        int num = int.Parse(Console.ReadLine());

        int[] arr = new int[num];

        for (int x = 0; x < num; x++ )
        {
            Console.Write("Enter number {0}: ", (x+1));
            arr[x] = int.Parse(Console.ReadLine());
        }

        Console.WriteLine();
        Console.Write("Lowest (1) \nHighest (2) \nEnter choice: ");
        int choice = int.Parse(Console.ReadLine());


        Math math;

        if (choice == 1){
            math = new Math(new HighestNumber(), arr);
            Console.WriteLine("Answer: " + math.returnAnswer());
        }
        else if (choice == 2){
            math = new Math(new LowestNumber(), arr);
            Console.WriteLine("Answer: " + math.returnAnswer());
        }

        Console.ReadKey();
    }
}

The IMathPerformer interface:

interface IMathPerformer
{
    int getAnswer(int[]arr);
}

...and its implementations:

class HighestNumber : IMathPerformer
{
    public int getAnswer(int[]arr)
    {
        int highNum = 0;
        for(int x = 0; x<arr.Length - 1; x++)
        {
            if(arr[highNum] > arr[x+1])
            {
                highNum = (x + 1);
            }
        }

        return arr[highNum];
    }
}

class LowestNumber : IMathPerformer
{
    public int getAnswer(int[]arr)
    {
        int lowNum = 0;
        for (int x = 0; x < arr.Length - 1; x++)
        {
            if (arr[lowNum] < arr[x + 1])
            {
                lowNum = (x + 1);
            }
        }

        return arr[lowNum];
    }

The Math class:

class Math
{
    private int[] arr;
    private readonly IMathPerformer imathPerformer;
    public Math(IMathPerformer imathPerformer, int[]arr)
    {
        this.imathPerformer = imathPerformer;
        this.arr = arr;
    }

    public int returnAnswer()
    {
        return imathPerformer.getAnswer(arr);
    }
}

I just want to know if I am doing right or this is a bad practice. If it is bad, it would be greatly appreciated if you could point out why and how to fix it and add some more advice and suggestions.

\$\endgroup\$
12
\$\begingroup\$

First of all, kudos for exercising OOP and DI! Coding against interfaces is going to make your life easier in so many wonderful ways!

I agree, no problem is too simple to solve with SOLID principles, especially for learning purposes. Shifting from procedural to object-oriented code takes practice.

The first key to proper OOP is abstraction.

These are the abstractions I see in your code:

  • IMathPerformer, an interface that gets some answer given an int[]. Implementations are responsible for computing the Min or Max value, or any other operation that returns an int given an int[].

That's unfortunately all. Unfortunate, because I see these requirements in your code:

  • Collecting the data (the int[] to work with)
  • Determining what to do with the data
  • Outputting the result

And from these requirements, I see these responsibilities:

  • Interacting with the console
    • Validating user input
  • Collecting the data
  • Knowing what to do with the data
    • Creating the menu with each available command
  • Performing the appropriate command
  • Outputting the result
  • Coordinating the sequence of operations

If a class should be responsible for one thing, then there should be at least 5-6 types in this program.

Back to your code: the Math class doesn't seem to really "fit in" anywhere in the above list - or rather, it's redundant: the command to execute is already known when the class gets created, and it acts as a middle man that doesn't really do anything that the IMathPerformer can't do just as well without it; it's forcing dependency injection, but there's nothing that justifies it because the abstraction level in the calling code is too low.

Abstraction levels are crucial. The entry point of your program (the Main method) should have a very high abstraction level, and is usually pretty short. For example:

public static void Main(string[] args)
{
    var app = CreateApp(args);
    app.Run();
}

We create an app object and call its Run method. The CreateApp method might look like this:

private static ArrayOpsApp CreateApp(string[] args)
{
    var validator = new UserInputValidator();
    var console = new ConsoleUserInterface(validator);
    var argsParser = new CommandLineArgsParser();
    var collector = new IntConsoleCollector(console, argsParser, args);

    var commands = new[] {
        new MinValueFinder(),
        new MaxValueFinder(),
        new AvgCalculator(),
        new StdDevCalculator(),
        //...
    };
    var commandSelector = new ConsoleCommandSelector(console, commands);
    var app = new ArrayOpsApp(console, collector, commandSelector);

    return app;
}

This method has a very special role: it's the only place in the application we're allowed to new up one of our concrete types - everywhere else, we are coding against interfaces. It's the application's composition root, where we compose the application's dependency graph.

Just by looking at this code, you can get a grasp of how the entire application is structured, because every type has its dependencies injected through the constructor.

You know that the IntConsoleCollector is the specific implementation for what the rest of the code sees as some abstract IDataCollector, or even a generic IDataCollector<int>.

You know that the user input will be validated somehow.

Because that type takes a ConsoleUserInterface implementation for what might be its IUserInterface dependency, we know that the type needs an object that's responsible for dealing with UI concerns, and because it takes an IArgsParser, we can infer that we can specify the input with command-line arguments and that the collector might just spit them back and skip prompting the user for them. Perhaps it's also used for parsing the user's input if the args array is empty or doesn't contain anything useful - but that's an implementation detail.

We know exactly which commands will be available for the commandSelector, and again we know that the selector will work with the console, because we're giving it console for its IUserInterface dependency; we can infer that the selector will be iterating the available commands to create a menu interface using the console.

The method then injects the collector and the selector dependencies into a new ArrayOpsApp, which will obviously be responsible for consuming these dependencies and coordinating the sequence of operations.


The Main method in your code has both hands into that sequence of operations; it's responsible for collecting the data, dealing with the user's input, determining which concrete types to use, ... it does pretty much everything except actually computing the min/max values.

You started implementing DI, but stopped somewhere along the way. Half-implemented DI unfortunately doesn't get half the benefits of DI: the only testable code is the code that you've extracted into an interface... which makes the IMathPerformer interface pretty much useless, since if you test the implementations you don't really need the interface it's implementing.

There is no way to write a test that documents what happens when the user enters invalid input. Sad, because you would have found out that you get a FormatException when the int.Parse call fails.

Because of the high coupling and low cohesion of the Main method, most of the code can't be unit-tested at all. You'll want to reverse that, and achieve low coupling and high cohesion.

SOLID isn't everything though: it doesn't really touch on the minute implementation details. This is where things like the DRY principle kick in:

if (choice == 1){
    math = new Math(new HighestNumber(), arr);
    Console.WriteLine("Answer: " + math.returnAnswer());
}
else if (choice == 2){
    math = new Math(new LowestNumber(), arr);
    Console.WriteLine("Answer: " + math.returnAnswer());
}

Don't Repeat Yourself. That Console.WriteLine call doesn't belong in either branch of that conditional:

if (choice == 1){
    math = new Math(new HighestNumber(), arr);
}
else if (choice == 2){
    math = new Math(new LowestNumber(), arr);
}
Console.WriteLine("Answer: " + math.returnAnswer());

But then again, implementation details are also impacted by the high-level structure of the code - for example with a higher abstraction level and injected dependencies, the entire ArrayOpsApp might look like this:

public class ArrayOpsApp
{
    private readonly IUserInterface _ui;
    private readonly IDataCollector _collector;
    private reaodnly ICommandSelector _selector;

    public ArrayOpsApp(IUserInterface ui, IDataCollector collector, ICommandSelector selector)
    {
        _ui = ui;
        _collector = collector;
        _selector = selector;
    }

    public void Run()
    {
        var data = _collector.CollectData();
        var command = _selector.SelectCommand();
        var result = command.Execute(data);
        _ui.OutputResult(result);
    }
}

Notice you can write a test that validates that ArrayOpsApp.Run() calls the CollectData member of its IDataCollector dependency, the SelectCommand member of its ICommandSelector dependency, and the OutputResult of its IUserInterface dependency. If we also mock the output of the selector's SelectCommand method we can also verify that the command's Execute method is called, so we can write tests that document what the Run method is supposed to be doing; how it does it doesn't matter.

Writing SOLID OOP code makes your code testable, right. But writing SOLID OOP code is much easier if you start by writing the tests, or rather, by asking yourself how you would want to be able to test the code. Don't just rush into implementing the functionality - of course you can implement the functionality, it's just a very simple exercise after all! It's not about the functionality itself, it's about properly separating concerns and doing things right. Remember:

  • Single Responsibility Principle: a class should have one single reason to change.
  • Open/Closed Principle: the design is extensible, so instead of modifying a class to add functionality, you just add a new one that implements it.
  • Liskov Substitution Principle: all implementations of an interface fulfill the same contract - substituting one implementation for another doesn't break the program. IUserInterface could very well be a WinForms or a WPF UI, makes no difference. Or a console UI. Or no UI at all, just a mock.
  • Interface Segregation Principle: interfaces should be specialized. Remember modifying an interface is a breaking change.
  • Dependency Inversion Principle: depend on abstractions, not concrete types.

(SOLID)

\$\endgroup\$
  • \$\begingroup\$ SRP covers DRY. It is even more specific and also tells us when DRY is going wrong. Nevertheless: DRY is a good first shot. But in the sense of SRP DRY is only an indicator for code smell not a proof for bad code. The point is: You may have semantic redundancies without code duplication that is a problem. And you have different semantics with code duplication that is fine. On both DRY will not help you. The first you are not able to discover with DRY and the second will lead to errors if you follow DRY. \$\endgroup\$ – oopexpert Feb 3 '17 at 23:21
6
\$\begingroup\$

Additional to @Mat's Mugs great and detailed answer addressing DI, I would just adding few words about alternative solutions.

Because you would like to practice DI, your implementation is absolute OK! However DI / abstractions introduce additional complexity, therefore (IMHO) it should be avoided for logic that doesn't need abstractions.

That is usually the case for pure functions. Your chosen problem (finding min/max value of a list) is such a pure function and therefore I wouldn't use abstraction for that logic in real-world applications.

Alternatively, I would create an extension method or an utility method for such logic.

Fortunately, the extension methods already exists:

var max = numbers.Max();
var min = numbers.Min();

The utility method could look like:

public static void GetMinMax(int[]arr, out int min, out int max)
{
    min = int.MaxValue;
    max = int.MinValue;
    for(int x = 0; x < arr.Length; x++)
    {
        if(min >= arr[x]) min = arr[x];
        if(max <= arr[x]) max = arr[x];
    }
}

or (supposably) in C# 7

public static (int min, int max) GetMinMax(int[]arr)
{
    min = int.MaxValue;
    max = int.MinValue;
    for(int x = 0; x < arr.Length; x++)
    {
        if(min >= arr[x]) min = arr[x];
        if(max <= arr[x]) max = arr[x];
    }
    return (min, max);
}

Some words about your code:

  • The class Math is absolute useless. It gets an abstraction with an argument and calls the method of the abstraction. That is a good example for "over engineering".
  • Methods should start with a capital letter in C# (GetAnswer)

As @Mat's Mug already mentioned, you could use your abstraction to remove duplications:

var options = new Dictionary<int, IMathPerformer>();
options.Add(1, new HighestNumber());
options.Add(2, new LowestNumber());

IMathPerformer performaner;
if (options.TryGetValue(choice, out performaner))
    Console.WriteLine("Answer: " + performaner.GetAnswer());

In your case, that is also possible without abstraction:

var options = new Dictionary<int, Func<int[], int>>();
options.Add(1, Enumerable.Max);
options.Add(2, Enumerable.Min);

Func<int[], int> func;
if (options.TryGetValue(choice, out func))
    Console.WriteLine("Answer: " + func(arr));
\$\endgroup\$
  • \$\begingroup\$ I believe on what you all guys are saying that my Math class is useless, but I wonder what alternative way is possible to get the Min and Max value without that class? \$\endgroup\$ – Jhe Feb 3 '17 at 7:18
  • 2
    \$\begingroup\$ See my updated answer where the Math class is dropped. The Math class is a second abstraction that is useless, because the logic is already abstracted by the interface IMathPerformer. You can just use the instance of the interface. \$\endgroup\$ – JanDotNet Feb 3 '17 at 7:21
  • \$\begingroup\$ You might as well return a Tuple instead of using out args. (It'll be much cleaner to use a Tuple with C#7.) \$\endgroup\$ – RubberDuck Feb 5 '17 at 12:32
1
\$\begingroup\$

I would include null in GetAnswer

Minor performance

public int? GetAnswer(int[] arr)
{
    if(arr.Count == 0)
        return null;

    int lowNum = arr[0];
    for (int x = 1; x < arr.Length; x++)
    {
        if (lowNum < arr[x])
        {
            lowNum = arr[x];
        }
    }
    return lowNum;
}
\$\endgroup\$
  • 2
    \$\begingroup\$ I suggest to ignore null handling. So I would omit "arr == null" and rely on your method signature with typed parameters not to be misused. You expect an int-array so you should get one. \$\endgroup\$ – oopexpert Feb 3 '17 at 22:59
0
\$\begingroup\$

I want to point out the thing that other answers do not. As you tell that you want to practice object-oriented knowledge, you had better understand that OOP is about abstraction and encapsulation rather than a way to manage dependencies. Main violation in your code from the OO point of view is data. In OO, there is no data, only objects. Your code should looks like this:

var objWithBusinessLogicMeaning = //anything to create such an object
var min = objWithBusinessLogicMeaning.Min;
var max = objWithBusinessLogicMeaning.Max;
// do something usefull with min and max
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.