Client to connect to server and show a text menu

Question

First of all: Sorry for my English, that said...

I am developing an obligatory for my University, so far it has a class that provides the user with options to connect to a server (once connected it should offer other functionalities, but I only reached the connection option so far), it only shows console options, and checks they are correctly chosen, nothing else.

The class has everything (including itself) static, the reason is simple: I didn't considered necessary to instantiate that class, not even once (what is allowed by using Singleton Pattern), and so I decided going everything static inside it.

I showed my code to my teacher, who said It was an awful design more or less :( well, he suggested I used Singleton and I explained that I understood Singleton to be only useful when you wanted (needed) only one instance for the class, I don't need even one!

He said static variables are global, which could cause conflict with other variable names and then I would have to remember the static variable names I used so as to avoid conflict, so I highlighted that the class has everything private (except a method called ShowStartMenu() which is internal, and is the one called in the Main) and then he almost exploded...

He told me it wasn't a good design because it didn't allow extensibility (what if I wanted to add more options to the menu, I had to add them in that class... which is exactly what I intended, is that wrong?), and had other disadvantages I could ask my design teacher ¬¬

So, I want to show you my code and ask you what do you think about it.

using System;

namespace ARIP.Client.Console
{
    internal static class ClientOptions
    {
        private const int START_MENU_CONNECT_TO_SERVER_OPTION = 1;
        private const int START_MENU_END_APPLICATION_OPTION = 0;

        private static int _minOption;
        private static int _maxOption;
        private static int _givenOption;

        #region General Client-Option's Functions

        private static void askForUserOption()
        {
            System.Console.WriteLine("");
            System.Console.Write("Su opción: ");
        }

        private static void readUserOption(Action methodCaller)
        {
            try
            {
                string givenOptionAsString = System.Console.ReadLine();
                _givenOption = int.Parse(givenOptionAsString);
            }
            catch (FormatException)
            {
                System.Console.WriteLine("");
                System.Console.WriteLine("Atención! Sólo se permite ingresar dígitos. Intente nuevamente.");
                methodCaller();
            }
        }

        private static void checkUserOptionIsBetweenMinAndMaxOptions(Action methodCaller)
        {
            if (!givenOptionBetweenMinAndMaxOptions())
            {
                showInvalidOptionMessage();
                methodCaller();
            }
        }

        private static bool givenOptionBetweenMinAndMaxOptions()
        {
            return _givenOption >= _minOption && _givenOption <= _maxOption;
        }

        private static void showInvalidOptionMessage()
        {
            System.Console.WriteLine("");
            System.Console.WriteLine("Atención! La opción ingresada no es válida, debe ingresar un número entero entre " + _minOption + " y " + _maxOption + ".");
        }

        #endregion

        #region Start Menu

        internal static void ShowStartMenu()
        {
            System.Console.WriteLine("Menú Principal");
            System.Console.WriteLine("");
            System.Console.WriteLine("Ingrese la opción deseada:");
            System.Console.WriteLine(START_MENU_CONNECT_TO_SERVER_OPTION + "-Conectarse a un Servidor");
            System.Console.WriteLine("");
            System.Console.WriteLine(START_MENU_END_APPLICATION_OPTION + "-Terminar Aplicación");
            setMinAndMaxOptionsForStartMenu();
            askUserOptionForStartMenu();
            startMenuResponsalToOptionGiven();
        }

        private static void setMinAndMaxOptionsForStartMenu()
        {
            _minOption = START_MENU_END_APPLICATION_OPTION;
            _maxOption = START_MENU_CONNECT_TO_SERVER_OPTION;
        }

        private static void askUserOptionForStartMenu()
        {
            askForUserOption();
            readUserOption(askUserOptionForStartMenu);
            checkUserOptionIsBetweenMinAndMaxOptions(askUserOptionForStartMenu);
        }

        private static void startMenuResponsalToOptionGiven()
        {
            if (_givenOption == START_MENU_CONNECT_TO_SERVER_OPTION)
            {
                //TODO Conectarse a un Servidor y desplegar las demás opciones
                //TODO crear las demas opciones una vez conectado
                throw new NotImplementedException();
            }
        }

        #endregion
    }
}

Main code:

namespace ARIP.Client.Console
{
    class Program
    {
        static void Main(string[] args)
        {
            ClientOptions.ShowStartMenu();
        }
    }
}

Answer 1

Regarding static solution, I agree with your teacher. This design makes handling nested menus virtually impossible. Once you consider submenus, it becomes obvious that menu title, texts of menu choices, option limits, etc are instance members, and ShowStartMenu should naturally iterate over choices instead of hardcoding them.

Two other design choices make me very uncomfortable.

First, goal of menu is to compute the user choice. What the rest of applications is going to do with this choice is not of menu's business. It means that _givenOption doesn't belong to the ClientOption class, but better be returned from ShowStartMenu to the caller.

Second, the recursive nature of error handling is totally uncalled for. Consider instead

            private static int askUserOptionForStartMenu()
            {
                while (true) {
                    askForUserOption();
                    try {
                        int user_choice = readUserOption();
                        checkUserOptionIsBetweenMinAndMaxOptions(user_choice);
                        return user_choice;
                    } catch (FormatException) {
                        ....
                    } catch (RangeException) {
                        ....
                    }
                }
            }

Answer 2

using System

That first line of code makes it unnecessary to give the fully qualified class name. However, you always use

System.Console.Whatever()

while you could just do

Console.Whatever()

Either make use of your usage of using System or don't use it in the first place.

START_MENU_OPTION

You have those to make code more readable and give some constant values a name. The thing is that they are not defined as a type and you could still exchange them for any other value. Any function you have that expects one of those can only demand int typed parameters. It cannot explain that sensible values are stored in some static const members somewhere.

Use an enum for that instead, something like

enum StartMenuOption {ConnectToServer, EndApplication}

those recursive method calls

Your intention here is to jump around in your code. The problem with your method is that it might nest method calls indefinitely.

Calling askUserOptionForStartMenu() leads to calling readUserOption() which may lead to calling askUserOptionForStartMenu() rinse repeat

When you "arrive" at askUserOptionForStartMenu() the second time, the first call isn't actually over, you create a nested call structure which is called recursion.

In your case, this isn't necessary. It makes it hard to follow your code because it jumps around from one method to another. If you want to keep doing something until some condition is met, use a while loop, see section below.

Parse -> TryParse

You want to use TryParse to read user input.

private static int readUserInt()
{
    int userInput;

    while(!intTryParse(Console.ReadLine(), out userInput))
    {
        Console.WriteLine("Invalid integer input, please try again");
    }
    
    return userInput
}

This method polls the user until he enters a valid int. A very important concept is that the method actually returns the value instead of storing it into some arbitrary variable. This makes it possible to reuse it.

Enum.IsDefined

You can combine this with the enum.

private static StartMenuOption readUserOption()
{
    int userInput;
    
    while(!Enum.IsDefined(typeof(StartMenuOption), userInput = readUserInt()))
    {
        Console.WriteLine("Valid integer input is not a valid option, please try again");
    }
    
    return (StartMenuOption) userInput;
}

This makes _minOption, _maxOption, setMinAndMaxOptionsForStartMenu(), givenOptionBetweenMinAndMaxOptions() and checkUserOptionIsBetweenMinAndMaxOptions() obsolete. There's a single definition of all values of the enumeration. The check for valid values is derived from that definition.

methods for text output

The reusability of the methods askForUserOption() and showInvalidOptionMessage() is questionable. You call those only once.

On the contrary, you hard coded other text output without placing it into a separate method:

        System.Console.WriteLine("Menú Principal");
        System.Console.WriteLine("");
        System.Console.WriteLine("Ingrese la opción deseada:");

Having some text in methods and other text not is inconsistent.

choose your battles

The whole discussion about Singletons, static, etc. is somewhat moot. Your example is just too small. Which makes the arguments for your design plausible

The class has everything (including itself) static, the reason is simple: I didn't considered necessary to instantiate that class, not even once (what is allowed by using Singleton Pattern), and so I decided going everything static inside it.

Yes, that's true. But that's not the important thing to take away from this lesson. You're too eagerly trying to apply some restriction to this code.

Just because something might be only instantiated once, doesn't necessarily mean you should make it a singleton.

Just because something might never be instantiated, doesn't necessarily mean it should be all static methods.

While it's certainly possible to make your application entirely static, there's no real advantage in doing it. In the future, concepts like inheritance will be introduced along techniques like unit tests that rely on them. This is where the "everything is static" idea will fall apart completely.

Singleton special disclaimer

This is the pattern that is abused the most. Your teacher tried to address this with what he referred to as "global". Your understanding

I understood Singleton to be only useful when you wanted (needed) only one instance for the class

is indeed correct. But the pattern has some side effects that lead to horribly designed code.

Be very conservative when applying this pattern and only do so if you really have to for the reason mentioned in your quote and only for that reason.

multiple instances

So you wouldn't suggest Singleton nor static... what would be the solution in this case? allowing multiple instances?

Allowing multiple instances is indeed what I suggest. Consider it to be the "default" thing to start with.

A distributed system with a server/client architecture is in general considered to have any number of clients. Just imagine you want to implement some communication from client to server and then to all connected clients. Wouldn't it be great to write a test for that:

1. instantiating two clients, both connecting to the server,
2. being able to shove data into one client
3. while testing what the other one receives?

I think so. Again, for your situation, it makes sense to argue that everything could be static, but it misses the bigger picture, which you may not have seen yet as you are still learning. Keep going.

Answer 3

Neither singleton nor everything static per se is bad. It's how you are going to use it that renders one or the other design in some particualar scenario as not the optimal choice.

Your code won't cause you many troubles but I think it's rather a coincidence than a intentional.

private static int _minOption;
private static int _maxOption;
private static int _givenOption;

In a class this is virtually always bad. You set the first two to some constant values... so why do you need those additional fields? You can use the constants directly.

The third field will however cause you troubles... imagine you code runs on a server where two users at the same time can login and use the readUserOption.

In this method you do:

string givenOptionAsString = System.Console.ReadLine();
_givenOption = int.Parse(givenOptionAsString);

You assign _givenOption a value specified by the user1, let's say 10, then the other user2 calls the same method and gives a different value 22.. now user1 calls this;

private static bool givenOptionBetweenMinAndMaxOptions()
{
  return _givenOption >= 15; // deliberately modified
}

and what he sees is false. Why? Because there is only one _givenOption shared between everyone and there is no other way becasue even if you create an instance of the class it still works with static fields.

---

Singleton was useful for having one instance of the class which could help you testing for example.

This is not necessrily true - in general. Usually tests can run in parallel and they will overwrite each others data so you'll get invalid results in each test. The same will happen as described above.

Singleton != static - singleton is a single instance of itself - for testing you can create several different instances and test each one separately. Later in your application you can use only one if this is the requirement. If anything other then methods is static (like fields) then it usually means problems.

Answer 4

In addition to what the other reviewers have mentioned

• It's always better to restrict your Console.WriteLine to your Main method. Hence, you wouldn't require a method such as askForUserOption to print to the console. Likewise showInvalidOptionMessage
• If you ask me _givenOption should have been implemented as a property rather than a variable as you use it to get and set the content in startMenuResponsalToOptionGiven and readUserOption methods
• I'm afraid this class breaks Single Responsibility of SOLID PRINCIPLE; the class is responsible for reading, writing and handling exceptions
• As @I'll add comments tomorrow indicated the recursion achieved by methodCaller can be replaced using a while..loop. This will avoid readUserOption, checkUserOptionIsBetweenMinAndMaxOptions and showInvalidOptionMessage methods
• I hate to be a spoiler ...but I don't see this code as being unit-testable. Almost all the methods are making several method calls. You want to separate printing to the screen and reading from the screen i.e ShowStartMenu