Tuesday 18 August 2020

A study in bad naming


Recently, while working for one of my clients on someone other's code I spotted the following definitions of members in some class:

What's that???!!!  Can you see it? Are you as much shocked as I was on its first sight? You are not? OK, I will explain my abomination - every declaration had to be commented! And that's not enough - the comments are essential in understanding the purpose of each member!

Why I do not like it, you might ask? Well, I'm lazy and commenting each declaration seems like too much work... You might ask 'why?' again - well, if I would like to add some definition to this code, I'd have to write a comment as well in order to maintain the (arguably flawed) coding style! I'm only a service provider for my client and wouldn't like to break this code's look and feel.

As you might know I'm not a friend of the newfangled "no comments" politics, but this is an example where we could definitevely use some of it. But why's that so bad again? It's because you could fix the example code with careful usage of naming* and it's even not that difficult! 

Let's have a go at it:
  /* State m_state => */ State m_dateValidity;
here we don't have to include "state" in the name, as it is already stated as the domain of the variable.
  /* QAction* m_action => */ QAction* m_datePickerPopupAction;
Here, however, we somehow inconsistently included "Action" in the member's name mainly because of my vague gut feeling. In this instance we could indeed try to improve the naming by considering shortening it, maybe even to m_datePickerAction;
  /* QDate m_date => */ QDate m_parsedDate;
A no-brainer.
  /* QStringList m_parseFormats => */ QStringList m_dateParsingFormats;
Another one!
  /* bool m_twoYearIsPast => */ bool m_treat2DigitYearsAsPast;
Here we could try to make the name a little better - maybe m_twoDigitYearsInThePast
  /* QDate m_minDate => */ QDate m_earliestDate;
As minimum and maximum are't commonly used with dates.
  /* QDate m_maxDate => */ QDate m_latestDate;
Idem.
  /* bool m_justFocused => */ bool m_focusReceived; // used for workaround ABC... 
Here I see the only place where a comment is needed.

So, what do you say? It wasn't even that difficult. Arguably the result isn't perfect but it's a lot better than the original. Maybe it's even good enough? 

So watch your names, they are important - because code readibility is important!

--
* As they say -"there are only 2 hard problem in comp. sci. - cache invalidation and naming things"...

No comments: