« Back to home

Reading code as if it were a book

Daj się poznać

Sometimes we hear that good source code should be read as easily as a book. Let’s look at this code as an example:

public class EventService
{
 private string _eventTitle;
 private string _participantName;
 private Event _event;
 private Participant _participant;

 public void Process(string eventTitle, string participantName)
 {
   InitFields(eventTitle, participantName);
   CreateParticipant();
   CreateEvent();
   SaveEvent();
 }

 private void SaveEvent()
 {
   var repository = new Repository();
   repository.Save(_event);
 }

 private void CreateParticipant()
 {
   _participant = new Participant(_participantName);
 }

 private void InitFields(string eventTitle, string participantName)
 {
   _eventTitle = eventTitle;
   _participantName = participantName;
 }

 private void CreateEvent()
 {
_event = new Event(_eventTitle, _participant);
 }
}

The important part of this code is the method Process. Let’s see if we can read this method as a book.
InitFields: eventTitle and participantName
CreateParticipant
CreateEvent
SaveEvent

I think we can agree that you can pretty much read it like a book. When I run this sample app with Repository class like this:

public class Repository
{
 public void Save(Event @event)
 {
   Console.WriteLine($"Saved event: '{@event.EventTitle}' with participant: '{@event.Participant.ParticipantName}'");
 }
}

and the Main method like this:

public static void Main(string[] args)
 {
   var eventService = new EventService();
   eventService.Process("Some event","Some participant");
 }
}

I got this output in the terminal:

Saved event: 'Some event' with participant: 'Some participant'

Suddenly, the requirements have changed and the event should also have the place where it’s going to happen. A programmer gets this task Add the place to the event. They come to the code, look at it and write a method like this:

private void CreatePlace()
{
 _place = new Place(_placeName);
}

Of Course they add the fields _place and _placeName and change the signature of the Process method to this:

public void Process(string eventTitle, string participantName, string placeName)
{
//...

They must remember to init field _placeName in the InitFields methods and they must change the signature to this again:

private void InitFields(string eventTitle, string participantName, string placeName)
{
//...

Fortunately, this programmer has Reshaper. Actually, they are testing JetBrains’ Rider and ALT+ENTER did a lot of this work for them but it’s still a lot of changes. They also have changed the CreateEvent method to what you see below:

private void CreateEvent()
{
 _event = new Event(_eventTitle, _participant, _place);
}

Finally, the last thing to do is to use the CreatePlace method in the Process method , but the phone rings. After a 5 minute conversation, they return to the code and enter CreatePlace like this:

public void Process(string eventTitle, string participantName, string placeName)
{
 InitFields(eventTitle, participantName, placeName);
 CreateParticipant();
 CreateEvent();
 CreatePlace();
 SaveEvent();
}

Can you spot where the mistake is? The method Save in Repository was written by another programmer and now it looks like this:

public void Save(Event @event)
{
 Console.WriteLine($"Saved event: '{@event.EventTitle}', at '{@event.Place.Name}' " +
                   $"with participant: '{@event.Participant.ParticipantName}'");
}

So, the task completed and the place was added to the event. Now let’s run the app…this is what we get:

System.NullReferenceException: Object reference not set to an instance of an object

We get a beautiful System.NullReferenceException. Why? Because the programmer called the method CreatePlace after the method CreateEvent by mistake. The mutability of these private fields and the side effects of these Create… methods come back to haunt the programmer. Fortunately, the Place was required and the error shows up immediately but if it doesn’t, how do you know what happened? You can imagine many other possibilities to make a mistake with such kind of code. For example, you can forget to init fields in the InitFields method.

Believe it or not, I see this kind of code almost every day. Yes, I know someone would say with TDD you don’t see such kinds of errors but what if you work on a legacy project which doesn’t have tests for this specific fragment of code. What’s more, this way of writing code is also problematic when you want to refactor your code. For example, let’s say that CreateParticipant got so many requirements that you must take it out of EventService and put it in its own service. Then you must cut off not only CreateParticipant method itself but also private fields and private methods on which it depends and sometimes this is not so easy.

But we can do this better. Let’s get rid of all these private fields and replace them with local variables so the code look like this:

public class EventService
{
 public void Process(string eventTitle, string participantName, string placeName)
 {
   var participant = CreateParticipant(participantName);
   var place = CreatePlace(placeName);
   var @event = CreateEvent(eventTitle, participant, place);
   SaveEvent(@event);
 }

 private static void SaveEvent(Event @event)
 {
   var repository = new Repository();
   repository.Save(@event);
 }

 private static Participant CreateParticipant(string participantName)
 {
   return new Participant(participantName);
 }

 private static Place CreatePlace(string placeName)
 {
   return new Place(placeName);
 }

 private static Event CreateEvent(string eventTitle, Participant participant, Place place)
 {
   return new Event(eventTitle, participant, place);
 }
}

And now when you run the app you’ll see this in the console:

Saved event: 'Some event', at 'Some place' with participant: 'Some participant'

With the code like this, it is impossible to create Event without creating Place first. Now all these Create… methods can be made static. Is this code so hard to read? I don’t think so. A source code is a book with constantly changing pages. I cannot print it and forget about it. You must come back to some places and alter them constantly. It’s okay if you’re altering code you’ve written yourself, but if you work with code written by someone else, then you must be careful about what you do if you come across a situation like the example I shared.

It’s a very trivial example but you can get a feel for what I’m trying to express. This code won’t be read by a manager or other business people, it will be read by programmers and they can handle a drawback like local variables. The second example of code given can be even more readable for a programmer because they see the whole context for variables and know exactly where they are used. They don’t have to search for Find usages because they see the usages straight away. What’s more, this functional way of writing code helps in the maintenance of the application. Less side effects and less mutability makes your application more predictable and reliable, especially when multithreading is at stake.

Related posts:

Comments

comments powered by Disqus