C#•2y ago

❔ Any tips on how to make this function cleaner? I feel it's a mess

I tried to copy this interactive menu from this video: https://www.youtube.com/watch?v=YyD1MRJY0qI Source code: https://github.com/ricardogerbaudo/Console.InteractiveMenu My code: https://github.com/JsPeanut/InteractiveMenu/blob/main/TestingArea/TestingArea/Program.cs I feel my function is a mess... Would appreciate any help

Ricardo Gerbaudo

YouTube

How To Create a Interactive Menu in Console App Using C#

How to create a interactive menu in #console app using #csharp 📁 Source code: https://github.com/ricardogerbaudo/Console.InteractiveMenu 00:00 — Welcome 00:24 — Creating a new console project 01:30 — Opening the project in Visual Studio Code 01:56 — Adding VS Code build and debug assets to the project 02:20 — Displaying the title in the conso...

GitHub

GitHub - ricardogerbaudo/Console.InteractiveMenu

Contribute to ricardogerbaudo/Console.InteractiveMenu development by creating an account on GitHub.

GitHub

InteractiveMenu/Program.cs at main · JsPeanut/InteractiveMenu

Interactive menu. Contribute to JsPeanut/InteractiveMenu development by creating an account on GitHub.

18 Replies

Haze.•2y ago

if (welcomemsg == "")
{

}
else
{
   Console.WriteLine($"{welcomemsg}");
}
Console.ResetColor();
if (arrowstousemsg == "")
{

}
else
{
   Console.WriteLine($"{arrowstousemsg}");
}

if (welcomemsg == "")
{

}
else
{
   Console.WriteLine($"{welcomemsg}");
}
Console.ResetColor();
if (arrowstousemsg == "")
{

}
else
{
   Console.WriteLine($"{arrowstousemsg}");
}

could be simplified to

if (welcomemsg != ""){
  Console.WriteLine($"{welcomemsg}");
}
Console.ResetColor();
if (arrowstousemsg != ""){
   Console.WriteLine($"{arrowstousemsg}");
}

if (welcomemsg != ""){
  Console.WriteLine($"{welcomemsg}");
}
Console.ResetColor();
if (arrowstousemsg != ""){
   Console.WriteLine($"{arrowstousemsg}");
}

also replacing the "" with String.Empty might be better, i cant remember

                    case ConsoleKey.UpArrow:
                        if (arrow1 == "UpArrow")
                        {
                            option = option == 1 ? options : option - 1;
                        }
                        else
                        {
                        }
                        break;

                    case ConsoleKey.UpArrow:
                        if (arrow1 == "UpArrow")
                        {
                            option = option == 1 ? options : option - 1;
                        }
                        else
                        {
                        }
                        break;

you dont need else statements for if statements (also you can nest tertiary operators if you really wanted to

option = arrow1 == "UpArrow" ? (option == 1 ? options : option - 1) : option;

option = arrow1 == "UpArrow" ? (option == 1 ? options : option - 1) : option;

) there might be some stuff ive missed but im too tired

Keswiik•2y ago

for one you should really replace the optX parameters with a variadic param params string[] opts or some other enumerable (note that if you do this you would need to also update your navigation logic to use the length of the opts array to determine the max option available) you are also duplicating logic with your switch used to handle menu navigation not sure what the point in passing in arrow1 arrow2 arrow3 and arrow4 are when you are already switching on ConsoleKey.UpArrow, ConsoleKey.DownArrow, ConsoleKey.LeftArrow, ConsoleKey.RightArrow, then doing a redundant check with a hardcoded string if you want to customize navigation bindings, you would be better off passing in an enumerable/list/array of ConsoleKey which you could then use for positive / negative nav

Haze.•2y ago

some code from chatgpt (the best coder) which does actually seem decent

case ConsoleKey.UpArrow:
case ConsoleKey.LeftArrow:
    if (arrow1 == "UpArrow" || arrow1 == "LeftArrow")
    {
        option = option == 1 ? options : option - 1;
    }
    break;

case ConsoleKey.UpArrow:
case ConsoleKey.LeftArrow:
    if (arrow1 == "UpArrow" || arrow1 == "LeftArrow")
    {
        option = option == 1 ? options : option - 1;
    }
    break;

Keswiik•2y ago

i'd also recommend doing !string.IsNullOrWhiteSpace(welcomemsg) instead of != "" or you just get rid of op1, opt2, opt3, opt4, opt5, and opt6 and update the method signature to have a single list, which was my original suggestion. No point in updating logic of the method to be flexible while keeping that mess of a signature around which forces you to use 6 parameters.

Haze.•2y ago

yeah that code was just copied from chatgpt and i couldnt be bothered to remove the optionsArray part

Keswiik•2y ago

Then I would probably advise against copy-pasting chat GPT code that could confuse someone asking for help, as it could end up misleading them

Haze.•2y ago

eh it works

for (int i = 0; i < opts.Length; i++)
{
    if (!string.IsNullOrWhiteSpace(opts[i]))
    {
        Console.WriteLine($"{(option == i + 1 ? decorator : "  ")}{opts[i]}\u001b[0m");
    }
}

for (int i = 0; i < opts.Length; i++)
{
    if (!string.IsNullOrWhiteSpace(opts[i]))
    {
        Console.WriteLine($"{(option == i + 1 ? decorator : "  ")}{opts[i]}\u001b[0m");
    }
}

ill remove the misleading code

XDOP•2y ago

Thank you for your answers guys, I'll try this later

Aurumaker72•2y ago

You won't learn anything by copy-pasting code from the internet without any prior cognitive effort

XDOP•2y ago

That's true, thank you

XDOP•2y ago

https://github.com/JsPeanut/InteractiveMenu/blob/main/TestingArea/TestingArea/Program.cs

GitHub

InteractiveMenu/Program.cs at main · JsPeanut/InteractiveMenu

Interactive menu. Contribute to JsPeanut/InteractiveMenu development by creating an account on GitHub.

XDOP•2y ago

Guys, I was able to optimize my code. Thank you all so much! @keswiik @Haze. and @Aurumaker72 because I tried to understand more of the code thanks to the advice. And gladfully I came to that solution without copy-pasting it from your code haha

Keswiik•2y ago

@.._ Definitely looks better than the first attempt, but there's still some more you can do to clean it up. numberOfOptions is redundant, just check the length of the opts array. Your switch statement on key.Key still isn't very useful if you want to allow navigation to be rebound to other buttons. You likely want two different sets of keys to be passed in, one to navigate up the list, one to navigate down. This is probably the pattern you'd want to follow:

switch (key.Key) {
    case var consoleKey when someSetOfNavKeys.Contains(consoleKey):
        // logic and stuff
        break;
}

switch (key.Key) {
    case var consoleKey when someSetOfNavKeys.Contains(consoleKey):
        // logic and stuff
        break;
}

You have two sets of switch cases that both do the exact same thing. These go up the list: https://github.com/JsPeanut/InteractiveMenu/blob/ebe6b0f259b3fcf36ec4f9c1d88aa7d9f703733f/TestingArea/TestingArea/Program.cs#L55 https://github.com/JsPeanut/InteractiveMenu/blob/ebe6b0f259b3fcf36ec4f9c1d88aa7d9f703733f/TestingArea/TestingArea/Program.cs#L61 These go down the list: https://github.com/JsPeanut/InteractiveMenu/blob/ebe6b0f259b3fcf36ec4f9c1d88aa7d9f703733f/TestingArea/TestingArea/Program.cs#L58 https://github.com/JsPeanut/InteractiveMenu/blob/ebe6b0f259b3fcf36ec4f9c1d88aa7d9f703733f/TestingArea/TestingArea/Program.cs#L64 You only need one of each, there's no point in having that logic duplicated. https://github.com/JsPeanut/InteractiveMenu/blob/ebe6b0f259b3fcf36ec4f9c1d88aa7d9f703733f/TestingArea/TestingArea/Program.cs#L37 Instead of checking for string.IsNullOrWhiteSpace(...) every time you write something to the console, you could instead filter all invalid options when you begin ShowMenu. Doing this with linq would be easy - opts.Where(opt => <check to make sure the opt has text>) https://github.com/JsPeanut/InteractiveMenu/blob/ebe6b0f259b3fcf36ec4f9c1d88aa7d9f703733f/TestingArea/TestingArea/Program.cs#L39 I'd suggest creating an enum that allows you to specify the orientation of the list. That way you can control orientation independently from the keys used to navigate.

GitHub

InteractiveMenu/Program.cs at ebe6b0f259b3fcf36ec4f9c1d88aa7d9f7037...

Interactive menu. Contribute to JsPeanut/InteractiveMenu development by creating an account on GitHub.

XDOP•2y ago

Thank you really much man, could you specify on this though? I don't get it

I'd suggest creating an enum that allows you to specify the orientation of the list. That way you can control orientation independently from the keys used to navigate.

Keswiik•2y ago

Instead of checking for the type of keys you've passed in (left/right/up/down), you could create an enum with different display styles for your prompt, something like:

public enum MenuDirection {
    Horizontal,
    Vertical
}

public enum MenuDirection {
    Horizontal,
    Vertical
}

You could then pass this into your ShowMenu method and use it to change how you write to the console.

if (direction is MenuDirection.Horizontal) {
     //do something
}

if (direction is MenuDirection.Horizontal) {
     //do something
}

Keswiik•2y ago

It would replace the logic you have in this loop here: https://github.com/JsPeanut/InteractiveMenu/blob/ebe6b0f259b3fcf36ec4f9c1d88aa7d9f703733f/TestingArea/TestingArea/Program.cs#L39

GitHub

InteractiveMenu/Program.cs at ebe6b0f259b3fcf36ec4f9c1d88aa7d9f7037...

Interactive menu. Contribute to JsPeanut/InteractiveMenu development by creating an account on GitHub.

XDOP•2y ago

thank you man, now I got it.

Accord•2y ago

Was this issue resolved? If so, run /close - otherwise I will mark this as stale and this post will be archived until there is new activity.

Gaming

Programming

❔ Any tips on how to make this function cleaner? I feel it's a mess