[!info] 👍 Rule of thumb: Avoid try/catch as much as possible
try/catch is BAD. It hurts the application performance. Hence, all try/catch must to be justified.
You should put a comment to justify why would you have to try/catch here?
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.
Whenever you throw an exception please explain why? and give all the helpful information about the context...
Example: A BAD code: please never do it!
try {
MAIN_CODE
}
catch (Exception e)
{
// do nothing*
}Example: A BAD code:
try {
MAIN_CODE
}
catch (Exception e)
{
DO_SOMETHING_WITH_EXCEPTION
throw e; //BAD: you cut-off the stacktrace 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.
Example:
try {
..
throw new SomeException()
..
}
catch (SomeException e)
{
throw new OtherExeption(); //BAD: you cut-off the stacktrace right here..
//throw new OtherExeption(e); //BETTER: but why?
//throw new OtherExeption("more information, more context", e); //GOOD!
}Please keep the original exception, Only convert it to other exception if you need to enrich the original exception with more information.
BAD example
throw new Exception("blabla"); Example 2: A BAD code: please never do it
throw new Exception("");Example 3: Slightly better than Example 2:
throw new InvalidPaymentException("")Example 4: Good code
throw new InvalidPaymentException("Payment #12 is in pending...this status is unexpected blabla")Why?
The general “Exception” is usually reserved for fatal problems when we really don’t have any clue why it happened (”Unknow error”). These problems need to be investigate and fixed. If you need to throw something then you should know what you are throwing and have all the necessary context.. so what you are throwing is a “Known Error” ⇒ please don’t throw “Exception” for “Known Error”. If there isn’t any pre-defined exception fits what you are throwing then don’t hesitate to create a new one!
Se also: Technical Error vs Business Error (github.com)
This is a very common problem
Example: BAD code:
public Payment GetPaymentOnPsp(string paymentId)
{
Payment payment = psp.FindPaymentWithId(paymentId);
if (payment == null) {
throw new PaymentNotFoundException($"The payment #{paymentId} is not found on PSP side");
}
}The function GetPaymentOnPsp could not find any payment on the PSP side and throw a PaymentNotFoundException, everything seem perfectly normal, so why is it bad?
- We should only raise (throw) an Exception only if there is really a problem.
- The function
GetPaymentOnPspcannot decide if it is normal or not that the payment couldn’t be found on the PSP side. So it shouldn’t throw error in this case. It should just return null or something which tell the caller that it couldn’t find the payment on the PSP side. And the caller will decide if this situation is normal or not (and throw the appropriate error with appropriated message if needed). - the function
GetPaymentOnPspshould only raise/throw error for real anomalies such as it couldn’t connect to the PSP, or Database..
GOOD code:
//Get payment information on the PSP side, return null if not found
public Payment GetPaymentOnPsp(string paymentId)
{
return psp.FindPaymentWithId(paymentId);
}Here is the caller codes which illustrate that only the caller know if a exception should be throw or not and have all the context detail to do it.
var payment = GetPaymentOnPsp(callbackReceived.paymentId)
if (payment == null) //payment not found on Psp side
{
if (callbackReceived.Status == "cancelled") {
// it is normal that the payment is not found on PSP side because it is cancelled, we should not throw error here
...
}
else {
//it is not normal that we received the callback but couldn't find the payment
throw new PaymentNotFoundException($"We received the callback with status {callbackReceived.Status} but the payment #{paymentId} is not found on PSP side");
}
}
...If you kept the Bad code, then the caller would become ugly and inefficient:
try {
var payment = GetPaymentOnPsp(callbackReceived.paymentId)
}
catch (PaymentNotFoundException) {
if (callbackReceived.Status == "cancelled") {
// it is normal that the payment is not found on PSP side because it is cancelled, we will ignore the exception..
...
}
else {
//it is not normal that we received the callback but couldn't find the payment
throw new PaymentNotFoundException($"We received the callback with status {callbackReceived.Status} but the payment #{paymentId} is not found on PSP side");
}
}Conclusion:
You should not throw error whenever you feel like.
If you throw error then you will have to justify the reason why do you think that the situation is not normal, and give all the argument, context on the anomalies. (Suggestion on what we should to verify or to investigate are appreciated as well).
- 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..
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 usually testable, we only need to give it all the dependencies it wanted in the constructor (see the following)
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.
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 responsibility 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)
- 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.
- If you fragment the codes too much and complexify things with unclear logic, then the test codes will tell you as well (eg. test make no sense at all, or not testable class..)
- 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 unforeseeable time then think about making the parent classIDisposable. In other words: 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();
}
}In the following codes, the async/await keyword appeared to be useless
async Task<SendResult> SendNotification() {
//Do some stuff..
...
return await sender.Send()
}Remove the async / await keyword in this case calls “Elide async await”
Task<SendResult> SendNotification() {
//Do some stuff..
...
return sender.Send()
}This refactor will CHANGE the behavior of your function.
- async/await eliding is fine in most case, it will save you some nano-second + some bytes in memory.
- but there might be some unexpected behavior
Smart guys recommended that Elide async/await only for proxy function (generally, they are functions which have only 1 line of code to forward the result)
//GOOD ELIDING
async Task<SendResult> SendNotification() {
return await sender.Send()
}
//GOOD ELIDING
Task<string> PassthroughAsync(int x) => _service.PassthroughAsync(x);
//GOOD ELIDING
Task<string> OverloadsAsync() => OverloadsAsync(CancellationToken.None);- Otherwise, just don’t Elide async/await except if you are understand the differences and sure about your decision.
//BAD ELIDING => not a proxy function (it does other stuff which might crashed)
Task<SendResult> SendNotification() {
< OTHER CODES >
return sender.Send()
}- fake proxy function ⇒ BAD ELIDING:
//BAD ELIDING => not a proxy function
Task<string> PassthroughAsync(int x)
{
return _service.PassthroughAsync(GetFirstArgument(), x);
}
//there are in fact 2 line of codes in the above function => BAD ELIDING
Task<string> PassthroughAsync(int x)
{
var firstArgument = GetFirstArgument(); // this one might crashed
return _service.PassthroughAsync(firstArgument , x);
}Make your function Unit testable · duongphuhiep/blog1 Wiki (github.com)
- Major branching points in your code
- When errors or unexpected values are encountered
- Any IO operations: calling database / stored proc or make http request
- Significant domain events
- Request failures and retries
- Beginning and end of time-consuming operations
-
Be generous with your logging but be strict with your logging levels
-
the "Error" level should be reserved for events that you intend to act on (events required support intervention)
- If there is too much logs "Error" (critical error and meaningless errors are mixed together) => they will be ignored and nobody will notices when real problem happens .
- Errors such as “Product Not Found”, “Input validation error” / “Failed to do something on External services” etc.. are no need for Support intervention. They should be on Info or Warning level.
-
The "Information" level should be reserved for events that would be needed in production (to monitor the state or the correctness of the application).
- Application start / end, Application config changes
- Start / End events of any Time-consuming or Important operations. For cheap / unimportant operations such as "GetProduct", "GetConfig"... which are called 1000 times every sec, we don't need to logs them on production => use “Debug” level in this case.
-
In almost all other cases the level of your logs level should be "Debug".
-
If you are indecisive between "Debug" and "Information" => Use "Debug". If you are indecisive between "Debug" and "Trace" => Use "Trace"
- if we are indecisive it means that the event might important / interesting (but not important or interesting enough)
- It is easy to temporarily activate a lower logs level (Trace, Debug) on Production.
It is common to copy old codes to make new ones, or to copy paste codes from outside (stackoverflow..) 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 (fully) understand.
- You have to take responsibility of your codes: If the new codes (YOUR codes) is silly / strange or bug! please don’t blame that it was because this part was copied from elsewhere.
-
Don’t try to make everything reusable outside of your application by abusing
ReflectionandGenericunless you wanted to make a framework or a library. -
Please don’t anticipate that your codes might be reused by other applications.. in the future. If it’ll comes true (one day), we can always refactor it later.. => Focus on making your codes easy to read (by others) and effective specifically for your current application first.
-
If you really want to make a framework or a generic library, then you will have to engage to publish and maintain it in a proper repository with support, documentation etc..
-
Here are 6 “symptoms” we often see in over-engineering codes:
-
The codes is hard to tests / mock.
- Others team member will needs you to explain how your codes works!
-
Re-inventing the wheel: you develop your own DI library, mocking library, ORM library, Guard library…
- why not? but please do it elsewhere in another repository.
-
Use Generic and/or Reflection:
- if the application purpose is to handle generic / chaotic data then it is justifiable that you have to work with Generic + Reflection, because it is the Business Logic requirement (and you don't have other choice). Though it is preferable to challenge the Product Owner / Architect guys: Did they really need a super Generic Application (a kind of Framework) or multiple applications to handle specifics data might be a better choice?
- if you are forced to use the Generic and Reflection because of the external framework / library constraints which impose you to do things in this way then it is also justifiable. Though it is preferable to change to other alternative frameworks / library which don’t forced you to work with Generic/Reflection.
- Otherwise if all the system of Generic or Reflection are invented (originated) by yourself then WARNING ⇒ are you trying to develop an application or a generic library? Do you really need all these generic in this specific application? if you anticipate that these Generic codes “might be re-used” by other applications then it is preferable to externalize them to a Nuget, and maintain + document them separately.
-
Too many interfaces / abstractions and most interfaces has only 1 implementation:
- If you need them for mock / tests then it is justifiable.
- “Clean architecture” usually forces you to make abstraction for everything, if the implementation and the interface are in different layers then it is also justifiable. In fact, the codes will most likely become heavy (over-engineering) by following the “Clean Architecture”, consider the Slices (or Vertical) Architecture.
-
Codes fragmented:
- It happens when you complexify a simple function and spread the codes to many small interfaces / classes. These many small fragmented codes usually do not make sense individually. They make sense only when standing together with other small codes fragments.
- The codes fragmented also happens when you split a "big 50 lines of codes" function into 10 "small 5 lines of codes function". Note "a 50 lines of codes" function is not that big, it was an exaggerated example. While it is a good practice to split complex function into "less complex" one to make the algorithm clearer, self-documented and more maintainable. However, you shouldn't use "line of codes" measurement to decide if a function should be split or not. Many success and well maintained codes based has large functions, proving that large functions are not as bad as you think => Stop to fragment your "large function" just because it exceeded 10 lines of codes
- In case of doubt, please ask yourself
- Do you really need to split the codes so much for such a simple functionality?
- Are these fragments codes testable individually? if yes do their tests make any sense?
-
Use of class inheritance, abstract class
- is it possible to flat things down to stupid codes, or use composition over inheritance?
[!info]
They are just some hints to pay attention to. It is NOT means that you cannot use Generic / Inheritance.. or your codes is over-engineering because of that. The most important is to think about others team members and anticipate that others developers might have difficult time to maintain your codes and add more tests cases for your codes.
- Do you think that others can easily understand + debug and modify your codes without any of your explanation or documentation?
- Do you think that others (or even yourself) can easily add more tests cases for your codes?
- Did you write any tests case for your codes so that other can follow your example and easily add more test cases the way you did?
Generated-codes you usually see:
- Scaffolding C# Entity classes from existing Database (we are following the “Database-first” approach)
- Generate SDK C# (client) codes to call a SOAP Webservice or OpenApi/Swagger Webservice (Add Service Reference to project)
- High performance logging codes..
A common mistake is to refactor / enhance these generated codes ⇒ you will lose all modifications when your teammates generate the codes again.
Touching the generated-codes is usually “cursed”. However, there are 2 situations that allow you to modify the generated codes:
- If nobody would regenerate them again. For example: Project template is a kind of codes generation, you create the project only once so it’s ok to modify the codes generated by the template!
- If the generated-codes are not compiled (the code generator is bugged or not smart enough to support your special situation) then you can slightly touch the generated codes to make it compiled. But no further modification / refactoring is allowed!
When you use generated-codes in the project then you will have to trust them as a working “black box”, and inside that black box might have long, ugly, smelly codes… So you don’t need to maintain or debug them. (otherwise what is the point of generating the codes?). It means that:
- You should test them a little to make sure that the generated codes work in your project but there is no need to write tests to cover for them in the CI/CD.
- they can be excluded from the sonar reports.
- if you want to update the generated-codes ⇒ the only way is to regenerate them again (with the latest generator for eg)…
If you couldn’t trust the generated-codes or dislike them ⇒ then remove them and manually write the equivalent codes by yourself in your way! You have total-control BUT.. as any other normal codes
- You should include them in the sonar report..
- You have to maintain them + write tests to cover for them.
- In case of Entity Framework DbContext: you MUST to write some integration tests to demonstrate that your DbContext works (on a real database) and we are able to do some Linq with them.
- In case SDK client: you MUST to write some integration tests to demonstrate that your Client codes works, it could at least “ping” / call (consume) the External Service, and get back a response (evens if the response is an error, it should be able to parse the error).
- These integration tests are not simple to setup in the CI/CD pipelines. However, you should NOT wait until the very end of your Dev and rely on the end-to-end tests. Find a way to tests these codes separately at least once during the development time.
- Create a Console program to test them if you need to…
- Or better! make this console application in the format of a ”XUnit project”, name it
*.OnceTime.IntegrationTests. It is more convenient to run these*.OnceTime.IntegrationTestswith a test runner instead of a Console program. Don’t forget to exclude these “once time integration tests” from the CI/CD pipelines. - If you are unable (or refuse) to deliver these required integration tests ⇒ then Go for the generated-codes and bear with them.. (there is a community out there using these generated-codes on production, you are not the only one)
What happens if you touch or refactor generated-codes?
- Well, they are no longer generated-codes but becomes normal codes (your codes) that we could no longer trust ⇒ You have to deliver the required
*.IntegreationTestdescribed above. - Forbid other team mates to regenerate the codes again to protect your modification / refactor.
- Generated-codes are often long + ugly + copy/paste-like repetitive + They usually contains comments in the first line telling that they are generated.
- Generated-codes are usually come with a README.md explaining how they are generated
“Repository” interface defines the data access contract. When consumer calls a method in a repository for example transactionRepository.GetFirst20Transactions()
Most of time, they would assume that the repository implementation would make a round trip to database which means:
- open a connection to the database,
- do the execute some kind of “Select sql”, get the result
- and finally close the connection, clean thing up...
We are sure about this common behavior when the output of your repository was a concrete type such as Array[], List<T>.. However if the output type was IEnummerable<T> then the “common sens” might no longer be true, and the “Repository” behavior might surprise the consumer
BAD Example:
interface ITransactionRepository {
Task<IEnumerable<TeCashExchangeNew>> GetFirst20()
}Why is it “Bad”? Depending on the implementation of TransactionRepository, the following situation MIGHT happened and surprise the consumers:
ITransactionRepository _transactionRepo = ...;
IEnumerable<TeCashExchangeNew> myFirst20 = await _transactionRepo.GetFirst20(); ///it takes 1ms, no round-trip to database as consumer expected
...
//later (in other place) in the codes
myFirst20.First(); //it takes 100ms, with a roundtrip to database which might crashAs long as consumer had not (yet) enumerate the IEnummerable then there might not have any round-trip to the database + the database connection might not clean up after the call to the repository as expected. The consumer might enumerate the IEnummerable very late in the code, and they did not expected that this action would trigger a database round-trip.
Moreover, at the moment he did so then the database connection might be already closed/disposed and the enumeration would crash.
Good Example:
interface ITransactionRepository {
Task<TeCashExchangeNew[]> GetFirst20(); //or List<TeCashExchangeNew>
}With this contract, then the TransactionRepository implementation is forced to enummerate the collection, and we are sure that the “common sens” is true.
BAD Code:
async Task<Thing> GetSomething(string id) {
if (thing in cache) { //cache hit: 1%
return cachedThing // we retrieve the value synchronously 1% of time
}
return await readThingFromDatabase() //we retrieve the value asynchronously 99% of time
}GOOD Code:
async ValueTask<Thing> GetSomething(string id) {
if (thing in cache) { //cache hit: 1%
return cachedThing // we retrieve the value synchronously 1% of time
}
return await readThingFromDatabase() //we retrieve the value asynchronously 99% of time
}In this example, we get thing from the cache first, if it wasn't in the cache then we will return it from the database.
In the "Bad code", we returned a Task, we should return ValueTask instead. The more cache hits we might have, then the more advantage the ValueTask will give us over normal Task
Justification:
- It might be a micro-optimization depending on How much the
GetSomethingfunction is “bombarded”. It might be a real-optimization in case the function is called a lot and with a high rate cache hit. - In all other case, especially when the cache hit is low then the memory allocation of the ValueTaks wrapper is insignificant, we don’t have to worry about it.
Always do normal mapping until it becomes really problematic and you can describe clearly the problem.
Consider a library such as mapperly to see if it solves the exact problem described above. If it cannot make the problem better, then don't use it.
Anyway if your application has to handle less than 10 mapping functions then a mapping library probably won't help.