Version: 0.1
- Introduction
- Access Modifiers
- Naming Conventions
- Comments and Documentation
- General Code Quality
- Best Practices
This is a coding standard and best practices guide for C# we should use in AdGuard projects.
Based on: AdGuard Java Guidelines
Access level modifiers should be explicitly defined when it's possible. Examples:
// Bad.
class mountainBike // implicit internal
{
object Frame { get; set; } // implicit private
object[] Wheels { get; set; } // implicit private
}
// Good.
private class MountainBike
{
private object Frame { get; set; }
private object[] Wheels { get; set; }
}General Guidelines
- All names should be meaningful.
- Use whole words and avoid acronyms and abbreviations.
Classes
- Class names should be nouns in PascalCase.
- Multiple classes in the single file are allowed, but when it really makes sense.
Examples:
// Bad.
private class mountainBike
{
}
// Good.
private class MountainBike
{
}Interfaces
- Interface names should be nouns in PascalCase.
- Interfaces should be prefaced with the letter I. Examples:
// Bad.
public interface Bike
{
}
// Good.
public interface IBike
{
}Namespaces, Enums, Structs, Delegates, Events, Methods and Properties
- PascalCase should be used
Variables and Constants general guidelines
- Variable names should be short yet meaningful.
- Should not start with underscore(_) or dollar sign $ characters.
- Should be mnemonic i.e, designed to indicate to the casual observer the intent of its use.
- One-character variable names should be avoided except for temporary variables.
- Magic-numbers (hard-coded numbers) shouldn't be used
- Include units in variable names
Examples:
private void DoSomething()
{
// Bad.
long pollInterval;
int fileSize;
}
private void DoSomething()
{
// Good.
long pollIntervalMs;
int fileSizeGb.
}Constants
- Should be all uppercase with words separated by underscores (_).
Examples:
// Bad.
public const int TimeInSeconds = 5;
//Good.
public const int TIME_IN_SECONDS = 5;Private variables
- Should be written in PascalCase.
- Should be prefixed with the m_
Examples:
// Bad.
int fileSize;
// Good.
int m_FileSizeGb.Local variables
- Should be written in lowerCamelCase.
- Using the implicit typing for the local variables declaration is encouraged.
Examples:
private void DoSomething()
{
// Bad.
long PollInterval = Convert.ToInt32(Console.ReadLine());;
}
private void DoSomething()
{
// Good.
long pollIntervalMs = Convert.ToInt32(Console.ReadLine());
}Generics
-
Generic Version: If you have a generic only version of a class or interface, name the file with the format
Something.cs. -
Both generic and non generic versions: If you have both generic and non generic versions of a class or interface use
Something.csfor non generic version file andSomething(T).csfor generic version file.
Have you ever heard that "good code is supposed to be self-explanatory"? I'd love to find the author of this statement and tell him everything I think about it. This guy is responsible for thousands of unmaintainable projects because devs are lazy by nature and use it as excuse whenever it's possible.
The problem is rather obvious: self-explanatory code only tell how it is working. It rarely tells how it should work. That's why we have some strict rules regarding code documentation and comments.
The more visible a piece of code is (and by extension - the farther away consumers might be), the more documentation is needed.
- Use XMLDoc-style comments.
- Every public class or method must have a comment.
- Don't document overriding methods unless you want to tell what's different in its behavior.
- Documentation text should be written using complete sentences, full stops are not necessary.
Documentation for a class may range from a single sentence to paragraphs with code examples. Documentation should serve to disambiguate any conceptual blanks in the API, and make it easier to quickly and correctly use your API. A thorough class doc usually has a one sentence summary and, if necessary, a more detailed explanation.
/// <summary>
/// Implements a variable-size List that uses an array of objects to store the
/// elements. A List has a capacity, which is the allocated length
/// of the internal array. As elements are added to a List, the capacity
/// of the List is automatically increased as required by reallocating the
/// internal array.
/// </summary>
public class List<T> : IList<T>, System.Collections.IList, IReadOnlyList<T>
{
...
}A method doc should tell what the method does and what exceptions can be thrown by the method. Depending on the argument types, it may also be important to document input format.
/// <summary>
/// Adds two doubles and returns the result.
/// </summary>
/// <returns>
/// The sum of two doubles.
/// </returns>
/// <exception cref="System.OverflowException">Thrown when one parameter is max
/// and the other is greater than zero.</exception>
/// See <see cref="Math.Add(int, int)"/> to add integers.
/// <param name="a">A double precision number.</param>
/// <param name="b">A double precision number.</param>
public static double Add(double a, double b)
{
...
}A constructor doc should tell the specific information about usage and parameters, if there is some not obvious behaviour.
/// <summary>
/// Initializes a new instance of <see cref="SomeTransientObject"/> with the concatentaion of passed values: *explaination if required*
/// </summary>
/// <param name="value1">The unique value 1.</param>
/// <param name="value2">The unique value 2</param>
/// <exception cref="ArgumentException">Thrown, if values are equal.</exception>
public SomeTransientObject(
Value1 value1,
Value2 value2)
{
// thrown exceptions must be documented.
if (value1.Equals(value2){
throw ArguementException(nameof(value1));
})
// not obvious behaviour must be documented.
Value = value1.Append(value2);
}However, even if the constructor contains only transparent initialization code, we should leave a minimal boilerplate comment, e.g. :
/// <summary>
/// Initializes a new instance of <see cref="SomeTransientObject"/>
/// </summary>
/// <param name="value1">The description of value1</param>
/// <param name="value2">The description of value2</param>
public SomeTransientObject(
Value1 value1,
Value2 value2)
{
Value1 = value1;
Value2 = value2;
}There is an exclusion for singleton object constructors, that are not supposed to have any explicit usages and require only injected types as parameters (e.g. created only using IoC, or reflection). For such objects we don't describe parameters, because it clutters up the code and causes additional merge conflicts, while it does not contain any useful information:
// bad
/// <summary>
/// Initializes an instance of <see cref="SomeService"/>
/// </summary>
/// <param name="niceService">The nice service</param> // this information is useless.
/// <param name="goodService">The good service</param>
/// <param name="smartService">The smart service</param>
/// <param name="nService">The N service</param>
public SomeService(
INiceService niceService,
IGoodService goodService,
ISmartService smartService,
INService nService)
{
...
}
// good
/// <summary>
/// Initializes an instance of <see cref="SomeService"/>
/// </summary>
public SomeService(
INiceService niceService,
IGoodService goodService,
ISmartService smartService,
INService nService)
{
...
}Inline comments should always be added when the intent or purpose of any code isn't completely explicit, but the code itself ought to be clear enough to follow logically.
private void EnsureCapacity(int min)
{
if (m_Items.Length < min)
{
long newCapacity = m_Items.Length == 0 ? m_DefaultCapacity : m_Items.Length * 2;
// Allow the list to grow to maximum possible capacity (~2G elements) before encountering overflow.
// Note that this check works even when m_Items.Length overflowed thanks to the (uint) cast
if ((uint)newCapacity > Array.MaxArrayLength) newCapacity = Array.MaxArrayLength;
if (newCapacity < min) newCapacity = min;
Capacity = newCapacity;
}
}Single responsibility principle
Every module, class or method should have responsibility over a single part of the functionality provided by the software. For the more detailed description of the Single Responsibility Principle from Robert C. Martin follow the link.
Line length
Try to limit line length. This limit can be arbitrary (e.g. 80 or 100 characters) and not rigidly enforced, but the goal is to reduce the amount of horizontal scrolling for the developer.
Method and block length
Try to limit the length of method and code blocks by 50 lines so that they are not trying to do too much. Shorter methods are easier to test, and smaller sections of code are more quickly comprehended by developers.
Never use Extension Methods
All the additional logic that is necessary is implemented through utility classes
Brackets and blocks
Open braces should always be at the beginning of the line after the statement that begins the block.
// bad
private void DoSomething() {
...
}
// good
private void DoSomething()
{
...
}An empty line should be used after block of code in the methods.
// bad
private void DoSomething()
{
if (foobar)
{
...
}
DoAnotherAction();
}
// good
private void DoSomething()
{
if (foobar)
{
...
}
DoAnotherAction();
}Always use brackets when creating code blocks of any kind. Every block, even if it is only one line, needs to have its own curly braces in order to avoid confusion and prevent the possibility of hard to track bugs.
// bad
if (foobar) DoSomething();
// good
if (foobar)
{
DoSomething();
}Explicit type
Please use explicit type instead of var in all cases (may be except creating a new object with new keyword). It makes code cleaner and more understandable (especially while reading pull requests)
// bad
var foo = GetSomething();
// good
FooDto foo = GetSomething();Ternary operators
Ternary operators are fine for clear-cut conditionals, but unacceptable for confusing choices.
// bad
int value = a && b ? 11 : a ? 10 : b ? 1 : 0;
// good
int value = isSimple ? 11 : 1;Ternary expressions should never be nested because they just add to the confusion.
Expression-bodied members
Don't use Expression-bodied members in all over the cases where it can be used. Despite of the fact c# supports such "sugar" and it looks more compactable, it makes code more complicated to read
// bad
public bool Foo() => false;
// good
public bool Foo()
{
return false;
}Centralize duplicate logic in utility functions
Using the dynamic keyword allowed only in tests
Use a StringBuilder instead of string if multiple concatenations are required
Override ToString method for the types which you want to provide with custom information.
Avoid straightaway copy/pasting of code from other sources. It is always recommended to hand write the code.
Usage of 'out' and 'ref' keywords be avoided as recommended by Microsoft (in the Code analysis Rules and guidelines)
In a class API, you should support access to any methods and fields that you make accessible. Therefore, only expose what you intend the caller to use. This can be imperative when writing thread-safe code.
public class Parser
{
// Bad.
// - Callers can directly access and mutate, possibly breaking internal assumptions.
public Dictionary<String, String> RawFields;
// Bad.
// - This is probably intended to be an internal utility function.
public String ReadConfigLine()
{
..
}
}
// Good.
// - rawFields and the utility function are hidden
// - The class is package-private, indicating that it should only be accessed indirectly.
class Parser
{
private Dictionary<String, String> m_RawFields;
private string ReadConfigLine()
{
..
}
}Catch narrow exceptions
Sometimes when using try/catch blocks, it may be tempting to just catch Exception, so you don't have to worry about what type was thrown. This is usually a bad idea, as you can end up catching more than you really wanted to deal with. For example, catch Exception would capture NullReferenceException, and would capture OutOfMemoryException.
// Bad.
// - If a OutOfMemoryException happens, the program continues rather than aborting.
try
{
storage.InsertUser(user);
}
catch (Exception ex)
{
LOG.Error("Failed to insert user.");
}
try
{
storage.InsertUser(user);
}
catch (StorageException ex)
{
LOG.Error("Failed to insert user.");
}Don't swallow exceptions
An empty catch block is usually a bad idea, as you have no signal of a problem. Coupled with narrow exception violations, it's a recipe for disaster.
Throw appropriate exception types
Let your API users obey the "catch narrow exceptions" rule, don't throw Exception. Even if you are calling another naughty API that throws Exception, at least hide that so it doesn't bubble up even further. You should also make an effort to hide implementation details from your callers when it comes to exceptions. Also, don't forget to add the information about exceptions to the method's documentation, as it was described in Documenting a Method.
// Bad.
// - Caller is forced to catch Exception, trapping many unnecessary types of issues.
public class DataStore
{
public string FetchValue(string key)
{
...
throw new Exception("error message");
}
}
// Better.
// - The interface leaks details about one specific implementation.
public DataStore
{
public string FetchValue(string key)
{
...
throw new SQLException("error message");
}
}
// Good.
// - A custom exception type insulates the user from the implementation.
// - Different implementations aren't forced to abuse irrelevant exception types.
public DataStore
{
public string FetchValue(string key)
{
...
throw new StorageException("error message");
}
public class StorageException : Exception
{
...
}
}Premature optimization is the root of all evil.
Donald Knuth is a smart guy, and he had a few things to say on the topic.
Unless you have strong evidence that an optimization is necessary, it's usually best to implement the un-optimized version first (possibly leaving notes about where optimizations could be made).
So before you spend a week writing your memory-mapped compressed huffman-encoded hashmap, use the stock stuff first and measure.
Reason behind this is, it makes your code properly encapsulated in OOPs environment. By using getters & setters, you can restrict the user directly accessing the member variables. You can restrict setting the values explicitly thus making your data protected from accidental changes. Also, properties give you easier validation for your data.
Use IDisposable interface to free all the resources from the memory. Once you implement IDisposable interface in your class, you will get a Dispose() method there. Write code there to free the resources. If you implement IDisposable, you can initialize your class like this:
using (PersonDataSource personDataSource = DataSource.GetPerson())
{
// write your code here
}After the using() {} block, it will call the Dispose() method automatically to free up the class resources. You will not have to call the Dispose() explicitly for the class.
It is better to use “is” and “as” operator while casting. Instead of Explicit casting, use the Implicit casting.
As the name suggests, these are purely informational messages. To use this log level effectively, try to think about what general information would be useful for diagnosing an application error or oddities.
Should be used to indicate that the application faced a potential problem; however, the user experience has not been affected in any way. In this case, it is acceptable to swallow an exception in the catch block.
Should be used to indicate that the application faced a significant problem and that, as a result, the user experience was affected in some way. In this case, it is necessary to, optionally, modify exception and throw it again.
Information that is diagnostically helpful to the developer.
Very detailed information used only in specific cases to find out the reasons of much-complicated issues