Refactor C# code to become more expressive

Rafael Nicoletti

Rate me:

3.92/5 (7 votes)

26 Jul 2012CPOL

33.6K

Refactor C# code to become more expressive

Introduction

This article brings clarification on some small tips to keep your code elegant and clean.

Background

Recently i began as architect on a large e-commerce corporation. My mission is to optimize code, by using cutting edge technologies, code reviewing and code rewriting/refactoring.

The main purpose of this article is to present some techniques on how to rewrite code in elegant way.

Using the code

Refactoring and make use of coalesce:

/*  Coalesce: */
if (Request["search"] == string.Empty || Request["search"] == null)
    base.searchText = string.Empty; 
else 
    base.searchText = Convert.ToString(Request["search"]);
return base.searchText;
/*  Can be written as:  */
base.searchText = Request["search"] ?? String.Empty;

Inversion of condition (AKA early return):

/*Inversion of condition:*/
public void DoSomething(String url)
{
    if (!string.IsNullOrEmpty(url))
    {
        if (migrate)
        {
            // code here
        }
    }
    else
    {
        PopulatePage(); 
    }
}
/*  Can be written as:   */
public void DoSomething(String url)
{
    if(String.IsNullOrEmpty(url))
    {
	PopulatePage();
	return;
    }        
    if (domainMigration)
    {
        // code here
    }        
}

Avoiding double calls:

/*  Double calls:   */

if (string.IsNullOrEmpty(Request["location"]))
    url = (FacadeFactory.GetCatalog()).ValidateUrl(filter.Value, canonicalUrl);
else
    url = (FacadeFactory.GetCatalog()).ValidateUrlByName(Request["location"], canonicalUrl);

/*  Can be written as:  */
var catalog = FacadeFactory.GetCatalog();
if (string.IsNullOrEmpty(Request["location"]))
    url = catalog.ValidateUrl(filter.Value, canonicalUrl);
else
    url = catalog.ValidateUrlByName(Request["location"], canonicalUrl);

/*  or */
var catalog = FacadeFactory.GetCatalog();
url = if (string.IsNullOrEmpty(Request["location"]))
    ? catalog.ValidateUrl(filter.Value, canonicalUrl)
    : catalog.ValidateUrlByName(Request["location"], canonicalUrl);

Comparison simplification

/*  compare using Contains  */
if (store.Current == Stores.Starbucks
    || store.Current == Stores.BarnsAndNobles
    || store.Current == Stores.Amazon)
{
    //do something
}

/*  can be written as:  */
var stores = new[]{ Stores.Starbucks, Stores.BarnsAndNobles, Stores.Amazon };
if (stores.Contains(store.Current))
{
    //do something
}

Defensive programming against null objects:

/*  defensive programming   */
var categories = Platform.ECommerce.Services.GetCategories();

var location = Helper.StringHelper.RemoveSpecialChars(
                    categories.Where(a => a.LocationId == filter).FirstOrDefault().Name);
//line above: possible NullReferenceException

var someString = ConfigurationManager.AppSettings["urlSite"] + "/" + location + "/?" + Request.QueryString.ToString();

/* can be written as:   */

var categories = Platform.ECommerce.Services.GetCategories();

var location = Helper.StringHelper.RemoveSpecialChars(
    categories.Where(a => a.LocationId == filter.Select(c=>c.Name).FirstOrDefault() ?? String.Empty));
//safe property access

Points of Interest

By using small tips and techniques, the code readability gets enhanced and the maintenance makes less painful. Some parts of this code can be refactored (using filters, extension methods, etc), but in this case i need to make greater changes that impacts the system.

History

Jun, 21, 2012:
- Coalesce tips;
- Avoiding double calls
- Optimizing comparison
- Defensive programming against null objects

License

This article, along with any associated source code and files, is licensed under The Code Project Open License (CPOL)

Written By

Rafael Nicoletti

Architect

Brazil

This member has not yet provided a Biography. Assume it's interesting and varied, and probably something to do with programming.

Comments and Discussions

My vote of 5

Ramkumar_S12-Nov-13 4:35

Ramkumar_S

12-Nov-13 4:35

good work...

Thoughts

PIEBALDconsult27-Jul-12 8:34

PIEBALDconsult

27-Jul-12 8:34

Under "Comparison simplification" I recommend a HashSet rather than an array, especially as the list grows.

Re: Thoughts

Rafael Nicoletti9-Apr-14 4:02

Rafael Nicoletti

9-Apr-14 4:02

Thanks for the trick, I will rewrite this tip to reflect your opinion!

Avoiding double calls...

Paulo Zemek26-Jul-12 9:14

Paulo Zemek

26-Jul-12 9:14

In avoiding double calls, there is still the double call to Request["location"]

The best is to store the Request["location"] result into a variable and use the variable everytime.
In fact, avoiding duplicated code between if and else does not really improve performance (as it is executed once), but avoiding duplications that come before really entering the if and inside the if/else block is really important.
Also, the use of local variables may reduce the line lengths and make things easier to read.

Re: Avoiding double calls...

Rafael Nicoletti27-Jul-12 4:34

Rafael Nicoletti

27-Jul-12 4:34

Sure, using local variables improves readability and in some case, performance gains.
The main purpose is to avoid duplicated code than make performance improvements.
And... thanks for attention and mention the Request["location"], i forgot that Wink | ;)

My vote of 3

Philippe Mori26-Jul-12 5:20

Philippe Mori

26-Jul-12 5:20

Either the trick are trivial or discutable (not generally accepted as being a good practice).

And in many case, it does not worth modifying existing code.

Re: My vote of 3

Rafael Nicoletti27-Jul-12 4:37

Rafael Nicoletti

27-Jul-12 4:37

I agree with you. The case is that all samples are got from code reviews. This moment is perfect to suggest changes in code optimization.
This article is highly discutable.
But, thanks for the reply. Wink | ;)

My vote of 3

MadDoc8725-Jul-12 22:33

MadDoc87

25-Jul-12 22:33

you could give more detail.

My vote of 3

Klaus Luedenscheidt22-Jun-12 18:07

Klaus Luedenscheidt

22-Jun-12 18:07

Hope you will add more samples over time.

Think about your sample about "avoiding double calls". For me in a simple if / else it's absolutely legal to make the call the factory in both parts, because it's definetly called only once. If i have a more complex method where i need to make different calls to the factory object i would store an instance in a variable.

Re: My vote of 3

Rafael Nicoletti26-Jul-12 2:42

Rafael Nicoletti

26-Jul-12 2:42

Thats right.
But... if you're using a factory method, my understanding is that every call creates a new, clean instance.
Ill review the codes and try to split more with samples.
And thanks for the review!

[My vote of 2] Your tips are quite debatable, especially since you hardly give an reason for your advises...

Andreas Gieriet22-Jun-12 16:10

Andreas Gieriet

22-Jun-12 16:10

Hello Rafael,

please find my notes on your statements above.

Coalesce: that original example suffers two flaws in my eyes: 1. it does too much (converts a string to a string) and 2. as you show, could be done with coalesce operator. I use that ?? operator quite often (same for the ?: operator), but be aware that many people do not like that short circuit notation. It would be interesting why, though... Any comments from anybody?
Early return: very debatable. I do it if it is very clear and if the function is short enough to grasp this pattern at a glance. Your example, though, is a code smell, in my eyes. Reason: it confuses more than it clarifies. I would rather write
C#
```
public void DoSomething(String url)
{
    if(String.IsNullOrEmpty(url))
    {
	PopulatePage();
    } 
    else if (migrate)
    {
        Migrate();
    }
    else
    {
       // do nothing: no need to return a new page... (or whatever reason is given here...)
    }  
}
```
Note: the empty else with the comment is very important for the maintainer of your code...
Avoid double calls: this is poorly described from your part. What exactly is the issue? Looking at your suggested examples, you try to avoid calling a factory twice. But you don't care to call the index operator twice. What exactly is the trigger to do one but not the other? A consequent implementation would rather do the following:

cs"
```
private Url GetValidatedUrl(Filter filter, Url canonicalUrl)
{
    var catalog = FacadeFactory.GetCatalog();
    string request = Request["location"];
    return string.IsNullOrEmpty(request)
        ? catalog.ValidateUrl(filter.Value, canonicalUrl)
        : catalog.ValidateUrlByName(request, canonicalUrl);
}
```
First, put the operation into a function and secondly consequently don't call potentially "expensive" functions more often than really necessary. Potentially expensive functions are factory access, dictionary/hash-table access, find, sort, any linq call, etc.
Comparison simplification: I use that pattern too for statically stable list of items. More important: pack it into a function, e.g.
C#
```
private static readonly Store[] _preferredStores = new Store[] { ... };
...
private static bool IsPreferredStore(Store store) { return _preferredStores.Contains(store); }
...
if (IsPreferredStore(store.Current))
{
   ...
}
```
Advise: Simplify comparison be making functions that tell what comparison it is. The implementation of that function may than be more or less sophisticated.
Defensive programming against null objects: this is rather sloppy defined in your example again. It is not clear what your chained names mean. The basic rule is: avoid chains of accessing members and check before dereferencing, e.g. ~~a.b.c~~ is to be avoided. The other thing is, if the function RemoveSpecialChars can deal with a null name, it is ok to pass a null name. In your example, I would rather separate the calls, e.g.
C#
```
private static string GetLocationById(string id)
{
  var categories = Platform.ECommerce.Services.GetCategories();
  string name = categories.Where(a=>a.LocationId==id).Select(c=>c.Name).FirstOrDefault();
  return Helper.StringHelper.RemoveSpecialChars(name); // it is ok to pass a null arg
}
```
In your example you seem to focus on the quirks of Linq access where null objects linger around. So, the advise is to store each access where Linq may return a null object (e.g. FirstOrDefault() or alike) into a variable and then check that variable for null (or use coalesce, etc. to deal with that null object).

In any case, your tips are quite sloppy defined and I hope you improve with the upcoming parts... You need to explain what you try to achieve and on what you focus in the examples. Simply posting some code to show an alternative does not help anybody. Give advise *why* you decided to do an alternative way. Only with giving a "rule" or a reason why allows your co-workers (or the "rest of the world") to make independent decisions if faced with such situations in the future.

Cheers
Andi

Re: [My vote of 2] Your tips are quite debatable, especially since you hardly give an reason for your advises...

Rafael Nicoletti26-Jul-12 2:51

Rafael Nicoletti

26-Jul-12 2:51

When i first published this article, i received some warns that it would cause flames.
From your point of view, there are some gotchas in the code and a lot to be improved in this article.
All those code (the "before" samples) are production code.
The main reason for refactoring is to make the code cleaner and clearer.
About early return: Its very debatable, but, its a personal preference return as soon as i can. There are exceptions like you said, to make code clearer. Im using context samples, its not a rule to follow.

After all, thanks for the feedback. Ill review the article to optimize and make it simpler/clearer for you, reader!

Use Ctrl+Left/Right to switch messages, Ctrl+Up/Down to switch threads, Ctrl+Shift+Left/Right to switch pages.