Console autocomplete for complex names

Question

I am currently writing a C# console application. Part of it is, that the user needs to enter a fairly complex system name. To make it easier I wrote a function that uses a string[] keywords and auto completes the string the user is typing in - on the run (which makes it different to this post in my opinion)

The code is working and acts as expected, but I am curious how the code could be improved (e.g. usability, efficiency). Also, what functionalities are missing, which you'd expect?

Thank you for the feedback!

 class Program
 {
 static void Main(string[] args)
 {
 string[] Keywords = new string[4] { "aX1223", "aE_334", "L_test1", "L_test2" };
 if (Keywords.Length == 0)
 throw new ArgumentException(nameof(Keywords), "No Keywords set!");
 bool searching = true; // true while looking for the keyword
 Console.CursorVisible = true; // To help users understand where they are typing
 string userInput = ""; // Initialization of output
 string suggestion = Keywords[0]; // Initialization of default suggestion
 int toClear = suggestion.Length; // Get the length of the line that needs to be cleared
 while (searching)
 {
 Console.Write(new String(' ', toClear)); // Clear line
 Console.CursorLeft = 0; // Relocate cursor to line start
 Console.Write(userInput); // Write what has been written previously
 if (suggestion != "") // If there is a suggestion fitting the entered string,
 { // complete the string in color and reset the cursor
 int col = Console.CursorLeft;
 Console.ForegroundColor = ConsoleColor.Magenta;
 Console.Write(suggestion.Substring(userInput.Length));
 Console.ForegroundColor = ConsoleColor.White;
 Console.CursorLeft = col;
 }
 string tempInput = Console.ReadKey().KeyChar.ToString();
 if (tempInput.Equals("\r")) // Evaluate input:
 { // -> Enter
 if (!suggestion.Equals("")) // Valid entry?
 {
 searching = false;
 userInput = suggestion; // -> use suggestion
 }
 }
 else if (tempInput.Equals("\b")) // Deleting last sign
 {
 if (userInput.Length > 0)
 userInput = userInput.Substring(0, userInput.Length - 1);
 }
 else // Any other sign is added to the string
 userInput += tempInput;
 // Set length to clear, if suggestion == "", the system string length needs to be cleared
 toClear = (userInput.Length > suggestion.Length) ? userInput.Length : suggestion.Length;
 // Reset suggestion. If no match is found, suggestion remains empty
 suggestion = "";
 // Check keywords for suggestion
 for (int i = 0; i < Keywords.Length; i++)
 {
 if (Keywords[i].StartsWith(userInput))
 {
 suggestion = Keywords[i];
 break;
 }
 }
 }
 // reset cursor visibility
 Console.CursorVisible = false;
 }
 }

• sineemore: Crosspost on Stack Overflow

Answer 1

A few minor comments:

• Don't mix equality types.

  You use suggestion != "" in one place, then !suggestion.Equals(""). Don't. Use the same type of equality (either using .Equals or not using .Equals) everywhere.

• Only use non-braced statements in the case of guard clauses, and even then be careful.

  Basically, this is OK:

  if (Keywords.Length == 0)
   throw new ArgumentException(nameof(Keywords), "No Keywords set!");
  

  This is not:

  if (userInput.Length > 0)
   userInput = userInput.Substring(0, userInput.Length - 1);
  

• Try to use built-ins (especially System.Math) when possible.

  I.e. take toClear = (userInput.Length > suggestion.Length) ? userInput.Length : suggestion.Length; and convert it to toClear = Math.Max(userInput.Length, suggestion.Length). They're functionally equivalent and the Math.Max reads nicer.

The rest of it looks good, I think you did a pretty good job implementing the idea here. I might suggest building functions out, but this is a small enough program it's probably not necessary.

Answer 2

Comments

There's a common misconception about comments. You should not write comments to explain what something does. Your code should be clear enough so that does comments are not necessary. Sadly, there are times where we need to write complex code that is hard to understand. In this very case, you are "allowed" to write comments to explain :

1- Why you did what you did. Example :

FooBar was super complex because of Bar and Foo dependency, so I had to write this like this because of that.

This comment gives insight on what happened in your head while you were writing the code. So if someone ever comes up on the FooBar complex method, they'll be able to use your comment to understand what needs to be considered.

2- Explain what the code does.

FooBar does something by using [Insert shady algorithm] to have maximum performance.

A good rule of thumb would be to ask yourself "If I came back to my code in 2 weeks, is there something I might have trouble understanding in my code?". If the answer is yes, try to refactor that thing. If the answer is still yes, add a comment.

So, in my opinion, most, if not all, your comments could be deleted.

Coding style

Brackets

I'll use this piece of code as an example :

else if (tempInput.Equals("\b"))
{
 if (userInput.Length > 0)
 userInput = userInput.Substring(0, userInput.Length - 1);
}
else
 userInput += tempInput;

The parenthesis on the else if aren't even necessary, yet you use them, but with the else, you don't. This lack of consistency makes the code harder to read and adds bugs potential. Since the Apple bracket's fail, I always use brackets around my flow operators (for, if, etc..). You should too!

Constants vs local variables

Keywords could be a const. That's that. :p

Application flow

Method extraction

There is a lot of different flows in your code (lots of ifs, while, for, etc), it would help if some parts were separated in different methods. For example :

private string GetSuggestionFromKeywords(string userInput)
{
 for (int i = 0; i < Keywords.Length; i++)
 {
 if (Keywords[i].StartsWith(userInput))
 {
 return Keywords[i];
 }
 }
}

This method is small, super easy to understand and as a bonus you get a method name which "acts as a comment" on what the inside of the method does.

Tip : Try to figure out what other pieces of code could be extracted to methods, it's a very good exercise to help you develop (pun intended) as a better developer.

Conditions

else if (tempInput.Equals("\b"))
{
 if (userInput.Length > 0)
 userInput = userInput.Substring(0, userInput.Length - 1);
}

So, what if tempInput.Equals("\b") == true but userInput.Length > 0 == false? Nothing happens right? In this case, why not use :

else if (tempInput.Equals("\b") && userInput.Length > 0)
{
 userInput = userInput.Substring(0, userInput.Length - 1);
}

That's one if less. Try to reduce nesting as much as possible as it cripples readability.

Tip : Once again, try to find every way you can to reduce nesting, this is another good exercise which will make you a better developer.

Exceptions

if (Keywords.Length == 0)
 throw new ArgumentException(nameof(Keywords), "No Keywords set!");

Now, you're the one who sets the Keywords and they are not modifiable by anyone, ever, except you. This means that the only way this exception would be thrown is if you forgot to write the line just above, which would be hard, since you wrote it already.

Considering this, you should think about removing this exception.

Regarding my "tips". Extracting methods, reducing nesting etc. should never be at the detriment of code readability. If extracting a method makes your code uglier, don't do it. But think about ways to do it to make your code more readable.