1
\$\begingroup\$

I wrote small console application where you can choose an item from given list of strings. Works fine so far but since I am beginner I have some questions if I am doing this right. My biggest concerns are if I should use fields here or properties, if I am using the static keyword correctly, if I should use the fields in the methods directly (like I am doing it now) or I should pass them as parameters. In future this class I want to use in multiple places so it makes sense to be static I think. If you have suggestions on some other parts also please comment.

ConsoleSelectMenu.cs:

namespace ConsoleApp
{
    class ConsoleSelectMenu
    {
        static int selectedLineIndex = 0;
        static ConsoleKey pressedKey;
        static bool readyToSelectUp = true;
        static bool readyToSelectDown = true;
        static bool haveWeStarted = false;

        public static string DrawSelectMenu(SelectMenuOptions options)
        {
            Console.WriteLine(options.Question);
            do
            {
                UpdateMenu(options.Choices);
                pressedKey = Console.ReadKey().Key;
                Console.Write("\b \b");

                readyToSelectUp = pressedKey == ConsoleKey.DownArrow && selectedLineIndex + 1 < options.Choices.Count;
                readyToSelectDown = pressedKey == ConsoleKey.UpArrow && selectedLineIndex - 1 >= 0;

                if (readyToSelectUp) selectedLineIndex++;
                else if (readyToSelectDown) selectedLineIndex--;

            } while (pressedKey != ConsoleKey.Enter);
            return options.Choices[selectedLineIndex];
        }

        private static void UpdateMenu(List<string> list)
        {
            if (!readyToSelectUp && !readyToSelectDown) return;
            if (haveWeStarted) ClearList(list);

            foreach (var item in list)
            {
                bool isSelected = item == list[selectedLineIndex];
                Console.WriteLine($"{(isSelected ? "> " : "  ")}{item}");
            }
            haveWeStarted = true;
        }

        private static void ClearList(List<string> list)
        {
            for (int i = 0; i < list.Count; i++)
            {
                int currentLineCursor = Console.CursorTop;
                Console.SetCursorPosition(0, Console.CursorTop - (i + 1));
                Console.Write(new string(' ', Console.WindowWidth));
                Console.SetCursorPosition(0, currentLineCursor);
            }
            Console.SetCursorPosition(0, Console.CursorTop - list.Count);
        }
    }

    public class SelectMenuOptions
    {
        public string Question { get; set; }
        public List<string> Choices { get; set; }
    }
}

Programs.cs

var options = new SelectMenuOptions
{
    Question = "Please choose an option",
    Choices = new List<string>() { "Opt_1", "Opt_2", "Opt_3" }
};

var result = ConsoleSelectMenu.DrawSelectMenu(options);
Console.WriteLine(result);
\$\endgroup\$
2
  • \$\begingroup\$ How does this question differ from your previous one? \$\endgroup\$ Dec 20, 2022 at 20:06
  • \$\begingroup\$ This one is made into a static class that should be reusable. The previous one wasn't. Like it says in the description i am worried most about those aspects, like fields vs properties etc... The logic is almost the same and I am not so worried here about that (we solved that in the previous question). The things I am worried are stated in the description. I read in the guideline that It should be better if I just make another question. If I am wrong I will delete this no problem. \$\endgroup\$ Dec 20, 2022 at 21:29

1 Answer 1

2
\$\begingroup\$

Firstly, about fields vs. properties: properties are methods in disguise - they allow you to use logic when changing a variable. Personally, I would almost always use the standard Property property { get; set; } so it's faster to customize the way to read and update a property - unless we want the property to be private.

Secondly, and more importantly, I wouldn't make DrawSelectMenu a static class. The reason for that is when you make a class static, you no longer get the benefits of abstraction, and Dependency Injection. Instead, I would create the following abstract class:

internal abstract class SelectMenu
{
    public string Title { get; set; } = "";
    public List<Option> Options { get; set; } = new List<Option>();
    public Option? SelectedOption { get; set; }

    public SelectMenu(string title, List<Option> options)
    {
        this.Title = title;
        this.Options = options;
    }
    public abstract void Display();

    public void SelectPrevious()
    {
        if (this.SelectedOption != null)
        {
            int indexOfSelected = this.Options.IndexOf(this.SelectedOption);
            if (indexOfSelected != -1)
                this.SelectedOption = this.Options.ElementAt(Modulus((indexOfSelected - 1), this.Options.Count));
        }
    }
    public void SelectNext()
    {
        if (this.SelectedOption != null)
        {
            int indexOfSelected = this.Options.IndexOf(this.SelectedOption);
            if (indexOfSelected != -1)
                this.SelectedOption = this.Options.ElementAt(Mod((indexOfSelected + 1), this.Options.Count));
        }
    }
    private static int Modulus(int a, int b)
    {
        return (Math.Abs(a * b) + a) % b;
    }
}

And then, I would create the following derived class:

internal class ConsoleSelectMenu : SelectMenu
{
    public ConsoleSelectMenu(string title, List<Option> options) : base(title, options)
    {
        this.SelectedOption = this.Options[0];
    }

    public override void Display()
    {
        Console.Clear();
        this.Options.ForEach((option) => Console.WriteLine(this.GetPrefix(option) + option.Name));
    }

    private string GetPrefix(Option option)
    {
        return this.SelectedOption == option ? "> " : "  ";
    }
}

And so Program.cs main function becomes:

var options = new List<Option>() {
    new Option { Name = "opt 1" },
    new Option { Name = "opt 2" },
    new Option { Name = "opt 3" }
};

string title = "Please choose an option";

SelectMenu selectMenu = new ConsoleSelectMenu(title, options);

while (true)
{
    selectMenu.Display();

    var key = Console.ReadKey().Key;

    if (key == ConsoleKey.UpArrow)
        selectMenu.SelectPrevious();
    else if (key == ConsoleKey.DownArrow)
        selectMenu.SelectNext();
    else if (key == ConsoleKey.Enter)
        break;
}

Console.WriteLine(selectMenu?.SelectedOption?.Name);

And that solution allows flexibility; if we'd want, for example, a WindowSelectMenu, we could simply create a class that implements the required methods, and just replace the type of SelectMenu when we create it: SelectMenu selectMenu = WindowSelectMenu(title, options)

We can also refactor the user input handling, and create a generic system for handling input and dispatching events (for example, an OnEnterKeyPressed event).

Thirdly, you can still use a class even if it's not static; you just need to pass a reference to the instance or create a new one. For example:

public Option petOwnershipSurvey()
{
    SelectMenu selectMenu = ConsoleSelectMenu("Do you own a pet", new List<Option>() {
        new Option { Name = "Yes" },
        new Option { Name = "No" },
        new Option { Name = "I don't know" }});
    selectMenu.Display();
    /* insert here a code that gets input from the user */
    
    return selectMenu.SelectedOption;
}
  • here's the Option class I created:
internal class Option
{
    public string Name { get; set; } = "";
    public string Description { get; set; } = "";
}
  • Also, please note that I haven't addressed all edge cases in my code in order to focus on what's relevant to the question.
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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