Base on my years experiences in codes review, I recognized some best practices which is really simple to apply to sanitize the codes, make them more testable or reusable. Experienced developers might apply some of them and don’t even realize it.
In this article I will expose some of the common coding mistakes which I often see in codes review sessions along with some simple / best practice to avoid them. The sample codes is in C# flavor, but the ideas is applicable for other similar languages Java, Kotlin…
1. try/catch the right way
a) Don’t try/catch whenever you feel like it
Rule of thumb: Avoid try/catch as much as possible
try/catch is BAD. It hurts the application performance. It should only be use on the out-most application entry and thread entry. In other parts of the code, all try/catch must to be justified.
So avoid to write “try” and “catch” as much as you could. If you have no choice => put the comment to justify why would you have to try/catch here?
If you use try/catch only to capture silly errors or to hide a problem => please think twice and justify why would you do that?
For example a BAD Code:
var v
try {
v = int.Parse(something);
}
catch {
//empty code: nothing to do with the exception just hide the Parse error
}
to make this codes better we can remove the try/catch and use int.TryParse() instead.
b) Don’t hide error
Example: A BAD code: please never do it!
try {
MAIN_CODE
}
catch (Exception e)
{
// do nothing
}
c) Don’t cut off the stacktrace
Example: A BAD code:
try {
MAIN_CODE
}
catch (Exception e)
{
DO_SOMETHING_WITH_EXCEPTION
throw e; //BAD: you cut-off the stacktrace completely right here
}
A better one (but still second-rates for .Net framework, OK for .NetCore)
try {
MAIN_CODE which calls OTHER_FUNCTIONS
}
catch (Exception e)
{
DO_SOMETHING_WITH_EXCEPTION
throw; //still average (for .Net framework)! it keep the stacktrace of OTHER_FUNCTIONS but it hide the line in MAIN_CODE which trigger exception
}
Update: This solution is ok in .NetCore which is smart enough to pin-point the line in the MAIN_CODE in the stacktrace. But it is not the case for the old .Net framework. This moron puts the “throw” line (not the line of the MAIN_CODE line) to the stacktrace.
d) Avoid to convert an Exception to other Exception
try {
..
throw new SomeException()
..
}
catch (SomeException e)
{
throw new OtherExeption(); //BAD: you cut-off the stacktrace right here..
}
Please keep the original exception if possible, only convert it to other exception if you really must to do so (please justify the reason).
In fact, we should make this conversion only to “hide” the real error (SomeException) to the consumer. So we expose to the consumer OtherException with other details instead => please explain why did you want to hide the real error to the consumer and cut-off the stacktrace?
If you don’t want to hide the real error from the consumer but stil have other “mysterious reason” to convert from SomeException to OtherException and complexify the stacktrace and the debugging task, then here is the “right” way
try {
..
throw new SomeException()
..
}
catch (SomeException e)
{
throw new OtherExeption("more information", e); //better (but still average) please justify why couldn't you keep the original exception?
}
2. Use or create static method the right way
a) Static is only good for Helper / Tools / Utilities which don’t change (mutate) our system
- it should always give the same output for the same inputs
- it shouldn’t call any thing which mutate the system
Otherwise: avoid to create static methods..
b) Think about Singleton pattern
Make normal class which is unit-testable in any context, then use a singleton to make it available globally in your application.
Don’t forget that the static singleton might not be testable because it depends on the application context.. but the class itself is often testable, we only need to give it all the dependencies it wanted in the constructor (see the following)
3. Constructors must to express dependencies
The parameters of the constructor should be the dependencies of the class. Example
class FlowsConverter {
public FlowsConverter(IUsdConverter usdConverter, IContractCustomerStore contractCustomerStore){
//should initialize object, don't do any I/O or complicated computation here
}
}
In this example the class FlowsConverter depends on UsdConverte and ContractCustomerStore => The dependencies are expressed in the constructor.
If a class has many (more than 3+) dependencies then it might be time to review the design; consider the Facade Service to group common logic together.
In case the dependencies graph grow complicate, it is time to adopt a Dependencies injection framework.
4. Single Responsibility Principle (SRP)
a) Avoid unpredictable behavior in the constructor: eg: calculation (business logic), validation, I/O (access to DB, files..), let the constructor do its job: create object
b) Avoid unpredictable behavior in the getter, setter: example this code seem ok but it is actually not good:
public class MailInfo
{
private string str_subject;
public string subject
{
get { return str_subject; }
set {
if (str_subject is empty) throw Exception
str_subject = value
}
}
...
- => it is not the responsiblity of
MailInfoto validate his property or to decide if he’ll allow the subject be empty - => it is the responsibility of the Consumer class which might use an external
MailInfoValidatorfor this purpose
c) Don’t save the number of class or get lazy to create a new class file! Please
- break a long method to smaller ones
- break a big class to smaller ones which do only one things (and do it well)
- And each time you create a new class or method, think about how you will write test for it.
- Test codes is often harder to write than main codes. But please, at least.. think about how you would write test to your new class / methods evens if you don’t actually write the test.
- If you think about how to write the test for your class, you will naturally refactor your class to make it easier to test and make the better codes.
5. A class which create a new IDisposable object should also be a IDisposable
- It is highly recommended that when you create a
new IDisposable, do it in ausing - But for some reason, you cannot create
new IDisposablein a “using”, and you have to keep your newIDisposableobject alive for an unforseenable time then think about making the parent classIDisposable. In other word: You should make your classIDisposableand free all otherIDisposablewhich is created during its life.
Example BAD:
public class A
{
public IDisposable b;
public M()
{
this.b = new SomeDisposable(); //A created something Disposable and keep it alive (not use "using")
}
}
A better version: make A also disposable and explicitly clean all the left-over IDisposable which it created during its life time.
public class A: IDisposable
{
public IDisposable b;
public M()
{
this.b = new SomeDisposable(); //A created something Disposable and keep it alive (not use "using")
}
public void Dispose()
{
if (b != null) b.Dispose();
}
}
6. Focus to code reusable inside application, not outside application
If you are NOT making a framework or a library to publish else-where then
- Don’t try to make everything reusable outside of your application by abusing
ReflectionandGenericwhenever you recognize a possible patterns. - Please don’t anticipate that your codes might be reused by other applications.. If it’ll comes true (one day), we can always refactor it later.. => Focus to make your code easy to read (by others) and effective specifically for your current application first.
Unless you really want to make a framework or a generic library, then engage to publish and maintain it in a proper repository with support, documentation etc.. There is no point to make everything as Generic as possible. We usually have Generic for common things already, find something popular / standard / well maintain then just use it.
If you have to code something reusable then make it generic “enough” for your 2, 3 uses cases at hand, but do not make it super generic / abstract thinking that other peoples will use it for 1 milions others uses cases.
7. Don’t blame old codes, try to enhance them!
It is common to copy old codes to make new ones. The new codes become YOUR codes. It means:
- You have to master it: If there is part that you don’t understand in your code then it is bad => so don’t blindly copy the codes that you didn’t understand.
- You have to take responsibility of your codes: If the new code (YOUR codes) is silly / strange or bug! please don’t blame that it was because this part was copied from the old codes.