3-Tier Object-Oriented Design Flaw?

Question

I am going through training at my new job to use good object-oriented design with a 3-tier programming style. My supervisor says I have a design problem with my code:

class Program
{
    public static void Main()
    {
        UserInputValidator Validater = new UserInputValidator();
        int logSelected = -2;
        Console.Write("\nFilepath: ");
        string filePath = Console.ReadLine();
        while (logSelected != -1)
        {
            Console.WriteLine("\n" + "SELECT AN OPTION MENU");
            Console.WriteLine("-----------------------");
            Console.Write("\n" +
            /* Display option menu here with several options */
            "9 ) Exit\n\n");
            Console.WriteLine("-----------------------");
            logSelected = Convert.ToInt32(Console.ReadLine());
            Validater.ValidateOptionsMenu(logSelected, filePath);
        }
    }

    public void DisplayMessage(string message)
    {
        Console.WriteLine(message);
    }
}

Validator.ValidateOptionsMenu() handles the logic for starting a process depending on the option that was selected.

This is a 3-layer application with Presentation, Business, and Data Access layer. Program is in the Presentation layer, and other layers use code similar to the following to display a message:

Program program = new Program();
program.DisplayMessage(message);

I need to understand why this is bad practice - my supervisor says the class Program only starts the application, like App class in WPF. It has to have only 1 responsibility. The interaction with user (my option menu) is delegated to another class. I am having a hard time understanding my supervisor, and I'm not sure how to fix it. Can anyone help me understand?

Answer 1

Its difficult to see with this example because the program is almost entirely trivial, however your supervisor is pointing out the fact that the Main method contains the core application logic (i.e. it defines how the application fundamentally behaves).

Why is this a bad thing? Well in this example its not really as the program is very simple (which is what makes it difficult to see), however in a more complex application this might cause a couple of problems:

1. Firstly Main may be responsible for a couple of additional tasks such as argument parsing and error handling. Its a good idea to ensure that each "component" (e.g. method or class) is responsible for only one thing - this would make Main responsible for both argument parsing and the main application loop.

2. Secondly (and more importantly) we often need to take chunks of functionality out and re-use them as part of a larger application (for example a windows forms application or maybe a web application).

Imagine that you have written the above code and now need to take the main "core" of the application (a loop that accepts text input and then outputs a text menu) and embed it as part of a separate Windows Forms application. Instead of reading from the console your app now needs to accept input from a text box, and instead of writing to the console it now needs to write output to a second text box.

At present you have some problems that would prevent you from being able to do this without changes to the code you have posted:

• Firstly the Program class is normally marked as internal and the Main class often has an args input and so you might not even be able to call this method (with sensible inputs) from another assembly.
• Secondly the code outputs directly to the console, providing no way to supply an alternative output mechanism.
• Finally the code also reads directly from the console, providing no way to accept input from an alternative source.

In fact in order to help understand this I'd recommend that you actually try this as an exercise:

• First attempt to modify your above code to allow you to re-use this in an external application.
• Then in a separate Windows Forms project add a reference to your console application and attempt to re-use this code so that the user interacts through text boxes instead of the console.

The end goal is that both your Windows Forms application and Console application work using the same code.

This exercise should help to highlight and explain some of the above.

Answer 2

There is a peice of information you are missing. Your UI layer is simply the Console object. The UI layer is the mediator between the user and the computer. With Console, you only have the abilty to write stuff to the console, and read input from the console. So your UI layer is very simple. The application however, also needs a controller.

The controller, which is often not mentioned in N-tier definitions, is responsible for telling the UI when and what to draw. In your case, Program is the controller. In more complicated applications the UI layer will tell the controller when the user has requested some sort of operation (e.g. update this information, show me widget B, etc.). Your application cannot not do this because your UI layer is the Console. It is requesting information from the UI layer via Console.ReadLine. So now that we have a good definition of what the UI layer does and what the controller does, lets use it to explain how to fix it.

After your controller recieves input, it should decide what lower layer operation to call, and format the input from the UI into something the lower layer can use. For example calling Convert.ToInt32 so that the string from the console can be passed to a function. You can take this formatted input and call your business layer.

Now when you call something from a lower layer, you usually expect some information to be returned. For example a log file, or a boolean value to indicate completion. The business layer needs to return this information so that something can be done with it. The business layer does not care what is done with it. It only cares about returning the correct information. Your controller knows what needs to be done with it, which is to write some text to the console. So the end of this operation concludes with the controller telling the UI how and what to write to the console, or draw on the screen, or display in a web browser.

In conculsion, all of the information in this concept, minus the examples specific to your situation, should carry over into any application you write. Basically the controller mediates between the UI and the business layer, and in most cases when someone is refering to the UI layer, they actually mean the UI Layer plus this controller.

Update: Its bad practice to do what you did because it becomes extremely difficult to maintain the software after it has been written. There is only one consistent thing in software development, and that is the application will change. A piece of software only gets developed once, that is a small portion of its life. It spends the rest of its time operating and being modified to accommodate new features. Until you have actually had to maintain code that does not follow this N-tier architecture, it is difficult to fully understand the purpose for the separation of concerns. The most important reason to separate these things is that you can adapt your software to unforeseen changes in a more efficient and reliable manor than when these boundaries are not observed.

Answer 3

I would recommend reading On the Criteria To Be Used in Decomposing Systems into Modules by D.L. Parnas. It's accessible, interesting, and relevant to the question you are asking. It may even provide the answer you are looking for.

Answer 4

When you supervisor says Program should only have one responsibility, what he means is this:

1. Program.Main() starts the application and gets everything running. That is one responsibility.
2. Program.DisplayMessage() shows messages to the user. That is another responsibility.

So your Program.Main() is in it's own category... like an infrastructure method that gets the other layers to start working together. But Program.DisplayMessage() is in the Presentation layer. You have tightly-coupled the infrastructure of your application to the presentation layer.

Also, as other people are saying, your Validator class is tightly-coupled to your Program class. If the option menu changes in Program.Main, you will need to change the Validator class too. The purpose of object-oriented design is to make it so that you do not need to make changes in more than one class at a time.

Answer 5

UserInputValidater needs to know about the options menu. Instead, think about putting in distinct methods on Validater that should be called. So Program would decide what method to call on UserInputValidater rather than UserInputValidater deciding. UserInputValidater is tightly coupled to your options menu at this point.

Ideally, Program.Main would call a presentation class. Then your presentation class would decide what method on UserInputValidater to run.

class Program {     
   public static void Main()
   {
        Presenter presenter = new Presenter();
         presenter.Start();
    }
}

class Presenter()
{
     public void Start()
     {  
            // all your UI code

            switch(userInput)
             {
                  case 1:
                   validator.(whatever method would deal with case 1) etc...
 ...
                  case 9:
                    //exit here or throw an event that Progam has subscribed to for signal to exit.
       }

     public void DisplayMessage(string message)
     {
          ....
     }
}

(consider this psuedo-code)

Answer 6

Sorry, I have to run so no time for an explanation, but here is some general code that would make it more OOP...good luck!

class Program
{
private IUserMenuRepository _userMenuRepository;
private UserMenu _userMenu;
private UserMenuService _userMenuService;

public static void Main()
{
    _userMenuRepository = new UserMenuRepository();
    _userMenuService = new UserMenuService(userMenuRepository);
    _userMenu = _userMenuService.GetAllUserMenuOptions();

    string filePath = Console.ReadLine();
    while (userMenu.LogSelected != LogSelectedType.Exit)
    {   
        foreach(var option in userMenu.Option)
        {
            Console.WriteLine(option);
        }
        userMenu.LogSelected = (LogSelectedType)Console.ReadLine();
    }
}

public void DisplayMessage()
{
    Console.WriteLine(_userMenuService.DisplayMessage(_userMenu)));
}
}

// DOMAIN LAYER
// Base Model class for all domain objects to inherit
 public class ModelBase
 {
private IList<string> _brokenRules {get;set;}

public ModelBase()
{
    _brokenRules = new List<string>();
}

public void AddBrokenRule(string brokenRule)
{
    _brokenRules.Add(brokenRule);
}

public abstract CheckForBrokenRules();

public bool IsValid()
{
    return _brokenRules.Count == 0;
}
}
// Model
public class UserMenu : ModelBase
 {
public int Id {get;set;}
public IList<string> Options {get;set}
public string FilePath {get;set;}
public LogSelectedType LogSelected {get;set;}

public UserMenu()
{
    LogSelected = LogSelectedType.Continue;
}

public override CheckForBrokenRules()
{
    // validate here
}

public enum LogSelectedType
{
    Exit = -1,
    Continue = -2
}
}


// Domain Service
public class UserMenuService
{
private IUserMenuRepository _userMenuRepository;
public class UserMenuService(IUserMenuRepository userMenuRepository)
{
    _userMenuRepository = userMenuRepository;
}

public IList<UserMenu> GetAllUserMenuOptions()
{
    _userMenuRepository.FindAll();
}

public string DisplayMessage(UserMenu userMenu)
{
    // return message
}
}

// Contract used for persistence
public interface IUserMenuRepository
{
IList<UserMenu> FindAll();
}

// DATA LAYER
// Repository layer used to pull the data
public class UserMenuRepository : IUserMenuRepository
{
public IList<UserMenu> FindAll()
{
    // connect to database and populate model for persistence
}
}