Finding the lowest and highest values in an array... with "Poor Man's DI" 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 &lt; 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&lt;arr.Length - 1; x++)
        {
            if(arr[highNum] &gt; 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 &lt; arr.Length - 1; x++)
        {
            if (arr[lowNum] &lt; 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.
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&lt;int&gt;.

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)