C# Code Smells: Eliminate By Refactoring - LinkedIn

Agree & Join LinkedIn

By clicking Continue to join or sign in, you agree to LinkedIn’s User Agreement, Privacy Policy, and Cookie Policy.

Sign in to view more content

Create your free account or sign in to continue your search

Sign in

Welcome back

Email or phone Password Show Forgot password? Sign in

or

By clicking Continue to join or sign in, you agree to LinkedIn’s User Agreement, Privacy Policy, and Cookie Policy.

New to LinkedIn? Join now

or

New to LinkedIn? Join now

By clicking Continue to join or sign in, you agree to LinkedIn’s User Agreement, Privacy Policy, and Cookie Policy.

Skip to main content 10 min read

This post was previously published on TheRockCoder.com

If you are a bit like me then your first stab at any programmatic problem doesn't produce the most elegant or well-written code.

Granted the more experienced you are as a developer the more pre-written (and time tested) code blocks you have stored in your subconscious (or wherever such things are stored in your mind) which you can call upon.

Yet first attempts are not always pretty. And that's fine, it's part of the problem-solving process. But as a conscious developer, you need to take the time to refactor your code to remove bad, unelegant and potential buggy code - what is known in the industry as Code Smell.

Many new developers sometimes forgo the refactoring stage thinking they are saving time, especially when under tight deadlines. Little do they realise they (or some other unfortunate developer) will pay the price at a later date when the application needs to be updated or maintained.

Thus I wanted to write this post and list the first three code smells I look for when I start my refactoring phase.

I must mention code refactoring goes hand in hand with unit testing. i.e. you need to make sure the changes you are making are not introducing more bugs.

Unfortunately, Unit Testing is outside the scope of this post as by itself the subject is large enough to be the source of many books and blog posts. However, I encourage you to brush up on this technique and adopt it as part of your developing pipeline/process.

Nothing gives you more of a buzz than when all your tests show up as green.

Ok let's see those code smells:

1. Magic values

As one of my pet hates, magic values refer to code constructs where some constant or setting is hardcoded inside the method or class.

There is a lot of literature out there in the big wide world where authors present this particular code smell very simplistically. Although this serves the purpose of teaching the concept I believe care must be taken in applying the solution as a mere 'Replace Magic value with Symbolic Constant.'

Create a constant, name it after the meaning, and replace the value with it.

from Refactoring: Improving the Design of Existing Code by Martin Fowler

Let me elaborate.

When you scan your code and identify a magic value, you must decide how you are going to eliminate this particular smell.

Look at the following code examples:

public decimal EnergyInMass(int massInKg) { var e = massInKg * (299,792,458)^2 //Einsteins famous E = mc^2 equation return e } public decimal AddVat(Basket basket) { var total = basket.itemsTotal() * ( 1 + 0.21); // VAT calculation at 21% return total }

Yes, both examples reek badly. And no, comments don't cut it. Good clean code should be self-explanatory without the need for comments. (this should be your refactoring mantra)

For our energyInMass function, the magic value is actually the speed of light in a vacuum. i.e 3.00 × 108 m/s This is a universal constant and it will never change (unless we live in a computer simulation that aliens can tweak at will).

This sort of constant adheres to the 'Replace Magic value with Symbolic Constant'. method.

I would refactor as follows:

1. Create a global static class (UniversalConstants) where I would store this and other such constants
2. Replace the magic value in the EnergyInMass function with the defined constant.

public static class UniversalConstants { public const int SPEED_OF_LIGHT = 299_792_458; public const double GRAVITY = 9.80665; } public int energyInMass(int massInKg) { var e = massInKg * (UniversalConstants.SPEED_OF_LIGHT) ^ 2; //Einsteins famous E = mc^2 equation return e; }

We now know exactly what the calculation is doing. We could remove the comments and any middle school student would recognise Einstein's equation. i.e. our code is now self-explanatory.

We thus come to our second method, AddVat. This is a bit more interesting.

Here we have a magic value which according to the comments is the VAT (Value Added Tax) percentage being applied. Now VAT being a human creation is bound to change according to the political whimsies of the day.

Thus our previous solution (i.e. the constant value in a static class) would help clarify the meaning of the value but we would end up with a hardcoded value. Definitely not the dynamic application we need to provide.

Here is where you have to think carefully about how you are going to implement your solution.

I would refactor as follows:

1. If the VAT is not going to change often then the value could be stored in a config file that is read in on application startup. This value can be changed by a systems admin. Ideally, the value should be stored in a database that can be edited within the application itself.
2. Create a FinancialValues class that can be passed onto whichever class will perform the calculations. These FinancialValues class values are loaded from the config file or the database.
3. Pass the FinancialValues class by composition to whatever class needs to perform calculations with any of the values contained. Then replace the magic values within the methods with the required Financial Values properties.

public class FinancialValues { public decimal VAT { get; private set; } public decimal INTEREST { get; private set; } } public class Invoice { private readonly Basket _basket; private readonly FinancialValues _values; public Invoice(Basket basket, FinancialValues values) { _basket = basket; _values = values; } public decimal addVat(Basket basket) { var total = basket.itemsTotal() * (1 + _values.VAT); return total; } //.. other Invoice class methods }

We thus end up with code that is not only testable (as we can pass in our versions of the FinancialValues class and Basket class when testing). But code that is also extremely readable and loosely coupled.

2. Long statement methods

I'll ask you a question. Who or what do you write code for?

If your reply was; 'the computer', then sorry wrong answer!

You write code in modern programming languages for you and other humans to understand. Computers only understand 0's and 1's. This is what your code will eventually end up being after the interpreter or compiler has finished processing what you have written.

So write your code in a way that speaks clearly to you (and others) what its intentions are, as only in the most extreme cases do you sacrifice intelligibility for the sake of performance.

Nothing is uglier and maintenance-heavy than methods whose statements go on forever. Put it this way, if your method is more than twenty or so statement lines then you are teetering on the brink of bad odours. In fact, some developers advocate ten lines are already bad enough.

Look at the following code ("its wafting"):

public string Parse(string template) { var regExInstance = new Regex(_regexString, RegexOptions.Compiled); Dictionary<string, Func<string>> directives; if (_ignoreCase) { var comparer = StringComparer.OrdinalIgnoreCase; directives = new Dictionary<string, Func<string>>(_directives, comparer); } return regExInstance.Replace(template, match => { var fieldName = match.Groups[1].Value; var found = _directives.TryGetValue(fieldName, out var action); if (found) { return action(); } var originalValue = match.Groups[0].Value; return originalValue; }); }

Before you scream at me, yes, it's intentionally less than twenty lines of code. Bad (smelly) code can be written in as little as one line of code!

What I want to show is, other than the function name stating what it's trying to do, i.e. Parse, the code contents takes effort to read and understand and is already too long for my liking.

In this case, my preferred refactoring method is 'Extract Method' :

Turn the fragment into a method whose name explains the purpose of the method.

from Refactoring: Improving the Design of Existing Code by Martin Fowler

My first target is the block of code in lines 6 to 8. This code will return a new dictionary whose keys are case agnostic if the _ignoreCase flag is set to true.

So I extract this block of code onto its own method:

Recommended by LinkedIn

Cypress 14 Is Here!🎉 LambdaTest 11 months ago Unit Tests Ahmed Hosny 8 months ago Strategies for Effective Code Reviews Greg Davis 1 year ago private Dictionary<string, Func<string>> ConvertToCaseInsesitive( Dictionary<string, Func<string>> directives) { var comparer = StringComparer.OrdinalIgnoreCase; return new Dictionary<string,Func<string>>(directives, comparer); }

and replace the block in the Parse method with: (note the use C#'s ternary conditional operator)

var directives=_ignoreCase?ConvertToCaseInsensitive(_directives):_directives;

My second target is the code block at line 14.

This seems to be calling an inner method or delegate as part of the Regex.Replace function. I'm not too happy with leaving the Regex.Replace function itself, it doesn't (at least for me) communicate its intention well enough within the context of the method Parse.

I'll extract two methods then, one of which will be the inner or delegate function to the Regex.Replace function. This then becomes:

private string InterpolateTemplate(string template, Dictionary<string, Func<string>> directives) { var regExInstance = new Regex(_regexString, RegexOptions.Compiled); return regExInstance.Replace(template, match => GetNewValue(match, directives)); } private string GetNewValue(Match match, Dictionary<string, Func<string>> directives) { var fieldName = match.Groups[1].Value; if(directives.TryGetValue(fieldName, out var action)) return action(); var originalValue = match.Groups[0].Value; return originalValue; }

The First method InterpolateTemplate encapsulates the Regex.Replace function and is titled more appropriately. That is, it defines the actual Regex.Replace functions' objective.

The second method GetNewValue is the extracted delegated inline method previously attached to the Regex.Replace function. Once again we give it a name more akin to its mission.

Our Parse Method thus becomes:

public string Parse(string template) { var directives = _ignoreCase ? ConvertToCaseInsensitive(_directives) : _directives; return InterpolateTemplate(template,directives); }

We now have a very readable and much reduced in size Parse method. I'm a happy camper.

We could refactor and reduce even further by extracting into a method the directives conditional conversion. This is a judgment call you need to take whether the effort is worth the extra clarity it provides.

3. Complex Conditional Expressions

In my refactoring phase, conditional expressions are probably the next items in my sights after I've eliminated magic values and decomposed long methods.

If any part of your code can introduce bugs in your code, you can bet your cotton socks this is where the majority of those nasty critters hide.

Why? Because condition expressions are usually typed in at the same time your brain is crunching the problem through. Unfortunately, this logic is sometimes flawed, in particular when ands' and ors' are intertwined.

Complex conditionals usually appear when you are writing service classes that need to process data following or modelling business logic.

Let's see a smelly example:

public decimal CallCharge(string destination, int duration, DateTime timeOfCall ) { decimal rate=_peakrate; if((destination=="UK" || destination=="FRANCE" || destination=="BELGIUM") && timeOfCall.Hour>=9 && timeOfCall.Hour<=18) rate=_euPeakRate; if((destination=="UK" || destination=="FRANCE" || destination=="BELGIUM") && timeOfCall.Hour<9 && timeOfCall.Hour>18) rate=_euLowRate; return duration * rate; }

Granted the example given above is quite naive and simplistic, yet it could have very well been the product of an initial attempt to model telephone calls rates to different destinations and at different periods throughout the day.

Out of the gate, we have magic values peppered across the method and long if conditions which are bound to grow even more as countries are added in.

Most of the time I will refactor conditionals by decomposition (i.e. method extraction) similar to what I did for long methods.

An easy first target are the time comparisons. These can be extracted to their method as follows:

private bool InPeakPeriod(DateTime timeOfCall) { return (timeOfCall.Hour>=_startofPeakTime && timeOfCall.Hour<=_endOfPeakTime) }

Note that I have replaced our magic values with class variables which are set via the constructor or through properties.

Our CallCharge method thus becomes:

public decimal CallCharge(string destination, int duration, DateTime timeOfCall ) { decimal rate=_peakrate; if((destination=="UK" || destination=="FRANCE" || destination=="BELGIUM") && InPeakPeriod(timeOfCall) rate=_euPeakRate; if((destination=="UK" || destination=="FRANCE" || destination=="BELGIUM") && !InPeakPeriod(timeOfCall) rate=_euLowRate; return duration * rate; }

Ok let's deal with the countries list

As before we extract the test onto its own method:

private bool InEu(string country) { return (_euCountries.IndexOf(country)>-1); }

As before we have eliminated the magic values for countries by having these added to a List type which is read in when the class is constructed or set via a property.

We refactor our CallCharge for a second time to produce the following:

public decimal CallCharge(string destination, int duration, DateTime timeOfCall ) { decimal rate=_peakrate; if(InEu(destination) && inPeakPeriod(timeOfCall) rate=_euPeakRate; if(InEu(destination) && !inPeakPeriod(timeOfCall) rate=_euLowRate; return duration * rate; }

And thus our conditional's intention is crystal clear.

Ok, one last refactor:

public decimal CallCharge(string destination, int duration, DateTime timeOfCall ) { decimal rate=_peakrate; if(InEu(destination)) if(inPeakPeriod(timeOfCall)) rate=_euPeakRate; else rate=_euLowRate; return duration * rate; }

Now compare our original conditional example with our refactored final one.

Tell me which one you would prefer to encounter if you needed to maintain this code many months down the line.

I know which one I would!

Final thoughts

I can not emphasise enough the importance of taking time to refactor your code once you have a working first draft.

Always bear in mind (as I have mentioned earlier) you write code for yourself and your fellow humans, not for the benefit of the computer. That role is the responsibility of the interpreters and compilers who will optimise the pants out of anything you throw at them.

You can not go wrong if you follow simple SOLID principles like DRY (Do not Repeat Yourself) and SRP (Single Responsibility Principle ). Basically extract repeating code onto their own methods and ensure methods or functions only do one thing but do it well.

Finally please note I have only scratched the surface on the techniques you should use to make your code extensible and maintainable.

Refactoring to improve the design of your code goes well beyond what I have covered and I encourage you to get hold of and study the book by Martin Fowler; Refactoring: Improving the Design of Existing Code.

George Gaskin is a freelance developer with more than thirty years of software development experience whilst working in the telecommunications industry. He is the author of TheRockCoder.com blog.

Like Like Celebrate Support Love Insightful Funny Comment

• Copy
• LinkedIn
• Facebook
• X

Share 5

To view or add a comment, sign in

More articles by George Gaskin

• C# fluid interfaces: the good and (maybe) the bad and the ugly Jul 28, 2021

  C# fluid interfaces: the good and (maybe) the bad and the ugly

  This post is an extract from a blog post previously published on TheRockCoder.com If you have used LINQ then you will…

• C# Composition vs Inheritance: OOP’s greatest dilemma Jul 15, 2021

  C# Composition vs Inheritance: OOP’s greatest dilemma

  This post was previously published on TheRockCoder.com In the world of Object-Oriented Programming (OOP) you may have…

  5 2 Comments
• C# interfaces: they can save the day! Jul 8, 2021

  C# interfaces: they can save the day!

  This post was previously published on TheRockCoder.com Interfaces can be one of the most useful constructs in the C#…

  2

Others also viewed

• Automated Code Reviews: Top 5 Tools to Boost Productivity

  Huenei IT Services 9mo

• How to Improve Code Quality and Reduce Bugs with AI-Powered Static Code Analysis

  Metizsoft Solutions Private Limited 5mo

• Make your test automation better with abstractions! 🤖

  Ivan Karaman 1y

• Clean Code in C#: 10 Habits That Changed My Projects and My Career

  Sandeep Pal 3mo

• The Art of Detecting Code Smells: A Developer's Guide to Better Software

  Frank Burcaw 12mo

• Freshen Up Your Codebase: The Essential Guide to Refactoring!

  Adhithi Ravichandran 1y

• Why should we standardize our delivery?

  Juan Pablo Bosnjak 9y

• Behavioural Unit Testing

  Andrew Walker 2y

• Mastering TDD with TypeScript: A Simple Calculator Example

  Stuart du Casse 9mo

• Is Clean Architecture messy?

  Jacob Ågård Bennike 1y

Show more Show less

Explore content categories

• Career
• Productivity
• Finance
• Soft Skills & Emotional Intelligence
• Project Management
• Education
• Technology
• Leadership
• Ecommerce
• User Experience
• Recruitment & HR
• Customer Experience
• Real Estate
• Marketing
• Sales
• Retail & Merchandising
• Science
• Supply Chain Management
• Future Of Work
• Consulting
• Writing
• Economics
• Artificial Intelligence
• Employee Experience
• Workplace Trends
• Fundraising
• Networking
• Corporate Social Responsibility
• Negotiation
• Communication
• Engineering
• Hospitality & Tourism
• Business Strategy
• Change Management
• Organizational Culture
• Design
• Innovation
• Event Planning
• Training & Development

Show more Show less