First let me share with you the revised version of your code.
Please note that this version contains a bug.
The fixed version can be found under the update #2 section.
Then I will guide you through the changes.
static readonly string[] Cities = new []
{
"New York",
"London",
"Mumbai",
"Chicago"
};
static void Main()
{
int selectedLineIndex = 0;
ConsoleKey pressedKey;
do
{
UpdateMenu(selectedLineIndex);
pressedKey = Console.ReadKey().Key;
if (pressedKey == ConsoleKey.DownArrow && selectedLineIndex + 1 < Cities.Length)
selectedLineIndex++;
else if (pressedKey == ConsoleKey.UpArrow && selectedLineIndex - 1 >= 0)
selectedLineIndex--;
} while (pressedKey != ConsoleKey.Enter);
Console.WriteLine($"{Cities[selectedLineIndex]} was chosen as an option");
}
static void UpdateMenu(int index)
{
Console.Clear();
foreach (var city in Cities)
{
bool isSelected = city == Cities[index];
ChangeLineColor(isSelected);
Console.WriteLine($"{(isSelected ? "> " : " ")}{city}");
}
}
static void ChangeLineColor(bool shouldHighlight)
{
Console.BackgroundColor = shouldHighlight ? ConsoleColor.White : ConsoleColor.Black;
Console.ForegroundColor = shouldHighlight ? ConsoleColor.Black : ConsoleColor.White;
}
Cities
- It seems like this collection is fairly static, so you can define it as a class-level static field
- In order to emphasize that the collection should not be changed during runtime you can mark the field as
readonly
- I would also suggest to use a simple array rather than a
List for this simple use case
Index and KeyInfo
- I would suggest to aim for more meaningful names
- I've used
selectedLineIndex and pressedKey to express the intent of the variables
- I've also changed the data type of
keyinfo from ConsoleKeyInfo to ConsoleKey
The infinite loop
- You have a single exit condition when the user has pressed Enter
- So, we could use that in a
do-while construction
Nested if statements
- Your nested (two levels) if statements can be combined by using the
&& operator
- With this change the conditions become a bit more complex
- but the your intention will remain clear
Write Cities
- Since we have moved the
cities collection to class level that's why we don't need to pass it as a parameter
- I've also changed the name of the method to
UpdateMenu to express the intent
- I've declared the
UpdateMenu as a static method outside of Main
- Please note that in case of C# the parameter's naming convention is camel casing
- I've refactored your code a bit to streamline your logic
Changing Colors
- I've extract your line color changing logic into its own method to reduce code duplication
- I've used conditional ternary operators to choose the right colors
UPDATE #1
I just noticed that there is some problem with this code. When you press down and the last city is already selected the background changes to white and the city black
According to my understanding you were referring to this bug
The root cause of the problem is that the UpdateMenu is called unconditionally inside the do-while loop. It should be called only if the selection is changed.
One way to fix this problem is to store the previously selected line index and the current index. The update of the menu will be called only if the numbers differ.
static void Main()
{
int previousLineIndex = -1 , selectedLineIndex = 0;
ConsoleKey pressedKey;
do
{
if(previousLineIndex != selectedLineIndex)
{
UpdateMenu(selectedLineIndex);
previousLineIndex = selectedLineIndex;
}
pressedKey = Console.ReadKey().Key;
...
} while (pressedKey != ConsoleKey.Enter);
...
}
UPDATE #2
Yes that is the bug a was talking about. However it still has this problem when you are on the last element and you go up the list.
This strange background colouring issue drive my attention to the ChangeLineColor method. After re-reading the code I've just realised that we don't need to change all the time the Background and Foreground colors. It is enough to do that only once when we draw the selected menu:
- Change colors to the inverted ones
- Print the selected line
- Change colors to the original ones
After the above code change the code looks like this
static readonly string[] Cities = new []
{
"New York",
"London",
"Mumbai",
"Chicago"
};
static void Main()
{
int previousLineIndex = -1 , selectedLineIndex = 0;
ConsoleKey pressedKey;
do
{
if(previousLineIndex != selectedLineIndex)
{
UpdateMenu(selectedLineIndex);
previousLineIndex = selectedLineIndex;
}
pressedKey = Console.ReadKey().Key;
if (pressedKey == ConsoleKey.DownArrow && selectedLineIndex + 1 < Cities.Length)
selectedLineIndex++;
else if (pressedKey == ConsoleKey.UpArrow && selectedLineIndex - 1 >= 0)
selectedLineIndex--;
} while (pressedKey != ConsoleKey.Enter);
Console.WriteLine($"{Cities[selectedLineIndex]} was chosen as an option");
}
static void UpdateMenu(int index)
{
Console.Clear();
foreach (var city in Cities)
{
bool isSelected = city == Cities[index];
if (isSelected)
DrawSelectedMenu(city);
else
Console.WriteLine($" {city}");
}
}
static void DrawSelectedMenu(string city)
{
Console.BackgroundColor = ConsoleColor.White;
Console.ForegroundColor = ConsoleColor.Black;
Console.WriteLine($"> {city}");
Console.BackgroundColor = ConsoleColor.Black;
Console.ForegroundColor = ConsoleColor.White;
}
And gladly the bug is gone
