Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

This question is intended to be used for linking to common issues, as discussed on meta. Feel free to add and modify any answer on this post, it's Community Wiki!


If you've posted several answers on this site, you might have found that there are a number of coding issues that many programmers are guilty of, and you keep rephrasing the same recommendations over and over, only with different variable names.

Questions tagged with aim at putting together a collection of best practices in the form of canonical answers to common coding issues, which can later be linked to in actual code reviews.


Sharing an answer

Every post has a [share] button. Simply click it, copy the URL, and paste it in your answer:

share-link

share|improve this question
3  
shouldn't this be a meta question? –  Malachi Mar 20 at 15:47
7  
I think that the main site is the right place for this, because the question is not actually about the Code Review site. –  svick Mar 20 at 17:04
add comment

put on hold as off-topic by MrSmith42, rolfl, 200_success, Simon André Forsberg, Jerry Coffin yesterday

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Questions must include the code you want reviewed. Code must not appear only in a link to an external source. Doing so makes us dependent on a third-party site and makes it harder to review your code. If your code is very large, please select only the portions in which you are especially interested for a review. You are welcome to keep the link to the rest of your code." – MrSmith42, rolfl, 200_success, Simon André Forsberg, Jerry Coffin
If this question can be reworded to fit the rules in the help center, please edit the question.

2 Answers

Naming

Let's take this code as an example:

public interface IUserMessenger
{
    void showmessage(string strMsg);
}

public interface IUserInputProvider : IUserMessenger
{
    string GetUserInput(string strMsg);
    T GetUserInput<T>(string strMsg, IUserInputValidator<T> v);
}

public class ConsoleUserInputProvider : IUserInputProvider
{
    public void showmessage(string strMsg)
    {
        Console.WriteLine(strMsg);
    }

    public string GetUserInput(string strMsg)
    {
        showmessage(strMsg);
        return Console.ReadLine();
    }

    public T GetUserInput<T>(string strMsg, IUserInputValidator<T> v)
    {
        string s;
        T input;

        if (string.IsNullOrEmpty(strMsg)) throw new Exception("Message cannot be empty!");

        var bValidated = false;
        while(!bValidated)
        {
            s = GetUserInput(strMsg);
            bValidated = v.Validate(s, out input);
        }

        return input;
    }
}

This code has a number of serious naming issues. MSDN has a thorough guideline for good naming.


Conventions

The IUserMessenger method showmessage doesn't follow the PascalCase convention for public members. It should be named ShowMessage.

Hungarian Notation & Disemvoweling

There is no need to prefix variables with anything that has the purpose of identifying the type of the variable "str" (for string) and "b" (for bool) only clutter up identifiers, get rid of them. Disemvoweling used to be a necessary evil, but in the age of IDEs, there is no need for it - keep names readable, name things with identifiers that can be pronounced; there is no reason not to rename strMsg to message.

Single-letter names

Avoid as well - unless it's something extremely common and known to any C# programmer, for example e is ok for an EventArgs-type argument in an event handler method, and i is ok for a counter in a for loop. s for a random string identifier is a bad name, there is no reason not to rename s to input and v to validator in this particular context.

Misleading names

In the body of method T GetUserInput<T>(string strMsg, IUserInputValidator<T> v), the variable T input has a misleading name. This is a clue:

bValidated = v.Validate(s, out input);

out input doesn't make any sense. s should be named input, and input should be named result. The clues aren't always as crystal-clear, but there's always a hint somewhere, that tells us the intent of a variable. The name should convey this intent.

The strMsg parameter in GetUserInput doesn't mean the same thing as the strMsg parameter in ShowMessage - good naming could make that distinction and call it a prompt when the user is expected to enter something in response, while message makes a good name for something that can be just informational.

Type parameters

Generic classes and methods often use the letter T to identity type parameters. Doing that is good, but an even better way to name those, is to start with the letter T and use a meaningful name. This way if a 2nd type parameter is ever added, you'll be tempted to also give it a meaningful name, rather than just calling it U.


With all naming issues addressed, the code would read like this:

public class ConsoleUserInputProvider : IUserInputProvider
{
    public void ShowMessage(string message)
    {
        Console.WriteLine(message);
    }

    public string GetUserInput(string prompt)
    {
        ShowMessage(prompt);
        return Console.ReadLine();
    }

    public TResult GetUserInput<TResult>(string prompt, IUserInputValidator<TResult> validator)
    {
        string input;
        TResult result;

        if (string.IsNullOrEmpty(prompt)) throw new Exception("Prompt message cannot be empty!");

        var isValid = false;
        while(!isValid)
        {
            input = GetUserInput(prompt);
            isValid = validator.Validate(input, out result);
        }

        return result;
    }
}
share|improve this answer
add comment

Throwing System.Exception

throw new Exception("Something bad has just happened.");

Don't do that.

There are really two alternatives to throwing exceptions.

One is to throw an existing exception, derived from System.Exception. This is trickier than it sounds, because you need to pick your exception carefully - you want to throw a meaningful exception.

In this case an InvalidOperationException seems in line with the Principle of Least Astonishment:

InvalidOperationException Class

The exception that is thrown when a method call is invalid for the object's current state. http://msdn.microsoft.com/en-us/library/system.invalidoperationexception(v=vs.110).aspx

There are a few exception subclasses like this one (and ArgumentException and its derivatives), that are very often the most meaningful and written-for-that-purpose exception to use.


Another way would be to define your own. That's easier that it sounds, you just derive a new class from System.Exception:

public class SomethingBadHappenedException : Exception
{
    public SomethingBadHappenedException()
        : base("Something bad has just happened.") { }
}

And now you can throw and catch a SomethingBadHappenedException.


That said...

Don't throw System.Exception.

In my quick research for why that class wasn't abstract in the first place if it wasn't intended to be instanciated and thrown, I found an interesting discussion on Programmers.SE. Emphasis mine:

when coding small demo apps or proof-of-concepts, I don't want to start designing 10 different exception sub-classes, or spending time trying to decide which is the "best" exception class for the situation. I'd rather just throw Exception and pass a string that explains the details. When it's throw-away code, I don't care about these things [...]

http://programmers.stackexchange.com/a/119683/68834

The flipside of this out-of-context quote, is that when it is quality code, you do care about these things.

share|improve this answer
add comment

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