This is yet another code review i was doing the other day and this time it's particularly on the .NET side of things and not about readability. Though i will share some of them, that gives perspectives to code reviewers, specifically .NET folks.

about:commit

We are migrating our code from .NET 4.6 to .NET Core 3.1 and most of the comments are related to platform neutrality in general and also covers some of the concerns with .NET framework and the C# language.

You might Win but won't Lin

Files

  • <Files.cs>

Explanation
The \n Line Ending won't work consistently across operating systems. It is heavily dependent on the OS, environment settings, the IO stream handling of the specific tools you are working with. The best way is to always use the abstraction that .NET gives, which is <span style="color:blue>Environment.NewLine</span>

All that's our code are not extensions

Files

  • <Files.cs>

Explanation


Extension methods are for those who use code from others that is not extensible, but there is a need for extension. For example , let's assume that we need a method called DefaultLogger() in the class log4j.Logger, so that all our code can just use the extension method, instead of injecting it. In this case you cannot extend Logger class (assuming it's sealed) and is not extensible by object orientation mechanics. But for convenience we can use the extension methods to pretend that we add the DefaultLogger method to the class. This is when we don't have control over the class log4j.Logger. But when you own the class, don't write extensions. Instead write them inside the class directly, or extend the class to write your functionalities. C# offers you language features as well as framework level features, and it's important to use the right features for the right use cases and equally important that we don't abuse features that are meant for something else.

The following is an excerpt from stack overflow (credits to the user rober) that i found to be a good analysis for the feature, and why it should not be used for obvious cases.

Single Responsibility: - You are almost always at least bending the single responsibility principle with an extension method because you are tacking on to something that is already a class (that you either don't have control over or are too afraid to touch).
Open/Close Principle: - Extension methods cannot be overridden, which means your method may not be adequately "open for extension".
Liskov substitution principle: - If you have any sort of inheritance structure you will not be able to use the extension methods on the sub types.
Interface segregation principle: - Although you can "extend" an interface, you have to provide a concrete implementation for that extension. So you can not program towards an interface >that can be implemented differently in a different context (eg Unit Test)
Dependency inversion principle: - If your code has any dependencies, how are you going to expose those dependencies

<U+FEFF>

If you see git diff command output you could possibly see this character as the first character in some files in the repository. This is call BOM (Byte Order Mark) character. The Unicode Standard permits the BOM in UTF-8 but does not require or recommend its use. Byte order has no meaning in UTF-8, so its only use in UTF-8 is to signal at the start that the text stream is encoded in UTF-8, or that it was converted to UTF-8 from a stream that contained an optional BOM. So if you see one you should remove it. (To remove it please refer to internet. There are many ways).

This character is primarily placed in the beginning of the file, when the file is using 16-bit or more to represent a character and the order of bytes to reconstruct the character is unknown, because of the unknown nature of the source of the file. The order of FE FF or FF FE will determine if the characters should be reconstructed using MSB then LSB or LSB then MSB.

In C# - static is a sin as in dynamic is a devil

FIles

  • <Files.cs>

If you can avoid, better avoid static classes and methods. Static classes are meant for situations where there is a specific use case in the problem domain that is static. More often we model behavior as static, that are not static in the real world. But there is better chance that they are singleton and end up as static in the source code. In these cases implement Singleton pattern. An example as below.

|Static class| Singleton |
|--|--|
|public class Hello<br>{<br>   public static int a;<br>   static Hello()<br>   {<br>      a= 500;<br>   }<br>}|<br> public class Hello<br>{<br>   public int a;<br>   private Hello()<br>   {<br>      a= 500;<br>   }<br>  private static Hello instance;<br>  public static Hello Instance()<br>  {<br>    if(instance == null)<br>        instance = new Hello();<br>    return instance;<br>  }| public class hello

The problem with static members is that their behavior is a bit complicated than we actually think Jon Skeet on BeforeFieldInit). Based on how we write code, calls from static initialisation / constructor run well before our program is even called. :-). I learned more things when digging information related to the above. Here it is

You don't need an I for yourself

Files

  • <IFiles.cs>

Interfaces are useful in following cases.

  • When you are using dependency injection and you want to inject a concrete type against a contract.
  • When your code is used by external parties who will be given interfaces and not concrete types.
  • When you have a design pattern that can change the behavior dynamically based on the conditions, but the contract and the flow of the overall workflow remains.

In cases where you have one interface and one implementation and you don't see any extension to that in any known near future (search about YAGNI principle) it's better to avoid an interface. If a class has just properties and no/merely methods, it's better to use the class instead and it's evolution can be towards growing abstract with more implementations. Don't create interfaces for Model classes and any data contract types. It's better to declare them as structs because they are just for messaging and interoperating purposes.

Reflect Thyself before Reflection

Files

  • <Files.cs>

Reflection is not a first class citizen in programming. The System.Reflection namespace is not for application developers. It's primarily intended for library development and tooling purposes. Using it as a primary means to handle object, it's properties and methods is not right. If we are in a situation where we cannot access a class / it's methods we have to re-think our class dependency setup and review and fix it. Reflecting won't help. This is the same with Activator.CreateInstance. This is primarily used for creating instances of Interop COM objects in managed memory (and honestly we don't use any of it) and also to create instances of objects when using dynamic languages in context of the CLR (popularly known as DLR). Never use reflection it's for Newtonsoft, NUnit and not for us. We know our types well and we can call them by name.

Some legitimate uses of reflection are

  • Middleware pipeline in a ASP.NET application
  • Document generator reflecting on types to generate API documentation
  • Dependency Injection container using it to discover and inject types. Example : MEF. (Though reflecting and injecting is something i consider bad)
  • A Test runner attempting to identify the test classes and methods to discover and run.

In all these cases the user is a framework / library that doesn't have control on the code it's trying to use. But when the caller and callee are the same user's code, never go for reflection.

Play with Player vs Play Player With

Files

  • <Files.cs>

I am going to keep this one simple, because this is more for readability and might not possibly open scope for error(s). Think of the following method call statements and see which one is more readable, and please follow that. Interestingly you will have this issue only with Generics on Static classes and not for non-static classes.

DallasCowBoys.Play<SeattleSeaHawks>(...)
vs
DallasCowBoys<SeattleSeaHawks>.Play(...)

This argument is for whether to make the class or the method generic. In most cases it is dependent on the use case. But there is a difference between both. When you are applying generic type on a class, you plausibly invoke actions against that generic type. But when you are passing the generic type to a method, you are passing the type as an input and the action will involve the object but might not necessarily act on it. So use it judiciously to convey right meaning and readability.

I think that's all i have for now, and stay tuned for the next code review.