Return to Answer

• 1

• 12.6k
• 3
• 48
• 99

##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.