Category: Software

Code quality has always been a concern when there are defects coming out of nowhere. There are code reviews and then there are code reviews. Testability of the code is also one of the code quality parameters. Unit Tests and their code coverage is indicator of the code quality and having a high quality set of unit tests gives confidence to make changes to the code. However we are not covering testing here.

What causes code quality issues

Lack of awareness across teams about what is code quality and coding principles like SOLID

Many projects tend to complete the reviews just because there is a process which they realize just before delivery and then they need to be compliant.

In other cases, when project has gone live the core team is dismantled and the support engineers who take over are scared of touching the code written by someone else. So they make changes and give some explanation and the reviewer is also scared of taking risks and the changes get approved without really doing a code quality check. They also comment around the code block with defect number.

Naming convention is a matter of choice. I wrote about Naming Conventions and Code Redability quite some time back In old times most of the developers chose to use Hungarian notation to prefix the variables to indicate the data type or UI control type etc. Microsoft chose to drop all of this since beginning of .Net. Many years have passed but we still see people using Hungarian notation. Junior developers tend to mimic the seniors of code styles from old projects. This is a habit that dies hard. So in a project you can see code developed by different developers and technical leads buy the argument that this is a trivial issue as long as the code works. However in a modern IDE like Visual Studio, which immediately can tell you about a wrong assignment, such use of abbreviated prefixes is really unnecessary. It has a huge impact on readability of the code.

In some projects for whatever reason no one bothers about code quality. Developers are either not aware or pressed by timeline or they take liberties because tech leads are too busy to review the code. The only way to ensure product quality is the black box tests executed by the test team.

Fixing code quality issues

Learn SOLID principles of coding and why they are important. Here are few resources to learn these principles

Off there principles one that impacts the code quality in long term is the SRP – Single Responsibility principle. Many times developers confuse what can be called as Single. Easy test for the same can be things that change together stay together. Another is to look at it from testing perspective. Does the block of code needs to be tested separately? Can something go wrong when executing the lines of code. In such cases take the code block as separate method. SRP can be applied to methods and it can also be extended to class and component design.

For a given process preferred style of coding could be to have a top level method which defines the process flow and for each step write a separate method. This way the code gets split at least at each conditional statement. This also brings down the cyclomatic complexity mentioned in the code metrics section.

Code Metrics is one of the most useful tools which is part of Visual Studio. It is not as enhanced as tools like NDepend, however it is extremely useful for day to day tracking of where the code is likely to have quality issues. Two key parameters to watch for are lines of code for any entity like class or method and Cyclomatic Complexity for any method. Typically any method with LoC about 30-40 and CC about 10 must be reviewed and should be simplified by way of refactoring. More about it can be found on following links:

Code Analysis is again a built-in tool of Visual Studio. It is integrated version of FxCop that many folks have used for earlier version of VS & .Net. Code Analysis depends on set of rules. These rules are categorized into groups like Performance, Globalization, Design, Security, Maintainability etc. There are many Microsoft defined rule sets available out of box in Visual Studio. However, teams can customize the rules applicable for their project. E.g. if the application is developed for only single language and culture, team can decide to ignore globalization rules. Code analysis can be configured to run on every build and the rules can be modified to raise a warning or error on failure to meet them.

Improve readability using StyleCop, a tool distributed free of cost by Microsoft. Style cop works similar to Code Analysis by using a predefined set of rules. StyleCop can also be integrated with Visual Studio using StyleCop VS Extension from the gallery.

How to include the code quality check in your daily routine

Select Code Analysis rule set suitable for your project. Making running code analysis at end of every build as default for all developers. Running code analysis after a lot of code is written has a negative impact on developers as it normally shows a lot of errors which creates an impression that the code needs to be rewritten. However running the analysis on every build gives early warning to developers before checking the code in to source control. This helps the code to remain high quality. Time required on every build: about 2 minutes. Assuming there are 5 major check-in per day in your source control, total time taken 10 mins per day

Run Code Metrics at least once a day across the whole code base. This helps the leads in identifying where the team is writing complex code or large code blocks. Code metrics is comparatively time consuming process and needs a successful build. Long methods and complex code are easy to smell for individual developer. Individual developer can keep refactoring & simplifying the code once the understanding of how it helps in long run. Typically teams will take about one iteration to realize the importance of Cyclomatic Complexity. The first aha moment comes when there is a change request for any reason. Time required every day for medium sized project: about 10 minutes to run and export the results to MS Excel.

Git Servers & Providers

Git Fundamentals for developers experienced in TFVC

TFVC (Team Foundation Version Control) is a centralized version control. Meaning there is a central repository from which we create a local working folder and keep the local folder in sync with the central repository. We check-in and check-out files in the central repository.

Git on the other hand is a distributed version control system. You don’t need a central repository to create a git repository. You can initiate a repository in any folder by calling command git-init in that folder on command prompt.

Git also works with central repositories called as remote repositories. However, all the local repositories always have a full copy of the repository and make it easy to work in an offline mode.

There is no explicit check-out process in Git. Git Commit is equivalent of Check-in of TFVC. However the Check-in process of Git is two step. First we commit the changes to local repo and then push those changes to remote repository. “Get latest version” command of TFVC can be compared with pull command of Git. We can also perform a sync operation which is a combination of Pull and then Push to the remote repository.

Two step vs single step process

In case of Git, files are not automatically added to local or remote repository. We need to explicitly add new files to the repository through commit. Also there is no need to push the commits (A term used for a group of files committed to local repo) immediately to the remote repository. We can club multiple commits in one push request.

Conflicts are resolved during push and pull operations. You can merge the conflicts but need to commit and push them again. This way there are separate records of original planned commit and merge. This is helpful in case of branching and merging.

Branching in TFVC and Git are different in nature. TFVC uses folder replica for branching. To change the branch, one needs to start working in different folder. However, Git always has one branch active within same folder. Once we change the branch, Git updates the local files to those in the new branch. So it is very important in Git to keep watch on current branch as one may accidentally do the changes in different branch. However, this makes switching branches in Git very easy.

Code Review on TFVC & Git

Code Reviews are conducted on TFVC by creating temporary check-ins called shelvesets. Developer creates the shelvset and shares it with the reviewer using a link/ id.

Git on other hand has a concept called pull request. Developer makes changes in his branch and creates a pull request on the branch where the changes are to be merged. The reviewer reviews the changes and either accepts or rejects the changes.

Git by default does not force to submit changes only through pull requests. However, git server products can enforce such policies on branches. In TFS we can set up branch policy to allow changes only through pull requests.

I always give one example to .Net developers. Specially those who came into profession after .Net 2.0. In the initial versions of Visual Studio for .Net (2003, 2005) there was a prominent component bundled with the installer. It was the Obfuscator. We did not understand initially what this was for. Later on when we came across decompilers which converted MSIL code into a C# or VB.Net code, we realized that the obfuscator was very critical to protect IP of your code. It was off course not foolproof solution. But it made the job hard. Many young developers today are not aware of this concept. So I introduce them to this concept first and then I tell them that although this is important but you will not need it if you continue write code the way you write today. I call this as natural obfuscation. Even without obfuscation, the code is extremely hard to understand.

Natural obfuscation is a technique to protect your IP by writing the code that is naturally hard to read and harder to understand. Only the developers who wrote the code should be able to fix it.

Whenever I do code reviews I came across suffixes such as BAL, DAL and UI etc. Every class in a particular layer is suffixed with these terms. So the namespace goes like this: CompanyName.ClientName.ProductName.BusinessLayer and the class name goes like CompanyName.ClientName.ProductName.BusinessLayer.ClassNameBAL or ClassNameDAL.

The biggest challenge an Architect faces is to move the developers from the concept to actual implementation where architectural terms/patterns do not make the naming overly complex to read.

While describing the architecture, we define layers, within layers there are patterns and concepts. If the team does not have any architect for some reason, the team will typically split the project into three layers: presentation, business and data access. I have asked many candidates during interviews about why these three layers and they tell me about all bookish concepts like loose coupling etc. But in reality

if you have not defined interfaces and coded against those interfaces and if you have not implemented DI, you really will never have loose coupling. It is just separation of concerns.

But many a times that is name sake. The business logic is really spread across multiple layers, at minimum presentation, business and stored procedures. Coming back to the layers, to show that they have implemented these layers, the developers ensure that the namespaces or class names have these acronyms in there.

In another project we defined three primary components of the business layer namely business processes, core components, business rules. Idea is straightforward. Core components are responsible for single function, rules allow you to check for conditional logics and processes orchestrate the business using core components as individual steps and rules as conditional statements to change the flows. The team understood the concept very well. The namespace conversion of this was very interesting. There were three different assemblies.

IMHO, except for business rules, all other entities can be part of single assembly. What should be general rule for creating a separate project/assembly? I feels things which change together should stay together. In the above case unless you are sharing Entities across multiple layers/ components (E.g Services and Business), all other components can be part of the single assembly. Typically business processes will be exposed to higher layers via interfaces.

There is another issue with naming. This is related to names of classes and methods. In OO it is natural convention that class names are nouns and method names are verbs. However many newbies use verbs as class names and nouns as methods such as

Now on the usage side this looks good. BookConferenceRoom().ForSlot(...) However this is more of a functional language syntax. In OO thos should look like BookingManager.BookConferenceRoom(...)

Lastly another issue which often annoys me but may be many people think it prevents bugs is that many developers use prefixes of suffixes for their variable or paramter names. E.g. a text box is named as txtFirstName, a string parameter is named as strDescription, an instance of a class is named as objPerson or oPerson. When .Net was first released, Microsoft released a guideline to avoid all such hungarion notation since all the languages are strongly typed. The recommendation was to use FirstNameTextbox, description, person with exactly same casing for the previously mentioned names. In case if you make error in assiging wrong typed value, compiler will let you know immediately. So I am all for avoiding hungarion notations. I am open to hear if you have another view.

I have listed down some of the issues related to naming conventions in .Net but there are many more issues which make reading & understanding the code difficult. Some of them are related to coding principles, which I will discuss later. But I would like to hear more from the readers about the challenges they face while reading someone elses code.