Code Naming Tips

Photo: Patrick Tomasso - Unsplash

Code style is like fashion style. It is very opinionated and covers a spectrum of good to bad taste with ease. Ask 10 developers how to name things in code and you will likely get 30 answers.

The main thing to remember, as I often state here, is that you will read code many more times than you write it. So, it figures that if you use well defined and clear names in your code it will make it easier when you come back and read it.

Below are a handful of suggestions that I try to adhere to in my own code and which I guide my team to. I do not suggest this is the perfect approach but I have adjusted it over the years through many projects.

1 - If your language has namespaces, use them to segment your code.

An odd opinion to share first but let me explain. Your variable and object names should be short and descriptive. What they should not do is repeat the namespace. Nor should they be tied to a feature by name or be used to group related objects. I have seen namespaces ignored many times in projects which leads to code like

namespace Foo
{
    public class FooReader{  }
    public class FooParser{  } 
    public class FooCalculatorService{  } 
    public class FooDatabaseService{  }  
}

There is no benefit to prefixing Foo to each class name. Use the namespace functionality of your language to segment and organise your objects.

2 - Names should be as descriptive and short as possible.

A bit of an oxymoron there but bear with me!

var o = new NamespaceFeatureFunctionPattern();

// lots of code

if(o.SomeProperty == true)
{
    // do something
}

So a variety of the above code is very common. Short variable names and abbreviations were very common back before I started. A raft of reasons including memory and screen real estate caused their use. But, this is 2017 and I am typing this on a Full HD screen with 32gb of RAM. So those reasons are no longer valid (in most cases). The trouble with very short or abbreviated names is that it makes reading the code really hard. If you use lots of o, a, j, i type variable names then very soon you won’t be able to determine which relates to what type. There are times that short and abbreviated names make complete sense such as counters in loops or indexes.

At the other end of the spectrum is the really, really, long variable names. I worked with a dev once that was so descriptive you would have to scroll to read the whole variable name. I am not joking! This is only just as bad as short names as it makes the code hard to follow and format. Although you do know what the variable relates to.

So how can we improve the code above? First, we need a short and descriptive name for o. We also need to name our object better.

using Myproject.Facades;
var gcalFacade = new GmailCalendarFacade();

// lots of code

if(gcalFacade.SomeProperty == true)
{
   // do something
}

As per point 1, do not include the namespace in your names. I have added a using directive in the code above. Then I have named the object GmailCalendarFacade. It is clear what the object is and indicates any pattern it is based on. The variable is then a shortened name that clearly states what it is referencing. The approach you take can vary based on the items you are referencing together.

3 - Code should read like prose.

Given we read code so much more than write it, it makes sense to make it as easy to read as possible. So, write your code like you would write a story.

A great test for this is to debug some code with a non-developer. Get a business side colleague to sit and run through your code and see if they can follow it. If you have written it clearly enough that they can then you have well named and maintainable code.

4 - It is OK to negate a condition in a name

Some guides you read state that you should not use names such as IsNot or DoesNot. I do not agree and would say you are still aiming for point 3 above.

When did your favourite author last use != in a book?

5 - Use your languages style guide as a guide

All languages have style guides created, usually by the community. Some guides are more strict than others (yes PEP 8 I am talking about you). These guides will offer specific naming and formatting advice for your language. Sometimes a language has more than one community written guides. Pick one, stick to it and remember that consistency is the key. This is especially true in teams.

I have written this based on my experience on many projects built using .Net, Python and Ruby. I am sure that the advice might not carry over to some languages. Functional languages especially put in place different naming conventions. These may recommend shorter more math base naming conventions. The important thing is to ensure your code is clear and concise and following an agreed standard.

If you’d like to discuss my thoughts here then message me via twitter or email.

Incorrect Use Of .Net Garbage Collector

Photo: Unsplash

I received a Pull Request a few days ago which included the following 2 lines repeated in 2 methods in a class.

GC.Collect();
GC.WaitForPendingFinalizers();

Now if ever a couple of lines would make me sit up and take notice it would be these two.

Some context. The code I was reviewing was a console application which was to run as a web job on Azure. The aim of the job was to join two sets of data and pick out the largest value for each month. The results were then persisted back to the database.

I started to read through the code. Most of it was boilerplate for how we build web jobs. The crux of the whole application was in a service processing class. When I got to the bulk of the service class I found something like:

var dataList = GetDataToBeProcessed();
while (dataList.Count > 0)
{    
  var subDataList = dataList.Take(500);
  RunCalculations(db, subList);
  
  // some other code .....
  
  GC.Collect();     
  GC.WaitForPendingFinalizers(); 
}

I peeked into RunCalculations() and found:

var entity	= _unitOfWork.GetRepository<Entity>().GetByKey(object.EntityId);
var fromDate = new DateTime(model.Year, model.Month, 1);
var toDate = new DateTime(model.Year, model.Month, DateTime.DaysInMonth(model.Year, model.Month));
model.ValueToBeSet = entity.CalculateValue(fromDate, toDate, _unitOfWork).Value;
GC.Collect();
GC.WaitForPendingFinalizers();

(As an aside I hate the use of nullable fields in all but a very select set of circumstances. I will write them down one day).

I decided to run the application and see what it would do. The logic was not far off the spec but I wanted to see how fast it would process our data. Locally it would be touching about 300,000 records. So I set it off. I waited and I read some emails and realized it had still not run. Tldr; I left it for an hour. One of the things that I like to add in Web Jobs is a lot of console writing/logging. It really helps debug issues once on Azure. There was none in this initial version of the application so I was wondering if it was even running still.

I killed off the app and re-read the code and instantly saw one of the issues. During the loop of the RunCalculations() method the developer was using a UnitOfWork class to pull back an Entity record on each iteration. That meant for each iteration we had a database call.

It was at this point I got the developer to look at the code with me. I started by asking them to walk me through the code. I wanted to get an idea of how they saw the code and how it executed. When they were finished I asked how long it took to run on their machine.

“Oh, the first time it will take a long time as there is a lot of data to process. I had problems running it on my machine so I decided to add code to make it process faster. The GC calls free up memory and the Take command batches the processing into smaller pieces.”

Now I had an insight into why the code was written in such a way. After discussing the approach they had taken I asked the developer if they knew how many database calls their code made. They did not. Out came SQL profiler and we run the job again on my machine. They saw straight away the issue.

The number of database calls was only one of many issues. The query used to return the bulk of the data was not filtering on any criteria. They were bringing back all of the data in the database. They had added logic to entity classes (which by choice we keep anaemic).

We spent a significant amount of time pair programming to refactor the service class. When doing exercises like this with a developer I lead with questions the whole way. The 5 Whys method comes in handy in this situation.

“Why do You have GC calls in your code?” - “Because the memory gets eaten up by the objects required for the calculations”. “Why do the calculations have so many objects” - “Because the calculation needs data and logic”. etc …

After refactoring chunks of code as we progressed through the questions our initial method ended up looking like:

processingDataList = processingDataFactory.GetPendingDataToBeProcessed();
RunCalculationsOn(processingDataList);
var dataToImport = processingDataList.Except(_failedItems.Keys.ToList());
tableGateway.ImportData(dataToImport.ToList());

and the RunCalculations() Method ended up looking like:

Parallel.ForEach(processingDataList, (dataItemToProcess) =>
{
  try
  {
    var fromDate = new DateTime(dataItemToProcess.Year, dataItemToProcess.Month, 1);
    var toDate = new DateTime(dataItemToProcess.Year, dataItemToProcess.Month, DateTime.DaysInMonth(dataItemToProcess.Year, dataItemToProcess.Month));
    
    using (var calculation = new Calculation())
    {
      var result = calculation.RunFor(dataItemToProcess.MeterId, fromDate, toDate);
      
      if(result.HasValue)
          dataItemToProcess.MaxDemand = result.Value;
      else
          _failedMeterMaxDemand.Add(dataItemToProcess, new Exception("There was no data to be processed"));
    }
  }
  catch (Exception ex)
  {
    _failedMeterMaxDemand.Add(dataItemToProcess, ex);
  }
}

The orchestration method in the processing service is now nice and clean and has all behaviour injected (omitted for brevity) into it. It gets, processes and imports the data. Simple, clean logic. The RunCalcuaitos() method is also much improved. The calculations have been removed from the entity and are wrapped in their own, disposable, class. With a parallel ForEach loop, the data needed to be processed access prior to the processing rather than during it and no GC calls.

With the improvements made a rerun of the application resulted in all records being processed and imported into the database in under 1 minute. The developer’s machine never once looked like it was struggling and there was no need for manual GC collection.

If you’d like to discuss any of my thoughts here then as always please contact me via twitter or email.

String Calculator Kata

Photo: Philip Estrada - Unsplash

I wrote recently about using Kata’s regularly to practice and improve your skills. I have been using them with a junior developer to support his training plan. The latest kata we have added to his practice is the String Calculator kata from Roy Osherove.

The premise of the kata is really straightforward. There are 9 steps to fully complete the kata with a time limit of 30 minutes. The aim is to define a method that accepts a string of numbers and returns an integer containing their sum. As the kata goes on you have to improve the method to handle delimiters of varying lengths.

I have created a C# based solution for the kata which can be found on Github. I attempted to commit as I implemented each test or point of the kata. If you read back through the commit history you can see the approach I took.

The important aspect of this kata is to following the steps without reading ahead. Implement your tests and only the code that you need to pass the test. You can and should refactor as you go but only to ensure you are doing the minimal amount to pass the next test.

If you’d like to discuss anything relating to the kata or my solution then as always please contact me via twitter or email.

Moto G Bluetooth Issue

Photo: Andy Crouch

The Moto G is an amazing phone. I do not own one but my wife has owned every model up to the 4th gen. For around £150 you get a very reasonable phone which does exactly what my sons and my phones do for a lot more money. The camera is very usable, the memory expandable and games play really well. So the last 5 years or so have been plain sailing with zero support from me needed.

Until this weekend!

Google Play’s, Play Protect, notified her that it had found a harmful Bluetooth sharing app. Having lived with me for 20 years she knew that it needed to be take seriously and hit the Deactivate button. She then carried on without a thought about it. That was until she tried to connect to the car’s Bluetooth later that day.

Play Protect had actually flagged the Lenovo installed Bluetooth Sharing app as harmful. That took me the best part of an hour to find out from some serious Googling about. The solution is to

• Open the Settings app.
• Got to Applications
• From the menu on the top right of the screen select “Reset Application Settings”
• Then reboot your phone.

This will solve the issue. I am led to believe that many Moto G users have reported this to Google so it should not happen again.

If you’re having a simular issue and this solution does not work then feel free to reach out via twitter or email and I will see if I can help.

The Power Of .Net Generics

Photo: Dương Trần Quốc - Unsplash

Generics are a genuinely useful and powerful tool in .Net. Sometimes developers see part of the potential but not the whole story. This was the case in a recent Pull Request I reviewed.

The Pull Request in question related to some cleaning up that a developer had undertaken within our user management pages. These are standard lists of different types of user which can be filtered, exported to excel etc. The original developer of these pages had done a great Ctrl-C, Crtl-V for the first 2 different types. The developer of the Pull Request I was reviewing had set about to clean this up and used generics to do so. So far so good. The problem came when I noticed a raft of switch statements throughout two of the methods in the revised service. For example:

public MemoryStream GenerateExcelFile<T>(string searchString) where T : IUserManagementModel, new()
{
    var obj = GetUsersQuery<T>(searchString: searchString, page: 0, countPerPage: 0, sort: null, sortAsc: true);
	
    switch (obj.UserType)
    {
        case UserType.Customer:
            return _excelExportService.GenerateExcelFile(obj.UserMembershipList.Select(x => x as CustomerModel).ToList());
        case UserType.Supplier:
            return _excelExportService.GenerateExcelFile(obj.UserMembershipList.Select(x => x as SupplierModel).ToList());
        case UserType.Admin:
            return _excelExportService.GenerateExcelFile(obj.UserMembershipList.Select(x => x as UserProfileModel).ToList());
    }

    throw new Exception($"Unexpected UserType: {obj.UserType}");
}

When I see a switch statement like this I have two clear thoughts. One, this does not allow for easy further extension or modification and two, there is actually just one line of code that needs executing on different models so lets write the code to do that.

public MemoryStream GenerateExcelFile<T>(string searchString) where T : class, IUserManagementModel, new()
{
    var userQuery = GetUsersQuery<T>(searchString: searchString, page: 0, countPerPage: 0, sort: null, sortAsc: true);
    
    return _excelExportService.GenerateExcelFile<T>(userQuery.UserMembershipList.Select(x => (T)x).ToList());
}

This is a nice solution that allows for further IUserManagementModel based objects to be exported to Excel without this code needing to change. This is a real world example of Bertrand Meyer’s Open/Closed principle that I mentioned in my last post.

As I continued the review I came across the issue further as shown in the GetUsersQuery() method show below:

private UserQueryResult<T> GetUsersQuery<T>(string searchString, int page, int countPerPage, string sort, bool sortAsc) where T : IUserManagementModel, new()
{
    IQueryable<GenericUserObject> query;
    UserType userType;
    
    if (typeof(T) == typeof(CustomerModel))
    {
        userType = UserType.Customer;
        query = (from c in _context.Customer
                 where c.Deleted == false
                 select new GenericUserObject { Id = c.Id, Email = c.Email, FullName = c.Name, Name = c.Name });
    }
    else if (typeof(T) == typeof(SupplierModel))
    {
        userType = UserType.Supplier;
        query = (from s in _context.Supplier
                 where s.Deleted == false
                 select new GenericUserObject { Id = s.Id, Email = s.Email, FullName = s.Name, Name = s.Name });
    }
    else if (typeof(T) == typeof(UserProfileModel))
    {
        userType = UserType.Admin;
        query = (from a in _context.Supplier
                 where a.Deleted == false
                 select new GenericUserObject { Id = a.Id, Email = a.Email, FullName = a.Name, Name = a.Name });
    }
    else
    {
        throw new Exception($"Unknown Type: {typeof(T)}");
    }

    // some other code ...
}

Once again the multiple switch statements were replicating code on objects that were all based on IUserManagementModel. It seemed obvious that the UserType and query values could actually be returned from the models. So that’s what we did:

private UserQueryResult<T> GetUsersQuery<T>(string searchString, int page, int countPerPage, string sort, bool sortAsc) where T : IUserManagementModel, new()
{
    IUserManagementModel model = new T();
    UserType userType = model.UserType();
    IQueryable<GenericUserObject> query = model.GetModelQueryFor(_context);

    // some other code ...
}

This was achieved by modifying the IUserManagementModel interface:

IQueryable<GenericUserObject> GetModelQueryFor(DbContext dbContext);
IOrderedQueryable<GenericUserObject> ApplySort(IQueryable<GenericUserObject> query, string sortBy, bool asc);

(The ApplySort() method follows the same approach as GetModelQueryFor() to remove other code in the class that started off with a switch statement handling sorting the query result).

By the time we’d finished pairing up, the developer and myself were happy with the results. We’d removed the switch statements and realised more of the power of generics. That power is released by ensuring that you push all data and behaviour down to your abstractions. That way, your generic code can be written to interact with the abstracted type and not the instance of the objects.

If you’d like to discuss any of my thoughts here then as always please contact me via twitter or email.