Print a Menu in the Command Line following SOLID

Question

Problem Description

I am building a library that helps me building TUI Apps more faster, for example printing TUI Menus and other components. I would like to get feedback on my design decisions to see if I'm properly following SOLID principles.

To solve this scenario while trying to follow the principles I created a class called TUIMenu that has the responsibility of giving access the object properties and methods. Then I created another class named TUIMenuPrinter that has the responsibility of printing a TUIMenu with a specified format.

All is working correctly, but even with this division of responsibility I found a little bit tedious to create a TUIMenu object and then creating a TUIMenuPrinter object to pass as a parameter the previous TUIMenu object. So I would like to know what do you think, is this solution has a good design or maybe I should improve in something. I was also thinking about implementing the TUIMenuPrinter as a Inner Class or a Static Class, so I don't need to instantiate another object.

Current Solution Code

UIMenu class

using System.Collections.Generic;

namespace CLILibrary
{
    public class UIMenu : UIControl
    {
        public string Title { get; set; }
        public List<string> MenuRows { get; }

        public UIMenu() => MenuRows = new List<string>();

        public void AppendLine(string line)
        {
            MenuRows.Add(line);
        }
    }
}

UIMenuPrinter class

using System;
using System.Collections.Generic;
using System.Text;
using CLITools;

namespace CLILibrary.UIComponentPrinters
{
    public class UIMenuPrinter : UIComponentPrinter
    {
        public UIMenu UiMenu { get; set; }
        
        public char FrameDecorationChar { get; set; } = '=';
        public char SeparationChar { get; set; } = ' ';
        public char RowDecorationChar { get; set; } = '|';

        public UIMenuPrinter (UIMenu uiMenu)
        {
            UiMenu = uiMenu;
        }

        public override void Print()
        {
            var values = new Stack<string>();
            _StackPropertiesLoader(ref values);
            var stringMeter = new StringMeter(values);
            var largestStringSize = stringMeter.MeasureLargestString();
            var largestString = (largestStringSize >= 40) ? largestStringSize : 40;
            
            StringBuilder formatBuilder = new StringBuilder();

            var frameDecorationLine = StringRepeater.Repeat(FrameDecorationChar.ToString(), (int) largestString);
            var spacesLine = StringRepeater.Repeat(SeparationChar.ToString(), (int) largestString);

            formatBuilder.Append(SeparationChar).Append(frameDecorationLine).Append(SeparationChar);
            
            formatBuilder.AppendLine();

            formatBuilder.Append(RowDecorationChar).Append(spacesLine).Append(RowDecorationChar).AppendLine();
            formatBuilder.Append(RowDecorationChar).Append(_FillRow(UiMenu.Title, (int) largestString)).Append(RowDecorationChar).AppendLine();
            formatBuilder.Append(RowDecorationChar).Append(spacesLine).Append(RowDecorationChar).AppendLine();
            
            formatBuilder.Append(SeparationChar).Append(frameDecorationLine).Append(SeparationChar).AppendLine();
            
            UiMenu.MenuRows.ForEach(Row =>
            {
                formatBuilder.Append(RowDecorationChar).Append(_FillRow(Row, (int) largestString)).Append(RowDecorationChar).AppendLine();
            });
            
            formatBuilder.Append(SeparationChar).Append(frameDecorationLine).Append(SeparationChar).AppendLine();
            
            Console.WriteLine(formatBuilder.ToString());
        }

        private string _FillRow(string row, int limitLength)
        {
            return $"{row}{StringRepeater.Repeat(SeparationChar.ToString(), limitLength - row.Length)}";
        }

        private void _StackPropertiesLoader(ref Stack<string> propertiesStack)
        {
            foreach (string Row in UiMenu.MenuRows)
            {
                propertiesStack.Push(Row);
            }

            propertiesStack.Push(UiMenu.Title);
        }
    }
}

SandBox Console Application for Basic Testing Code

using System;
using CLILibrary;
using CLILibrary.UIComponentPrinters;
using CLITools;

namespace SandBox
{
    internal class Program
    {
        public static void Main(string[] args)
        {

            var mainMenu = new UIMenu
            {
                Title = "Do you like to drink coffee?"
            };

            mainMenu.AppendLine("[1] Yes");
            mainMenu.AppendLine("[2] No");

            var uiMenuPrinter = new UIMenuPrinter(mainMenu);
            uiMenuPrinter.FrameDecorationChar = '*';


            uiMenuPrinter.Print();

            Console.ReadLine();
        }
    }
}

Extra Details

• I have one class that is an Abstract Class named UIControl that is the Parent of all the specific UI Components like UIMenu is.

• Also with the UIPrinter object I have a Parent of all the Component Printers, its name is UIComponentPrinter.

UML Class Diagram

Output Result

UPDATE

Thank you so much for your feedback comments!

Officially this is considered as a TUI, so I changed all my object names.

TUI (Text-based user interface) - similar to GUI, but instead of graphics, interface is drawn by text (mostly ASCII) symbols. Examples: Vim, Mutt

Someone asked me for the library code where I define the StringMeter and the StringRepeater, so here is the code :D

StringMeter Code

using System;
using System.Collections.Generic;
using System.Linq;

namespace CLITools
{
    public class StringMeter
    {
        private IEnumerable<string> _strings;

        public IEnumerable<string> Strings
        {
            get => _strings;
            set => _strings = value ?? throw new ArgumentException(nameof(_strings));
        }

        public StringMeter(IEnumerable<string> strings)
        {
            Strings = strings;
        }

        public double MeasureLargestString()
        {
            double largestStr = 0; 
            
            Strings.ToList().ForEach(eachString =>
            {
                if (!(eachString.Length > largestStr)) return;
                largestStr = eachString.Length;
            });
            
            return largestStr;
        }
        
        public double MeasureSmallestString()
        {
            double smallestString = double.MaxValue; 
            
            Strings.ToList().ForEach(eachString =>
            {
                if (eachString.Length > smallestString) return;
                smallestString = eachString.Length;
            });
            
            return smallestString;
        }
    }
}

StringRepeater Code

using System.Text;

namespace CLITools
{
    public class StringRepeater
    {
        public static string Repeat(string value, int count)
        {
            return new StringBuilder(value.Length * count).Insert(0, value, count).ToString();
        }
    }
}

As you can see this code is part of another project that I have (Class Library) called CLITools.

Answer 1

Here are my observations:

UIMenu

• Try to use consistent naming
  • Your current naming: Title, MenuRows, AppendLine
  • Suggested naming #1: Title, Items, AddItem
  • Suggested naming #2: MenuHeader, MenuRows, AddMenuRow
• You don't need the parameterless ctor
  • You can simply initialize the MenuRows when you declare it

public List<string> MenuRows { get; } = new List<string>();

• I would also suggest to consider a method which can receive multiple menu items, not just one

StringRepeater

• Your class could be marked as static, since it has only static members
• The code is quite error-prone since it does not check the parameters
  • if value is null >> NullReferenceException at value.Length
  • if count is negative >> ArgumentOutOfRangeException at new StringBuilder(

StringMeter

• _strings = value ?? throw new ArgumentException(nameof(_strings))
  • It is better to perform the validation before calling the setter
  • You are revealing an implementation detail to the caller nameof(_strings)
• You are calling the ToList each and every time when you access the _strings
  • It might make more sense to store it as List
• Strings declared as public even though it is used only from the class itself
• MeasureXYZ return double even though a string length can be only a positive integer
  • There implementations are too lengthy, they can be implemented as simple LINQ queries

public class StringMeter
{
    private readonly List<string> CollectionOfStrings;

    public StringMeter(IEnumerable<string> strings)
        => CollectionOfStrings = strings?.ToList() ?? throw new ArgumentException(nameof(strings));
        
    public int MeasureLargestString()
        => CollectionOfStrings.Select(s => s.Length).OrderByDescending(l => l).First();

    public int MeasureSmallestString()
        => CollectionOfStrings.Select(s => s.Length).OrderBy(l => l).First();
}

UIMenuPrinter

• FrameDecorationChar ... I know naming is hard but do not suffix the properties with the data type
  • Suggestions: FrameDecorator, Separator and RowDecorator
• I think if you change your API in the way that Print method anticipates a UIMenu instance, not the ctor, then a single Printer instance can be reused
• This whole logic with Stack is an overkill
  • The same can be achieved with this simple LINQ

uiMenu.MenuRows.Concat(new[] { uiMenu.Title }).Reverse()

• largestStringSize >= 40 rather than hard coding magic numbers I would suggest to make it as a parameter with default value
• formatBuilder yet again naming ... Try to capture the purpose of the variable, like menuBuilder, textualMenuVisualizer
• _FillRow In C# it is pretty uncommon to start with underscore a private method

Answer 2

mainMenu.AppendLine("[1] Yes");

This part is in need of some improvements.

AppendLine is not a descriptive enough name. AddOption, AddChoice, AddMenuItem, ... would be better.

One thing I also suspect is that you probably haven't though through the actual selection logic yet. You've encoded the input values (1, 2) into the string, which is inherently going to lead to DRY violations when you have to write the subsequent logic for checking which value the user has put in.

Additionally, you've also encoded your UI formatting (the [] brackets), but deciding what things should look like is really the responsibility of the UIMenuPrinter.
Additionally, because you are hardcoding the formatting, your UI can't account for e.g. longer numbers, and being able to print all the [] brackets using the same (max) width.

Taking these things into account, a much cleaner interface would be:

    mainMenu.AddChoice( 1 , "Yes"); // input value = int
    mainMenu.AddChoice('1', "Yes"); // input value = char
    mainMenu.AddChoice("1", "Yes"); // input value = string

Whether the input value is an int, or if you'll allow any char or even a string is up to you to decide.

---

var uiMenuPrinter = new UIMenuPrinter(mainMenu);

You've invariably tied a single printer to a single menu. That is impeding your ability to reuse the same printer across an application, which would be really helpful to ensure a consistent UI across the application.

A better interface would be:

var uiMenuPrinter = new UIMenuPrinter();

uiMenuPrinter.Print(mainMenu);

if(mainMenuChoice == 1) // just an example
    uiMenuPrinter.Print(subMenu);

---

var mainMenu = new UIMenu
{
    Title = "Do you like to drink coffee?"
};

// ...

var uiMenuPrinter = new UIMenuPrinter(mainMenu);
uiMenuPrinter.FrameDecorationChar = '*';

Either favor object initializers or don't; but I suggest not mixing and matching as you go, because it will detract from your style and readability.

---

public UIMenu() => MenuRows = new List<string>();

While it compiles and does what you want it to, I would suggest not using => for constructor bodies. => is used for returned values, and the only real reason the compiler allows the above is because a command doesn't technically return a value.

It works, but it ain't pretty, and I'd flag it in a code review. Stick with method bodies for a constructor and leave => for one-liner methods with a trivially returned value.

---

private string _FillRow(string row, int limitLength)

private void _StackPropertiesLoader(ref Stack<string> propertiesStack)

The _underscore naming convention applies to private fields, but not private methods. Personally, I don't even like it for private fields and prefer to omit the leading underscore, but that's a different discussion for a different day.

Secondly, method names should be imperative (do this, get that, ...), not nouns or noun phrases. LoadProperties is more idiomatic.

---

var values = new Stack<string>();
LoadProperties(ref values);

ref has its uses, but in this case it's not really needed. Because you are refing a newly created object, you can just do:

var values = LoadProperties();

---

StringMeter is obsolete since the advent of LINQ.

IEnumerable<string> myStrings = ...;

int shortest = myStrings.Min(s => s.Length);
int longest = myStrings.Max(s => s.Length);

Additionally, it's unclear to me why you decided to work with double, as string lengths are always integers.