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;
}
}