Both good code and bad code compile. That's the first thing to understand. The next thing to understand is that bad code is bad for a reason, and likewise, good code is good for a reason. Let's have a look at some of those reasons in the following comparison table:
| Good Code | Bad Code |
| Proper indentation. | Improper indentation. |
| Meaningful comments. | Comments that state the obvious. |
| API documentation comments. | Comments that excuse bad code. Commented out lines of code. |
| Proper organization using namespaces. | An improper organization using namespaces. |
| Good naming conventions. | Bad naming conventions. |
| Classes that do one job. | Classes that do multiple jobs. |
| Methods that do one thing. | Methods that do many things. |
| Methods with less than 10 lines, and preferably no more than 4. | Methods with more than 10 lines of code. |
| Methods with no more than two parameters. | Methods with more than two parameters. |
| Proper use of exceptions. | Using exceptions to control program flow. |
| Code that is readable. | Code that is difficult to read. |
| Code that is loosely coupled. | Code that is tightly coupled. |
| High cohesion. | Low cohesion. |
| Objects are cleanly disposed of. | Objects left hanging around. |
| Avoidance of the Finalize() method. | Use of the Finalize() method. |
| The right level of abstraction. | Over-engineering. |
| Use of regions in large classes. | Lack of regions in large classes. |
| Encapsulation and information hiding. | Directly exposing information. |
| Object-oriented code. | Spaghetti code. |
| Design patterns. | Design anti-patterns. |
That's quite an exhaustive list, isn't it? In the following sections, we will look at how these features and the differences between good and bad code impact the performance of your code.
Bad code
We will now take a brief look at each of the bad coding practices that we listed earlier, detailing specificallyhow that practice affects your code.
Improper indentation
Improper indentation can work toward making code really hard to read, especially if the methods are large. For code to be easy to read by humans, we need proper indentation. If code lacks proper indentation it can be very hard to see which part of the code belongs to which block.
By default, Visual Studio 2019 correctly formats and indents your code when parentheses and braces are closed. But sometimes, it incorrectly formats the code, to bring to your attention that the code you've written contains an exception. But if you are using a simple text editor, then you will have to do your formatting by hand.
Incorrectly indented code is also time-consuming to correct, and a frustrating waste of programming time when it could easilyhave been avoided. Let's look at a simple codeexample:
public void DoSomething()
{
for (var i = 0; i < 1000; i++)
{
var productCode = $"PRC000{i}";
//...implementation
}
}
The preceding code does not look all that nice, yet it is still readable. But the more lines of code you add, the harder the code becomes to read.
It is very easy to miss a closing bracket. If your code is not properly indented, then this can make finding the missing bracket that much harder, as you can not easily spot which code block is missing its closing bracket.
Comments that state the obvious
I've seen programmers get really upset at comments that state the obvious as they find them patronizing. In programming discussions that I have been part of, programmers have stated how they dislike comments, and how they believe the code should be self-documenting.
I can understand their sentiments. If you can read code without comments like you can read a book and understand it, then it is a really good piece of code. If you have a variable declared as a string, then why add a comment such as // string? Let's look at an example:
public int _value; // This is used for storing integer values.
We know here that the value holds an integer by its type of int. So there really is no need to state the obvious. All you're doing is wasting time and energy and cluttering up the code.
Comments that excuse bad code
You may have a tight deadline to meet, but comments such as // I know this code sucks but hey at least it works! are just awful. Don't do it. It shows a lack of professionalism and can really disgruntle fellow programmers.
If you really are pushed to get something working out the door, raise a refactor ticket and add it as part of a TODO comment such as // TODO: PBI23154 Refactor Code to meet company coding practices. Then you or the other developers who are assigned to work on technical debt can pick up the Product Backlog Item (PBI) and refactor the code.
Here's another example:
...
int value = GetDataValue(); // This sometimes causes a divide by zero error. Don't know why!
...
This one is really bad. Okay, thank you for letting us know that divide-by-zero errors occur here. But have you raised a bug ticket? Have you tried to get to the bottom of it and fix it? If everybody who is actively working on the project does not touch that code, how will they know that buggy code is there?
At the very minimum, you should at least have a // TODO: comment in place. Then at least the comment will show up in the Task List so that developers can be notified and work on it.
Commented-out lines of code
If you comment out lines of code to try something, fine. But if you are going to use the replacement code instead of the commented-out code, then delete the commented-out code before you check it in. One or two commented outlines is not that bad. But when you have many lines of commented-out code, it becomes distracting and makes code hard to maintain; it can even lead to confusion:
/* No longer used as has been replaced by DoSomethinElse().
public void DoSomething()
{
// ...implementation...
}
*/
Why? Just why? If it has been replaced and is no longer needed, then just delete it. If your code is in version control, and you need to get the method back, then you can always view the history of the file and get the method back.
Improper organization of namespaces
When using namespaces, do not include code that should be elsewhere. This can make finding the right code pretty hard or impossible, especially in large code bases. Let's look at this example:
namespace MyProject.TextFileMonitor
{
+ public class Program { ... }
+ public class DateTime { ... }
+ public class FileMonitorService { ... }
+ public class Cryptography { ... }
}
We can see that all classes in the preceding code are under one namespace. Yet, we have the opportunity to add three further namespaces to better organize this code:
- MyProject.TextFileMonitor.Core: Core classes that define commonly used members will be placed here, such as our DateTime class.
- MyProject.TextFileMonitor.Services: All classes that act as a service will be placed in this namespace, such as FileMonitorService.
- MyProject.TextFileMonitor.Security: All security-related classes will be placed in this namespace, including the Cryptography class in our example.
Bad naming conventions
In the days of Visual Basic 6 programming, we used to use Hungarian Notation. I remember using it when I first switched to Visual Basic 1.0. It is no longer necessary to use Hungarian Notation. Plus, it makes your code look ugly. So instead of using names such as lblName, txtName, or btnSave, the modern way is to use NameLabel, NameTextBox, and SaveButton, respectively
The use of cryptic names and names that don't seem to match the intention of the code can make reading code rather difficult. What does ihridx mean? It means Human Resources Index and is an integer. Really! Avoid using names such as mystring, myint, and mymethod. Such names really don't serve a purpose.
Don't use underscores between words in a name either, such asBad_Programmer. This can cause visual stress for developers and can make the code hard to read. Simply remove the underscore.
Don't use the same code convention for variables at the class level and method level. This can make it difficult to establish the scope of a variable. A good convention for variable names is to use camel case for variable names such as alienSpawn, and Pascal case for method, class, struct, and interface names such as EnemySpawnGenerator.
Following the good variable name convention, you should distinguish between local variables (those contained within a constructor or method), and member variables (those placed at the top of the class outside of constructors and methods) by prefixing the member variables with an underscore. I have used this as a coding convention in the workplace, and it does work really well and programmers do seem to like this convention.
Classes that do multiple jobs
A good class should only do one job. Having a class that connects to a database, gets data, manipulates that data, loads a report, assigns the data to the report, displays the report, saves the report, prints the reports, and exports the report is doing too much. It needs to be refactored into smaller, better-organized classes. All-encompassing classes like this are a pain to read. I personally find them daunting. If you come across classes like this, organize the functionality into regions. Then move the code in those regions into new classes that perform one job.
Let's have a look at an example of a class that is doing multiple things:
public class DbAndFileManager
{
#region Database Operations
public void OpenDatabaseConnection() { throw new
NotImplementedException(); }
public void CloseDatabaseConnection() { throw new
NotImplementedException(); }
public int ExecuteSql(string sql) { throw new
NotImplementedException(); }
public SqlDataReader SelectSql(string sql) { throw new
NotImplementedException(); }
public int UpdateSql(string sql) { throw new
NotImplementedException(); }
public int DeleteSql(string sql) { throw new
NotImplementedException(); }
public int InsertSql(string sql) { throw new
NotImplementedException(); }
#endregion
#region File Operations
public string ReadText(string filename) { throw new
NotImplementedException(); }
public void WriteText(string filename, string text) { throw new
NotImplementedException(); }
public byte[] ReadFile(string filename) { throw new
NotImplementedException(); }
public void WriteFile(string filename, byte[] binaryData) { throw new
NotImplementedException(); }
#endregion
}
As you can see in the preceding code, the class does two main things: it performs database operations and it performs file operations. Now the code is neatly organized within correctly named regions used to logically separate code within a class. But the Single Responsibility Principle (SRP) is broken. We would need to begin by refactoring this code to separate out the database operations into a class of their own, called something like DatabaseManager.
Then, we would remove the database operations from the DbAndFileManager class, leaving only the file operations, and then rename the DbAndFileManager class to FileManager. We would also need to consider the namespace of each file, and whether it should be modified so that the DatabaseManager would be placed in the Data namespace and the FileManager would be placed in the FileSystem namespace, or their equivalents in your program.
The following code is the result of extracting the database code from the DbAndFileManager class into its own class and in the correct namespace:
using System;
using System.Data.SqlClient;
namespace CH01_CodingStandardsAndPrinciples.GoodCode.Data
{
public class DatabaseManager
{
#region Database Operations
public void OpenDatabaseConnection() { throw new
NotImplementedException(); }
public void CloseDatabaseConnection() { throw new
NotImplementedException(); }
public int ExecuteSql(string sql) { throw new
NotImplementedException(); }
public SqlDataReader SelectSql(string sql) { throw new
NotImplementedException(); }
public int UpdateSql(string sql) { throw new
NotImplementedException(); }
public int DeleteSql(string sql) { throw new
NotImplementedException(); }
public int InsertSql(string sql) { throw new
NotImplementedException(); }
#endregion
}
}
The refactoring of the filesystem code results in the FileManager class in the FileSystem namespace, as shown in the following code:
using System;
namespace CH01_CodingStandardsAndPrinciples.GoodCode.FileSystem
{
public class FileManager
{
#region File Operations
public string ReadText(string filename) { throw new
NotImplementedException(); }
public void WriteText(string filename, string text) { throw new
NotImplementedException(); }
public byte[] ReadFile(string filename) { throw new
NotImplementedException(); }
public void WriteFile(string filename, byte[] binaryData) { throw
new NotImplementedException(); }
#endregion
}
}
We've seen how to identify classes that do too much, and how we can refactor them to do only a single thing. Now let's repeat the process as we look at methods that do many things.
Methods that do many things
I have found myself getting lost in methods with many, many levels of indentation doing many things in those various indentations. The permutations were mind-boggling. I wanted to refactor the code to make maintenance easier, but my senior prohibited it. I could clearly see how the method could have been smaller by farming out the code to different methods.
Time for an example. In this example, the method accepts a string. That string is then encrypted and decrypted. It is also long so that you can see why methods should be kept small:
public string security(string plainText)
{
try
{
byte[] encrypted;
using (AesManaged aes = new AesManaged())
{
ICryptoTransform encryptor = aes.CreateEncryptor(Key, IV);
using (MemoryStream ms = new MemoryStream())
using (CryptoStream cs = new CryptoStream(ms, encryptor,
CryptoStreamMode.Write))
{
using (StreamWriter sw = new StreamWriter(cs))
sw.Write(plainText);
encrypted = ms.ToArray();
}
}
Console.WriteLine($"Encrypted data:
{System.Text.Encoding.UTF8.GetString(encrypted)}");
using (AesManaged aesm = new AesManaged())
{
ICryptoTransform decryptor = aesm.CreateDecryptor(Key, IV);
using (MemoryStream ms = new MemoryStream(encrypted))
{
using (CryptoStream cs = new CryptoStream(ms, decryptor,
CryptoStreamMode.Read))
{
using (StreamReader reader = new StreamReader(cs))
plainText = reader.ReadToEnd();
}
}
}
Console.WriteLine($"Decrypted data: {plainText}");
}
catch (Exception exp)
{
Console.WriteLine(exp.Message);
}
Console.ReadKey();
return plainText;
}
As you can see in the preceding method, it has 10 lines of code and is hard to read. Plus, it is doing more than one thing. This code can be broken down into two methods that each perform a single task. One method would encrypt a string, and the other method would decrypt the string. This leads us nicely into why methods should have no more than 10 lines of code.
Methods with more than 10 lines of code
Large methods are not nice to read and understand. They can also lead to very hard-to-find bugs. Another problem with large methods is they can lose sight of their original intent. It's even worse when you come across large methods that have sections separated by comments and code wrapped in regions.
If you have to scroll to read a method, then it is too long and can lead to programmer stress and misinterpretation. This in turn can lead to modifications that will break the code or the intent, or both. Methods should be as small as you can make them. But common sense does need to be exercised, as you can take the matter of small methods to the nth degree to the point that it becomes excessive. The key to getting the right balance is to ensure the intent of the method is very clear and succinctly implemented.
The previous code is a good candidate for why you should keep methods small. Small methods are easy to read and understand. Normally, if your code drifts beyond 10 lines it may be doing more than it is intended to. Make sure your methods name their intentions, as in OpenDatabaseConnection() and CloseDatabaseConnection(), and that they stick to their intentions and do not deviate away from them.
We are now going to take a look at method parameters.
Methods with more than two parameters
Methods with many parameters tend to get a bit unwieldy. Apart from being hard to read, it is very easy to pass a value to the wrong parameter and break type safety.
Testing methods get increasingly more complex as the number of parameters increases, the main reason being that you have more permutations to apply to your test cases. It is possible that you will miss a use case that will cause issues in production.
Using exceptions to control program flow
Exceptions used to control program flow may hide the intention of the code. They can also lead to unexpected and unintended results. The very fact that your code has been programmed to expect one or more exceptions shows your design to be wrong. A typical scenario that is covered in more detail in Chapter 5, Exception Handling.
A typical scenario is when a business uses Business Rule Exceptions (BREs). A method will perform an action anticipating that an exception will be thrown. The program flow will be determined by whether the exception is thrown or not. A much better way is to use available language constructs to perform validation checks that return a Boolean value.
The following code shows the use of a BRE to control program flow:
public void BreFlowControlExample(BusinessRuleException bre)
{
switch (bre.Message)
{
case "OutOfAcceptableRange":
DoOutOfAcceptableRangeWork();
break;
default:
DoInAcceptableRangeWork();
break;
}
}
The method accepts BusinessRuleException. Depending upon the message in the exception, BreFlowControlExample() either calls the DoOutOfAcceptableRangeWork() method or the DoInAcceptableRangeWork() method.
A much better way to control the flow is through Boolean logic. Let's look at the following BetterFlowControlExample() method:
public void BetterFlowControlExample(bool isInAcceptableRange)
{
if (isInAcceptableRange)
DoInAcceptableRangeWork();
else
DoOutOfAcceptableRangeWork();
}
In the BetterFlowControlExample() method, a Boolean value is passed into the method. The Boolean value is used to determine which path to execute. If the condition is in the acceptable range, then DoInAcceptableRangeWork() is called. Otherwise, the DoOutOfAcceptableRangeWork() method is called.
Next, we will consider code that is difficult to read.
Code that is difficult to read
Code such as lasagna and spaghetti code is really hard to read or follow. Badly named methods can also be a pain as they can obfuscate the intention of the method. Methods are further obfuscated if they are large and if linked methods are separated by a number of unrelated methods.
Lasagna code, also known more commonly as indirection, refers to layers of abstraction where something is referred to by name rather than by action. Layering is used extensively in Object-Oriented Programming (OOP) and to good effect. However, the more indirection is used, the more complex code can become. This can make it very hard for new programmers on a project to get up to speed with understanding the code. So there must be a balance struck between indirection and ease of understanding.
Spaghetti code refers to a tangled mess of tightly coupled code with low cohesion. Such code is very hard to maintain, refactor, extend, and redesign. Though on the plus side, it can be very easy to read and follow since it is more procedural in its programming. I remember working as a junior programmer on a VB6 GIS program that was sold to companies and used for marketing purposes. My technical director and his senior programmers had previously tried to redesign the software and failed. So they passed the gauntlet to me so that I would redesign the program. But not being skilled in software analysis and design at the time, I also failed.
The code was just too complex to follow and group into related items, and it was way too big. With hindsight, I would have been better off making a list of everything the program did, grouping the list by features, and then coming up with a list of requirements without even looking at the code.
So my lesson learned when redesigning software is to avoid looking at the code at all costs. Write down everything the program does, and what the new functionality is that it should include. Turn the list into a set of software requirements with associated tasks, tests, and acceptance criteria, and then program to the specifications.
Code that is tightly coupled
Code that is tightly coupled is hard to test and hard to extend or modify. It is also hard to reuse code that is dependent on other code within a system.
An example of tight coupling is when you reference a concrete class type in the parameter rather than referencing an interface. When referencing a concrete class, any changes to the concrete class directly affect the class that references it. So if you have a database connection class for a client that connects to SQL Server, and then takes on another customer that requires an Oracle database, then the concrete class would have to be modified for that specific customer and their Oracle database. That would lead to two versions of the code.
The more customers there are, the more versions of the code required. This soon becomes untenable and a right nightmare to maintain. Imagine that your database connection class has 100,000 different clients using 1 of 30 variations of the class, and they all have the same bug that has been identified and affects them all. That is 30 classes that have to have the same fix put in place, tested, packaged, and deployed. That's a lot of maintenance overhead, and very costly financially.
This particular scenario can be overcome by referencing an interface type and then using a database factory to build the required connection object. Then the connection string can be set in a configuration file by the customer and passed into the factory. The factory would then produce a concrete connection class that implements a connection interface for the specific type of database specified in the connection string.
Here is a bad example of tightly coupled code:
public class Database
{
private SqlServerConnection _databaseConnection;
public Database(SqlServerConnection databaseConnection)
{
_databaseConnection = databaseConnection;
}
}
As you can see from the example, our database class is tied to using SQL Server and would require a hardcoded change to accept any other type of database. We will be covering refactoring of code in later chapters with actual code examples.
Low cohesion
Low cohesion consists of unrelated code that performs a variety of different tasks all grouped together. An example would be a utility class that contains a number of different utility methods for handling dates, text, numbers, doing file input and output, data validation, and encryption and decryption.
Objects left hanging around
When objects are left hanging around in memory, they can lead to memory leaks.
Static variables can lead to memory leaks in several ways. If you're not using DependencyObject or INotifyPropertyChanged, then you are effectively subscribing to events. The Common Language Runtime (CLR) creates a strong reference by using the ValueChanged event via the PropertyDescriptors AddValueChanged event, which results in the storage of PropertyDescriptor that references the object it is bound to.
Unless you unsubscribe your bindings, you will end up with a memory leak. You will also end up with memory leaks using static variables that reference objects that don't get released. Any object that is referenced by a static variable is marked as not to be collected by the garbage collector. This is because static variables that reference objects are Garbage Collection (GC) roots, and anything that is a GC root is marked by the garbage collector as do not collect.
When you use anonymous methods that capture class members, the instance of the class is referenced. This causes a reference to the class instance to remain alive while the anonymous methods stay alive.
When using unmanaged code (COM), if you do not release any managed and unmanaged objects and explicitly deallocate any memory, then you will end up with memory leaks.
Code that caches indefinitely without using weak references, deleting unused cache, or limiting the cache size will eventually run out of memory.
You would also end up with a memory leak if you were to create object references in a thread that never terminates.
Event subscriptions that are not anonymous reference classes. While these events remain subscribed to, the objects will remain in memory. So unless you unsubscribe from events when they are not needed, it is likely you will end up with a memory leak.
Use of the Finalize() method
While finalizers can help free up resources from objects that have not been correctly disposed of and help to prevent memory leaks, they do have a number of drawbacks.
You do not know when finalizers will be called. They will be promoted by the garbage collector along with all dependants on the graph to the next generation, and will not be garbage-collected until the garbage collector decides to do so. This can mean that objects stay in memory for a long time. Out-of-memory exceptions could occur using finalizers as you can be creating objects faster then they are getting garbage-collected.
Over-engineering
Over-engineering can be an utter nightmare. The biggest reason for this is that as a mere human, wading through a massive system, trying to understand it, how you are to use it, and what goes where is a time-consuming process. All the more so when there is no documentation, you are new to the system, and even people who have been using it much longer than you are unable to answer your questions.
This can be a major cause of stress when you are expected to work on it with set deadlines.
Learn to Keep It Simple, Stupid
A good example of this is at one of the places I've worked. I had to write a test for a web app that accepted JSON from a service, allowed a child to do a test, and then passed the resulting scoring to another service. I did not use OOP, SOLID, or DRY, as I should have according to company policy. But I did get the work done by using KISS and procedural programming with events in a very small time frame. I was penalized for it and forced to rewrite it using their homegrown test player.
So I set about learning their test player. There was no documentation, it did not follow their DRY principles, and very few people if any really understood it. Instead of a few days, like my penalized system, my new version that had to use their system took weeks to build because it did not do what I needed it to do, and I was not allowed to modify it to do what I needed it to do. So I was slowed down while I waited for someone to do what was required.
My first solution satisfied the business requirements and was an independent piece of code that cared about nothing else. The second solution satisfied the development team's technical requirements. The project lasted longer than the deadline. Any project that overshoots its deadline costs the business more money than planned.
The other point I would like to make with my penalized system was that it was far simpler and easier to understand than the newer system that was rewritten to use the generic test player.
You don't always have to follow OOP, SOILD, and DRY. Sometimes it pays not to. After all, you can write the most beautiful OOP system. But under the hood, your code is converted to procedural code that is closer to what the computer understands!
Lack of regions in large classes
Large classes with lots of regions are very hard to read and follow, especially when related methods are not grouped together. Regions are very good for grouping similar to members within a large class. But they are no good if you don't use them!
Lost-intention code
If you are viewing a class and it is doing several things, then how do you know what its original intention was? If you are looking for a date method, for example, and you find it in a file class in the input/output namespace of your code, is the date method in the right location? No. Will it be hard for other developers who don't know your code to find that method? Of course it will. Take a look at this code:
public class MyClass
{
public void MyMethod()
{
// ...implementation...
}
public DateTime AddDates(DateTime date1, DateTime date2)
{
//...implementation...
}
public Product GetData(int id)
{
//...implementation...
}
}
What is the purpose of the class? The name does not give any indication, and what does MyMethod do? The class also appears to be doing date manipulation and getting product data. The AddDates method should be in a class solely for managing dates. And the GetData method should be in the product's view model.
Directly exposing information
Classes that directly expose information are bad. Apart from producing tight coupling that can lead to bugs, if you want to change the information type, you have to change the type everywhere it is used. Also, what if you want to perform data validation before the assignment? Here's an example:
public class Product
{
public int Id;
public int Name;
public int Description;
public string ProductCode;
public decimal Price;
public long UnitsInStock
}
In the preceding code, if you wanted to change UnitsInStock from type long to type int, you would have to change the code everywhere it is referenced. You would have to do the same with ProductCode. If new product codes had to adhere to a strict format, you would not be able to validate product codes if the string could be directly assigned by the calling class.
Good code
Now that you know what not to do, it's time to look briefly at some good coding practices to be able to write pleasing, performant code.
Proper indentation
When you use proper indentation, it makes reading the code much easier. You can tell by the indentation where code blocks start and end, and what code belongs to those code blocks:
public void DoSomething()
{
for (var i = 0; i < 1000; i++)
{
var productCode = $"PRC000{i}";
//...implementation
}
}
In the preceding simple example, the code looks nice and is readable. You can clearly see where each code block starts and finishes.
Meaningful comments
Meaningful comments are comments that express the programmer's intention. Such comments are useful when the code is correct but may not be easily understood by anyone new to the code, or even to the same programmer in a few week's time. Such comments can be really helpful.
API documentation comments
A good API is an API that has good documentation that is easy to follow. API comments are XML comments that can be used to generate HTML documentation. HTML documentation is important for developers wanting to use your API. The better the documentation, the more developers are likely to want to use your API. Here's an example:
/// <summary>
/// Create a new <see cref="KustoCode"/> instance from the text and globals. Does not perform
/// semantic analysis.
/// </summary>
/// <param name="text">The code text</param>
/// <param name="globals">
/// The globals to use for parsing and semantic analysis. Defaults to <see cref="GlobalState.Default"/>
/// </param>.
public static KustoCode Parse(string text, GlobalState globals = null) { ... }
This excerpt from the Kusto Query Language project is a good example of an API documentation comment.
Proper organization using namespaces
Code that is properly organized and placed in appropriate namespaces can save developers a good amount of time when looking for a particular piece of code. For instance, if you are looking for classes and methods to do with dates and times, it would be a good idea to have a namespace called DateTime, a class called Time for time-related methods, and a class called Date for date-related methods.
The following is an example of the proper organization of namespaces:
| Name | Description |
| CompanyName.IO.FileSystem | The namespace contains classes that define file and directory operations. |
| CompanyName.Converters | The namespace contains classes for performing various conversion operations. |
| CompanyName.IO.Streams | The namespace contains types for managing stream input and output. |
Good naming conventions
It is good to follow the Microsoft C# naming conventions. Use Pascal casing for namespaces, classes, interfaces, enums, and methods. Use camel case for variable names and argument names, and make sure to prefix member variables with an underscore.
Have a look at this example code:
using System;
using System.Text.RegularExpressions;
namespace CompanyName.ProductName.RegEx
{
/// <summary>
/// An extension class for providing regular expression extensions
/// methods.
/// </summary>
public static class RegularExpressions
{
private static string _preprocessed;
public static string RegularExpression { get; set; }
public static bool IsValidEmail(this string email)
{
// Email address: RFC 2822 Format.
// Matches a normal email address. Does not check the
// top-level domain.
// Requires the "case insensitive" option to be ON.
var exp = @"\A(?:[a-z0-9!#$%&'*+/=?^_`{|}~-]+(?:\.
[a-z0-9!#$%&'*+/=?^_`{|}~-]+)@(?:[a-z0-9](?:[a-z0-9-]
[a-z0-9])?\.)+[a-z0-9](?:[a-z0-9-]*[a-z0-9])?)\Z";
bool isEmail = Regex.IsMatch(email, exp, RegexOptions.IgnoreCase);
return isEmail;
}
// ... rest of the implementation ...
}
}
It shows suitable examples of naming conventions for namespaces, classes, member variables, classes, parameters, and local variables.
Classes that only do one job
A good class is a class that does only one job. When you read the class, its intention is clear. Only the code that should be in that class is in that class and nothing else.
Methods that do one thing
Methods should only do one thing. You should not have a method that does more than one thing, such as decrypting a string and performing string replacement. A method's intent should be clear. Methods that do only one thing are more inclined to be small, readable, and intentional.
Methods with less than 10 lines, and preferably no more than 4
Ideally, you should have methods that are no longer than 4 lines of code. However, this is not always possible, so you should aim to have methods that are no more than 10 lines in length so that they are easy to read and maintain.
Methods with no more than two parameters
It is best to have methods with no parameters, but having one or two is okay. If you start having more than two parameters, you need to think about the responsibility of your class and methods: are they taking on too much? If you do need more than two parameters, then you are better placed to pass an object.
Any method with more than two parameters can become difficult to read and follow. Having no more than two parameters makes the code readable, and a single parameter that is an object is way more readable than a method with several parameters.
Proper use of exceptions
Never use exceptions to control program flow. Handle common conditions that might trigger exceptions in such a way that an exception will not be raised or thrown. A good class is designed in such a way that you can avoid exceptions.
Recover from exceptions and/or release resources by using try/catch/finally exceptions. When catching exceptions, use specific exceptions that may be thrown in your code, so that you have more detailed information to log or assist in handling the exception.
Sometimes, using the predefined .NET exception types is not always possible. In such cases, it will be necessary to produce your own custom exceptions. Suffix your custom exception classes with the word Exception, and make sure to include the following three constructors:
- Exception(): Uses default values
- Exception(string): Accepts a string message
- Exception(string, exception): Accepts a string message and an inner exception
If you have to throw exceptions, don't return error codes but return exceptions with meaningful information.
Code that is readable
The more readable the code is, the more developers will enjoy working with it. Such code is easier to learn and work with. As developers come and go on a project, newbies will be able to read, extend, and maintain the code with little effort. Readable code is also less inclined to be buggy and unsafe.
Code that is loosely coupled
Loosely coupled code is easier to test and refactor. You can also swap and change loosely coupled code more easily if you need to. Code reuse is another benefit of loosely coupled code.
Let's use our bad example of a database being passed a SQL Server connection. We could make that same class loosely coupled by referencing an interface instead of a concrete type. Let's have a look at a good example of the refactored bad example from earlier:
public class Database
{
private IDatabaseConnection _databaseConnection;
public Database(IDatabaseConnection databaseConnection)
{
_databaseConnection = datbaseConnection;
}
}
As you can see in this rather basic example, as long as the passed-in class implements the IDatabaseConnection interface, we can pass in any class for any kind of database connection. So if we find a bug in the SQL Server connection class, only SQL Server clients are affected. That means the clients with different databases will continue to work, and we only have to fix the code for SQL Server customers in the one class. This reduces the maintenance overhead and so reduces the overall cost of maintenance.
High cohesion
Common functionality that is correctly grouped together is known to be highly cohesive. Such code is easy to find. For example, if you look at the Microsoft System.Diagnostics namespace, you will find that it only contains code that pertains to diagnostics. It would not make sense to include collections and filesystem code in the Diagnostics namespace.
Objects are cleanly disposed of
When using disposable classes, you should always call the Dispose() method to cleanly dispose of any resources that are in use. This helps to negate the possibility of memory leaks.
There are times when you may need to set an object to null for it to go out of scope. An example would be a static variable that holds a reference to an object that you no longer require.
The using statement is also a good clean way to use disposable objects, as when the object is no longer in scope it is automatically disposed of, so you don't need to explicitly call the Dispose() method. Let's have a look at the code that follows:
using (var unitOfWork = new UnitOfWork())
{
// Perform unit of work here.
}
// At this point the unit of work object has been disposed of.
The code defines a disposable object in the using statement and does what it needs to between the opening and closing curly braces. The object is automatically disposed of before the braces are exited. And so there is no need to manually call the Dispose() method, because it is called automatically.
Avoiding the Finalize() method
When using unmanaged resources, it is best to implement the IDisposable interface and avoid using the Finalize() method. There is no guarantee of when finalizers will run. They may not always run in the order you expect or when you expect them to run. Instead, it is better and more reliable to dispose of unmanaged resources in the Dispose() method.
The right level of abstraction
You have the right level of abstraction when you expose to the higher level only that which needs exposure, and you do not get lost in the implementation.
If you find that you are getting lost in the implementation details, then you have over-abstracted. If you find that multiple people have to work in the same class at the same time, then you have under-abstracted. In both cases, refactoring would be needed to get the abstraction to the right level.
Using regions in large classes
Regions are very useful for grouping items within a large class as they can be collapsed. It can be quite daunting reading through a large class and having to jump back and forth between methods, so grouping methods that call each other in the class is a good way to group them. The methods can then be collapsed and expanded as needed when working on a piece of code.
As you can see from what we have looked at so far, good coding practices make for code that is far more readable and easier to maintain. We will now take a look at the need for coding standards and principles along with some software methodologies such as SOLID and DRY.
