Commit Messages

As a developer, you need to “commit” your work into source control. When you do this, you need to provide a message that describes the change. A bug fix or enhancement can consist of a few commits. When you use a source control like Git, you can even “rewrite” the “history” by using squash/rebase commands.

When I am writing code for my job, I often put a bit of thought into the wording to make it clear and professional. If it is for a personal side-project, I sometimes write meaningful messages, but sometimes I may group together multiple fixes, then come up with something generic like “Bug fixes”, or might commit more experimental work-in-progress under a name like “attempt” or “x feature part 1”.

It’s quite frustrating though to see colleagues write generic messages like “bug fix” which doesn’t describe what it is fixing, or how it is fixing it. Seeing messages littered with spelling mistakes is also annoying and unprofessional.

Examples include:

“EventNotificationBugFix After Resolving James’ Comment”
“bug resolved”
“Dev test changes from tester” (literally what does that mean?)
Updated the findCorrectCodeSp to findCurrectCode.
Taken off completly the fix Amanda done for the fix for 25477 and fixed. Included the fix for 8017 as well
Fix for SQLCrop issues (should be SQL Cop, our Code Analysis)
Fioxed further typos (ironically creating more typos when fixing typos)
fixed the failed unit testes (testes rather than tests. Brilliant)
“Post Christ’ comments re coding standards” (it’s the second coming of Christ to teach us coding standards! They meant to write Chris.)

There was a guy who worked in our short-lived Scotland office who sounded like an absolute nutcase and I have never seen someone not take their job seriously like this:

instructions unclear, got dick stuck in tfs
what a waste of eyes
but fuck wasps
be nice to bees
what if there were landmines in dinosaur times

A colleague recently showed me this website https://whatthecommit.com/. I don’t know if they are based on real messages, but it shows you a new message every time you refresh. Most are pretty basic along the lines of “does this work”, but there’s some more outlandish ones if you persevere and refresh many times.

One of my team members recently submitted a change that was labelled “Crash when cancelling out of the dialog”. That describes the bug that he fixed, rather than what he fixed. Another team member provided the following good advice:

The change looks good, but your commit message isn’t describing the change. Could you just reword it so that it completes the sentence “When applied, this commit will…” please. Another way of looking at it is that every commit should read like an instruction in a to-do list. I’d use something like “Return empty collection if user cancels out of the dialog”.
Senior Developer

A stricter idea of commit messages is described here: https://www.conventionalcommits.org/en/v1.0.0/

One of our Principal Developers loves linking people to this guide https://cbea.ms/git-commit/. One interesting idea is to “Use the imperative mood in the subject line”, whereas most people would write in the past tense.

Having a clear history of file changes helps you when it comes to finding which change caused an issue, and also gives you a better understanding why it changed.

Bug: Identification number mismatch

Recently, a developer had fixed a bug with title similar to “Tax Number is not populating in Save Registration dialog”

The original code looked like this:

public CarIdentification CarIdentification1
{
   get { return _carIdentification1; }
}
 
public CarIdentification CarIdentification2
{
   get { return _carIdentification2; }
}

if (!string.IsNullOrEmpty(transaction.TaxNumber))
{
   _carIdentification1.Value = transaction.TaxNumber;
}
 
if (!string.IsNullOrEmpty(transaction.RegistrationNumber))
{
   _carIdentification1.Value = transaction.RegistrationNumber;
}
 
if (!string.IsNullOrEmpty(transaction.NewRegistrationNumber))
{
   _carIdentification1.Value = transaction.NewRegistrationNumber;
}
 
if (!string.IsNullOrEmpty(transaction.NewTaxNumber))
{
   _carIdentification2.Value = transaction.NewTaxNumber;
}

So a quick glance at the code and you see that 3 of the 4 statements are using “_carIdentification1” then the last one sets _carIdentification2. So without putting much thought into this, I would expect one of the first 3 should actually be using _carIdentification2. But what does 1 or 2 actually mean? Are they representing TaxNumber and RegistrationNumber, or is it representing Old and New?

If it is the former, I think _carIdentification2 would represent TaxNumber so the first one was wrong.

If the latter, then I think _carIdentification2 would represent New so the third one is wrong.

I do know that the concept of TaxNumber was added into the system later on, so most likely it would be: “_carIdentification2 would represent TaxNumber”

However, the developer, Govind had changed it to the following:

public CarIdentification CarIdentification1
{
get { return _taxNumber; }
}
 
public CarIdentification CarIdentification2
{
get { return _registrationNumber; }
}

if (!string.IsNullOrEmpty(transaction.TaxNumber))
{
   _taxNumber.Value = transaction.TaxNumber;
}
 
if (!string.IsNullOrEmpty(transaction.RegistrationNumber))
{
   _registrationNumber.Value = transaction.RegistrationNumber;
}
 
if (!string.IsNullOrEmpty(transaction.NewRegistrationNumber))
{
   _registrationNumber.Value = transaction.NewRegistrationNumber;
}
 
if (!string.IsNullOrEmpty(transaction.NewTaxNumber))
{
   _taxNumber.Value = transaction.NewTaxNumber;
}

So they have named the fields _taxNumber and _registrationNumber which is much clearer. Notice though that they have deemed carIdentification1 to be _taxNumber which means they are saying the second, third, and fourth statements were wrong!

An additional thought: if a “transaction” comes into the system, what fields do you expect populated? if RegistrationNumber and NewRegistrationNumber are populated, then _registrationNumber will initially be set to RegistrationNumber, then instantly changed to NewRegistrationNumber! So I’d say this logic needs to be if/else so it only sets _registrationNumber once and _taxNumber once.

I point this out to the developer, but I find a large number of Indian developers just seem to respond with a sequence of meaningless words and you have no idea what they are thinking.

Me:

if NewTaxNumber and TaxNumber are specified, won't it just use NewTaxNumber?


Govind:

taxNumber identifier has to updated with new taxNumber value for transactions


Me:

if it is intentionally written this way, it should be if/else

and if it wasn't intended that way, then there is a bug here, similar to the one you are fixing

They ignored me and checked it in. I don’t know if just the original code was mad, or if this new code is mad too. Let’s hope it doesn’t cause more problems when it goes live.

Example of a bad project led by a Principal Developer

Intro

One problem we often talk about at work, is that software projects are sent to be Code Reviewed a month before release, and they often get torn apart. This is because many people at the company don’t seem to like doing code reviews, or aren’t good at critiquing code.

As the project goes along, each check-in requires review but the review is done within the team. When the project is complete, and needs to be merged into the Release branch, the review is done by people outside the team (by people of higher skill, or by people who review under more scrutiny).

It’s still strange when the project is led by a senior, but is then torn apart at the final Code Review stage. Or in this case, that I am covering today – Gerald, a Principal Developer; someone who you would consider an “expert”. 114 comments were left on this particular project.

Some comments were coding standard violations (but given the Principal has several years experience working here, it was hard to believe he overlooked them), general bad formatting, code comments left in code, loads of mutable classes (bad design, but often very easy to fix), then a few classes spamming the error log (we have a separate problem which another development team are trying to fix – about monitoring storage too high, mainly down to excessive/unnecessary logging)

Problems

Excessive Logging

Me
Is this creating 6 log entries per call?

Gerald 
Yes , maybe even 7, but that's if the config value is set to debug. We have turned off by default

Sean
How do you change the logging level? Do you need a new release to change it? What is your plan for changing it?

Me
shouldn't it just create 1 row with all the information in?

A single entry would be good, I point out it is excessively adding 6 rows per call, and he is like “no! you are wrong, it is worse than that!”

In another part of the code, Sean flags up the same problem. Weirdly, he then agrees it is a very bad thing to do, and his tone sounds like it is an obvious thing. Wasn’t he leading the team? why didn’t he notice the problem during the months he was working on it?

Sean 
Won't this write a lot of entries to the Error Logs?
We are actively trying to stop that

Gerald 
the audit logger can be turned off.
I agree here though we could not write to monitoring if there's no records

Sean 
There's an ongoing project to stop that amount of unnecessary, unused and ignored things to the error logs.

Gerald 
Agreed. especially from the client. because that clogs up networks - a lot worse than this.

Does the code work?

//Would this work? Or how do we get the organisation ID?

He left this code comment in the code he sent to review. If the comment is doubting that it works, what confidence do I have? Instead of writing any words, I send a simple emoji. 👀

Gerald 
Not sure what this comment means, could you please elaborate

Sean 
Might be the fact that there's a question in the code asking if it works or not.
What a cliff-hanger.... Well does it? 😄

Gerald 
I'll remove the comment. Thanks for clarifying Sean.

Gerald 
Removed the comment. I think the team was not aware that we can get the organisationid from the usercontext.

Why did it need clarifying? Did he even read the line I commented on? why do we need to explain that a comment like that shouldn’t be in code we send to production?

YAGNI

There’s a software development principal known as YAGNI: You Ain’t Gonna Need It. It’s about only writing features you know you need. If you write features that aren’t used, any code changes you make need to support this feature. It’s a maintainability problem. There were actually a few instances of this in his project, although it was a case of maintaining obsolete code.

Changing obsolete code just in case it is used, but yet it wasn’t tested because it wasn’t used…

Sean 
This looks like dead code...

Gerald 
Yes on the off chance it was used, we now redirect to the new stored procedure

Sean 
How did you test it?

Gerald 
We spoke to the other team and they said it wasn't used, so no testing required. I am sure.

Sean 
So you leave it in case it is used, but don't test it because it isn't?

Gerald 
Yes sort of. In case this was ever resurrected the change was there for them to use

Sean 
It won't be

Not knowing which server runs his code

I also found it odd that he thought some of the code was run on the Scheduler, when in fact the Scheduler never has much logic; its role is to send “jobs” to the Application Servers to process them. Therefore, the code his team wrote is executed on the application server…

Sean
There must be a better way than making a Remoting Call to get this information!

Gerald
We're stuck here. The enablement is on the Application Server. The scheduler service is running this, and we don't want to make direct DB calls.

Sean
No it isn't. This code is running on an Application Server

I was playing around with it

case when (BeforeXml is null and datalength(BeforeXml) = 5)

This code is always false. It cannot be null (empty) and also be 5 characters.

Gerald : Good spot I will make this change now. I was playing around with it

The Major Incident

After the project went out, the release got “Red Flagged” which means we have to produce an urgent fix, and the release is halted from the rollout.

“Please can we red flag the current release. Unfortunately the database Schema Patch 8037 altering the Audit table filled up TempDB (60gb+) resulting in the database server Failing over.”

The change he made seemed very rushed. From the Principal Developer’s own words, he said that managers were demanding the change as quickly as possible. He should know better from his experience. Which is better: a delayed but correct fix, or a rushed broken one which would cause yet another red flag?

His new, rushed fix was inefficient, mainly from not understanding LINQ-to-SQL. There were also some database changes which the logic was wrong, and there was a typo in the SQL script which would have caused it to fail.

If it doesn’t work, it’s not gonna get past testing, and won’t release on time anyway, so what is the point rushing it?

Sean: This can't be very efficient…

Gerald: Might need an index on that, but it's linq to sql - a mind of its own. i can try profiling this, but seems pressured to get this in

Sean:
And if you slow down the saving of new records, you will find even more pressure to get a fix out
Also, Linq-SQL doen't have a mind of its own…

Additionally, a tester had a look at his changes and pointed out yet another error in his new database patch. If Gerald had run his changes through the database tool we have, it would have flagged the error. Absolutely embarrassing. But that’s what you get for rushing.

20 Patterns to watch for in your engineering team

Pluralsight have a free book “20 Patterns to watch for in your engineering team“

There’s no point in pasting the entire book in my blog, so I’ll just give the initial paragraph to highlight their ideas.

They are mostly negative ideas, or can be in certain situations. However, there are some positive ones such as “Bullseye Commits”.

PART 1 Work patterns exhibited on an individual level

1. Domain champion

The Domain Champion is an expert in a particular area of the codebase. They know nearly everything there is to know about their domain: every class, every method, every algorithm and pattern.

2. Hoarding the Code

This pattern refers to the work behavior of repeatedly working privately and hoarding all work in progress to deliver one giant pull request at the end of the sprint.

3. Unusually High Churn

Churn is a natural and healthy part of the development process and varies from project to project. However, Unusually High Churn is often an early indicator that a team or a person may be struggling with an assignment.

4. Bullseye Commits

This pattern is relatively common in most teams, but it often goes unrecognized: an engineer understands a problem, breaks down the project into smaller tasks, and submits code that has little room for improvement.

5. Heroing

Right before a release, the “Hero” finds some critical defect and makes a diving catch to save the day. More formally, Heroing is the reoccurring tendency to fix other people’s work at the last minute.

6. Over Helping

Collaboration among teammates is a natural and expected part of the development process. Over Helping is the pattern whereby one developer spends unnatural amounts of time helping another developer to get their work across the line.

7. Clean As You Go

A codebase is continuously evolving by nature, but it doesn’t evolve evenly across all aspects. A Clean As You Go engineer will notice and refine shortcomings even if it’s not essential to the task at hand.

8. In the Zone

This pattern is exhibited by engineers whose work is, in a word, consistent. They have a knack for getting in the zone and shipping high quality work week in and week out. Their work is reliable and predictable in nearly every way.

9. Bit Twiddling

Bit Twiddling is like working on jigsaw puzzle to the point where everything looks the same and you’re not making progress anymore. You might pick up the same piece, try it in a few places, rotate it, put down, only to pick it up a few minutes later.

10. The Busy Body

The Busy Body is an engineer who skips all over the codebase: they’ll fix a front-end problem here, jump to some refactoring, then fiddle with the database over there.

PART 2 Work patterns exhibited on a team-wide level

11. Scope Creep

Intuitively, we all know what Scope Creep is — along with its associated risks. Still, there are plenty of different definitions for the issue so here’s what we’re focusing on:

Scope Creep (noun): a pattern
whereby the originally agreed upon
scope increases or changes after
implementation has begun. Often,
though not always, Scope Creep
happens incrementally and thus invisibly

12. Flaky Product Ownership

Miscommunications between Product and Engineering can easily lead to Scope Creep. Flaky Product Ownership, however, can show up slightly different in the data and also generally requires a different approach

13. Expanding Refactor

Expanding refactors happen when a planned effort to improve or revise a section of code triggers a dramatic widening of scope.

14. Just One More Thing

“Just One More Thing” refers to the pattern of late-arriving pull requests. A team submits work, but then—right before the deadline—they jump in and make additions to that work.

15. Rubber Stamping

Rubber Stamping is the process by which an engineer approves their colleague’s PR without giving it a substantial review.

16. Knowledge Silos

Knowledge Silos are usually experienced between departments in traditional organizational structures, but they also form within teams when information is not passing freely between individuals.

17. Self-Merging PRs

This pattern refers to when an engineer opens a pull request and then approves it themselves. This means no one else reviewed the work and it’s headed to production!

18. Long-Running PRs

Long-running pull requests are PRs that have been open for a very long time (more than a week). A PR that doesn’t close in a normal amount of time (within a day) can indicate uncertainty or disagreement about the code.

19. A High Bus Factor

“Bus factor” is a thought experiment that asks what the consequence would be if an individual team member were hit by a bus.

20. Sprint Retrospectives

Retrospectives are a common practice that offer an easy way to continuously improve: take time to reflect, as an individual or a team, on a project, action, or occurrence

My Thoughts

There’s some good observations here, and the book does go into how to spot these aspects and what you can do to counter the negatives.

Where I work, we used to focus on having “Domain Experts” which is the “Domain Champion” idea. In recent years, I think managers have drastically swung the other way which leads to “The Busy Body” who knows a bit about many things but doesn’t specialise; so now we have the opposite problem.

There are a few areas where we still have a “Domain Champion” and they tend to get work related to that area if they like it or not. Even when it does get initially assigned to someone else, they take too long and then say “the deadline is approaching, I think the Domain Champion should do it”.

I always complain we don’t have many developers that are willing to do Code Reviews, or at least; do them properly, so you end up getting “Rubber Stamping” which is like having no review at all.

I personally like to “Clean As You Go”. It’s not a good idea to go far out of your way to change code that didn’t need to be changed. Doing so can dramatically increase the risk of introducing a bug and is “Scope Creep” since more areas now need to be tested. I do some “Heroing” from time to time, mainly because I do the most Code Reviews so I spot more opportunities to improve things, or point out bugs in their code.

Recently, with people quitting or moving teams, the Bus Factor has been severely lowered. Currently, I am the only Developer on the team, so if I take time off; then no work at all is getting done.