A collection of mostly reasonable guidelines and best practices to write clean, readable, understandable and maintainable C# code.
var whenever appropriate and possiblePurpose: Better readability and cleanliness
GOOD
var httpClient = new HttpClient();BAD
HttpClient httpClient = new HttpClient();Purpose: Better readability and cleanliness
GOOD
var user = new User
{
Username = "admin",
Age = 31
};BAD
var user = new User();
user.Username = "admin";
user.Age = 31;string.Format
Purpose: Better readability and semantics
GOOD
var url = "http://localhost/api";
var resource = "users";
var path = $"{url}/{resource}";BAD
var url = "http://localhost/api";
var resource = "users";
var path = string.Format("{0}/{1}", url, resource);Purpose: No need to escape backslash characters
GOOD
var path = @"C:UsersAdministratorDocuments";BAD
var path = "C:\Users\Administrator\Documents";Purpose: Refactoring can become a nightmare when string literals are distributed all over the place multiple times
GOOD
const string error = "user_not_found";
Log.Error(error);
return BadRequest(error);BAD
Log.Error("user_not_found");
return BadRequest("user_not_found");xyz. It's just ridiculous.Purpose: Shortening variable names doesn't add any value, it even makes code way harder to read and understand
GOOD
// Nice
var validationResult = validator.Validate();
// Nice
var stringBuilder = new StringBuilder();
// Nice
const string directorySeparator = "/";BAD
//
var res = validator.Validate();
//
var sbd = new StringBuilder();
// Seriously?
const string dsep = "/";Purpose: Makes the code tremendously easier to read, maintain and work with
GOOD
// The purpose of this class can be easily inferred
public class OrderManager
{
// Using "Is" or "Has" as prefix clearly indicates that a method returns a boolean value
public bool IsFulfilled(Order order)
{
}
public bool HasPositions(Order order)
{
}
// Using a verb clearly indicates that a method performs some action
public void ProcessOrder(Order order)
{
}
public void CancelOrder(Order order)
{
}
}BAD
// Purpose of this class can not be easily inferred
public class OrderHelper
{
// Unclear
public bool Fulfilled(Order order)
{
}
// Unclear => users would likely expect a method that retrieves the positions of the order due to the verb "Get"
public bool GetPositions(Order order)
{
}
// Unclear
public void Order(Order order)
{
}
// Unclear
public void StopOrder(Order order)
{
}
}Purpose: Makes the code unnecessarily longer and harder to read and understand
GOOD
// Clearly an interface
public interface IOrderManager { }
// Clearly a list of orders
private IList<Order> orders;BAD
// Clearly an interface => no "Interface" postfix necessary
public interface IOrderManagerInterface { }
// Clearly a list of oders => no "List" postfix necessary
private IList<Order> orderList;Purpose: The practice shows that comments can become out of date very easily and quickly as the code base grows and evolves. Having a self-explanatory code base contributes to better maintainability and reduces the need to write and maintain unncessary comments. ( This doesn't mean that writing comments is obsolete.)
GOOD
public class OrderManager
{
public void ProcessOrder(Order order)
{
var items = order.GetItems();
foreach (var item in items)
{
var availability = item.GetAvailability();
}
}
}BAD
public class OrderManager
{
public void ExecuteOrder(Order order)
{
// Get all available items from the order
var data = order.GetData();
foreach (var x in data)
{
// Determine the availability of the item
var available = item.CheckAvailability();
}
}
}Purpose: Consistent with the .NET Framework and easier to read
GOOD
public class JsonParser { }BAD
public class JSONParser { }Purpose: enums are always addressed individually. For example EmploymentType.FullTime is much cleaner than EmploymentTypes.FullTime. Furthermore, classes always represent a single instance of an object, for example an individual User. Exception: bit field enums
GOOD
public enum EmploymentType
{
FullTime,
PartTime
}
public class User
{
}BAD
public enum EmploymentTypes
{
FullTime,
PartTime
}
public class Users
{
}Purpose: Syntactic sugar ¯(ツ)/¯
GOOD
public class User
{
public string Username { get; }
public int Age { get; }
public string DisplayName => $"{Username} ({Age})";
public User(string username, int age)
{
Username = username;
Age = age;
}
}BAD
public class User
{
public string Username { get; }
public int Age { get; }
public string DisplayName
{
get
{
return $"{Username} ({Age})";
}
}
public User(string username, int age)
{
Username = username;
Age = age;
}
}Purpose: Better awareness of what went wrong. Allows more precision during logging and error analysis.
GOOD
try
{
System.IO.File.Open(path);
}
catch (FileNotFoundException ex)
{
// Specific
}
catch (IOException ex)
{
// Specific
}
catch (Exception ex)
{
// Default
}BAD
try
{
System.IO.File.Open(path);
}
catch (Exception ex)
{
// SOMETHING went wrong
}Exception yourselfPurpose: Very bad practice. Exceptions should only be thrown by the .NET Framework.
GOOD
public void ProcessOrder(Order order)
{
if (order == null)
{
throw new ArgumentNullException(nameof(order));
}
}BAD
public void ProcessOrder(Order order)
{
if (order == null)
{
throw new Exception("Order is null.");
}
}Purpose: Better readability
GOOD
private string _username;BAD
private string mUsername__;
// OR
private string username;
// OR
private string username_;Purpose: Better readability, saves space
GOOD
public class JsonParser
{
private readonly string _json;
public JsonParser(string json)
{
_json = json;
}
}BAD
public class JsonParser
{
private readonly string _json;
public JsonParser(string json)
{
_json = json;
}
}Purpose: Prevents property from accidentally being changed from elsewhere, better prediction of the application's behavior
GOOD
public class JsonParser
{
public string Json { get; }
public JsonParser(string json)
{
Json = json;
}
}BAD
public class JsonParser
{
public string Json { get; set; }
public JsonParser(string json)
{
Json = json;
}
}Purpose: Prevents collection from being changed from elsewhere, better prediction of the application's behavior
GOOD
public class KeywordProvider
{
public IReadOnlyCollection<string> Keywords { get; }
public KeywordProvider()
{
Keywords = new ReadOnlyCollection<string>(new List<string>
{
"public",
"string"
});
}
}BAD
public class KeywordProvider
{
public IList<string> Keywords { get; }
public KeywordProvider()
{
Keywords = new List<string>
{
"public",
"string"
};
}
}using blocksPurpose: Resource is automatically disposed when the using block finishes
GOOD
using (var connection = new SqlConnection(connectionString))
{
}BAD
try
{
var connection = new SqlConnection(connectionString);
}
finally
{
connection.Close();
connection.Dispose();
}Purpose: Better readability, easier maintenance when another line is required inside the condition
GOOD
if (user.IsElevated)
{
// Do something
}BAD
if (user.IsElevated)
// Do somethingPurpose: So that you're aware of which part of the message contains variables.
GOOD
var filePath = @"C:tmpmy-file.txt";
try
{
var file = File.Open(filePath);
}
catch (Exception ex)
{
// GOOD: Add [ ] to the variable
Log.Error($"Could not open file [{filePath}].", ex);
}BAD
var filePath = @"C:tmpmy-file.txt";
try
{
var file = File.Open(filePath);
}
catch (Exception ex)
{
Log.Error($"Could not open file {filePath}.", ex);
}Purpose: So that the exception can be filtered in certain cases (for example in the user interface, since the entire stack trace is useless for the user).
GOOD
try
{
var file = File.Open(@"C:tmpmy-file.txt");
}
catch (Exception ex)
{
// Use appropriate signature of your logger to include the exception as separate parameter
Log.Error("Could not open file.", ex);
}BAD
try
{
var file = File.Open(@"C:tmpmy-file.txt");
}
catch (Exception ex)
{
// Strictly AVOID this. The exception is added directly to the log message, which makes it impossible to filter the exception
Log.Error($"Could not open file: {ex}");
}