Not gone to production

We recently had a Development department meeting. One person raised a point about the recent higher-than-normal staff turnover, and also commented on the number of cancelled projects:

“If we look at Group Technology that I’m part of as an example, a lot of our staff turnover can be attributed to problems specific to the department and specific to development work: onerous manual, slow processes, moving from one cancelled product / project to another etc.. Some developers have been working here years and never worked on anything that has actually got to the customers for example. How do we propose to tackle those specific culture problems?”

I thought it was funny, because I knew one particular developer who had worked on a small cancelled project, then worked on – what was supposed to be – a replacement for our flagship software. Then when that project got restarted, he moved back to our flagship product on what was supposed to be the most important project; which also got canned.

Andy Marsden 16:18
i just love free shares
we got 50 last year, basically like £600 for nothing

Me 16:18
Our wage is basically for nothing
remember that time your code went to production?

Andy Marsden 16:19
no
is it just me that doesn't care about who uses my code?
i just like writing code

Me 16:19
Andy is writing code for the terrorists

Andy Marsden 16:20
haha
seen something before that's like a moral question:
"As a developer, if an oppressive government took over and forced you to write code to destroy Paris for example, would you write it flexibly so that it could be used to destroy any city?"
i couldn't bring myself to do it (write rigid code that only destroys Paris)

At the start of last year, the CEO had made a big fuss about a Scotland project that needed doing by Jan 2023, so many of our best staff members were assigned to it and were told not to help out with any other requests. As it got closer to the end of the year, there were discussions to cancel it, and eventually it was canned. You are talking approximately 30 staff members and paying them for a year of work. Completely binned off. Well, they said it had been shelved until our new software is released, but given that it has been in development since around 2018, is that ever going to be finished?

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.

90/10 rule of project management

The biggest thing that annoys me about software development is how projects really drag out near the end. It feels like you get so much done at the start, then progress really grinds to a halt. So early on, you think you are way ahead of schedule, then all of a sudden you feel like you are behind. Even when you get an extension, no matter how little is left, it seems to drag to take the allotted time.

I do think I am terrible for dragging tasks out. It’s easy to look at the time and think “ah it is 3pm, may as well keep testing this rather than moving onto the next task”.

As well as Parkinson’s Law, I was reading that there is what is known as the “90/10 rule of project management”. This is where the first 90% is completed in 10% of the time, but the last 10% takes 90% of the time. So it seems it’s not just me that thinks this is a phenomenon.

So let’s think why this is:

Putting off the unknown until the end

To maintain progress, it is much more tempting to pick up the work with the clearest requirements, and leave the uncertain ones to the end. Even when there is a Product Owner prioritising the work, they are often swayed by the Developer’s opinion. Also, there can be potential work you uncover when looking at what the code currently does and you think “I’ll investigate that later”. When you eventually get around to it, it turns out to be a “can of worms”.

Last-minute changes:

No matter how much you plan, there will usually be some last-minute changes since the customer can change their mind. With “scope-creep”, that last batch of work has become larger. In a similar fashion, you could be implementing a feature but then discover there’s other features that need changing to be compatible – so over time, more requirements are discovered.

“Resources”:

Over time, other projects or bug fixes suddenly increase in priority and you can find yourself with (at least temporarily) fewer team members. I also find that people may delay their annual leave during the time when they feel most excited about the project, then when it starts to drag, they then take it. Managers can also think “ah, that project is nearly finished, we can take a developer away and put them on a new project”.

Lines of code

If you imagine starting a new project from scratch, and think of a fairly straightforward feature like a dialog with several text boxes. You can quickly code the UI and the server code to save the data to the database. But then when you come to test it, you realise that resizing the dialog doesn’t scale. So you make a few tweaks. What about users that only use the keyboard? So you add the tab order, some accelerator keys etc. What about invalid data entry? So you add some extra checks and appropriate error messages. Ok, what about automated tests, so you do some refactoring and add some tests. The manual testers find a bug that a certain scenario doesn’t work. So with this example, it is easy to understand that the dialog can be 90% complete really quickly, but that extra 10% to perfect it took a long time, and maybe some delays occurred communicating back and forth with the Testers and Product Owner in order to finalise the work.

But we can take this explanation further.

A few weeks later, you need to add a feature that interacts with this code. You might need to re-familiarise yourself with the code, re-reading several files, and your changes need to not break any existing feature. Now imagine this kind of sequence happening over many months. The codebase increases in size and complexity, and the constant refactoring might mean that files you remember with a certain name have now drastically changed, moved or been deleted. Development is far slower now for most features, and even bug fixes could involve stepping through more lines of code in order to diagnose and fix. So I’d say productivity drops proportional to the size of the codebase.

Learning Unity

Several years ago, I tried learning Unity (games development) but it took a long time to pick up the basics. I’ve been meaning to give it another go, and when Code Monkey (a Youtuber I did subscribe to) announced he was making a 10 hour free course, I thought it’s the perfect time to jump in.

Following along, rewatching and experimenting with what I was learning took ages. I ended up dedicating many weekends and a couple of hours each day to complete it.

It’s a basic Overcooked-style game. If you want to make it, you do need to sign-up to Code Monkey’s website https://unitycodemonkey.com/kitchenchaoscourse.php to download the assets (graphics and a bit of audio).

If you want to play it, you can get it on Steam for free. https://store.steampowered.com/app/2275820/Kitchen_Chaos__Learn_Game_Development/

A few days ago, Code Monkey released part 2 of his free course! This time he adds multiplayer. I haven’t watched that one, but have added it to my Watch List.

I have: many Youtube videos; websites; books from Humble Bundle to go through, so I’ll keep at it.

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.

4 Year Special: Derek

It’s been 4 years since I started writing this blog! how time flies.

The original inspiration for the blog was to tell stories about my incompetent colleague Derek. However, he left shortly after I started writing – so I didn’t end up writing many blogs about him.

After going through some old chat logs I found, I have dug up some extra stories, code snippets, and quotes.

“There’s a lot of duplication here, surprisingly. Functionally it is probably quite a big bag of spanners”
Derek

Hypocrite

When I first started working with Derek, I was telling my manager about the things he was doing and saying. My manager asked me how he is getting on to see if I had more stories to tell.

Matt:
How is Derek getting on?

Me 10:36:
on holiday, but he has spent a week or so sorting out some buttons. He has assigned two items to himself. 

So he complained that the User Stories were too big, so we have split them up a bit more. Yet he then assigns 2.

He hasn't sorted out all my comments I made on his last fix.

After Derek was making excuses that he wasn’t productive due to the size of our planned work, we split it into smaller chunks for his benefit. He then picks up multiple chunks in addition to also needing to rework his previous work. It seems we didn’t need to split the work up at all.

Redefining a boolean

bool hasSelectedReports = SelectedReports.Count != 1; 
bool noReportSelected = SelectedReports.Count == 0;

A boolean or “bool” for short represents true or false. Since there are two states, if you created a variable called “hasSelectedReports” you can invert it using the ! (not) operator, so saying !hasSelectedReports reads as “has no selected reports”. Derek decided to create a variable for the inverse, but then he got the logic wrong: If Count is 0, both hasSelectedReports and noReportSelected are true!

“I’m working from home today. Sorry I haven’t emailed sooner, I had some problems initially remoting in as my computer at work had a dicky fit.”
Derek

Pointless Conversion and check

return !String.IsNullOrEmpty(ConceptId.ToString()) ? ConceptId.ToString() : null;

ConceptId is defined as a long ,which is a number. It can represent any whole number between -9,223,372,036,854,775,808 to 9,223,372,036,854,775,807. It cannot be null/empty. By default it will be 0. Here, Derek converts the number to text (string) so he can take advantage of the IsNullOrEmpty method. If it is somehow null, we will return null… apart from if ConceptId was somehow null, calling ToString on it would crash.

If he really did want the text presentation of the number, this code should simply be

return ConceptId.ToString();

Pointless Conversion and check: part 2

if (!String.IsNullOrEmpty(conceptId.ToString()))
{
   	_conceptIdInt64 = Convert.ToInt64(conceptId);
}

Again, Derek decides to convert the number to a string so he can check it isn’t null. It is always true, so then goes into the code block. He then converts the long to an Int64. A long IS an Int64. It’s just an alias for it. So the code is completely pointless, other than assigning a simple variable.

“I may have taken too much artistic licence”
Derek

If at first you don’t succeed, try again

public Entity GetEntityFromConceptId(long conceptId)
{
  	return 
  	  	(EntityFinder.TryGetEntityFromId(conceptId)) != null 
  	  	? 
  	  	EntityFinder.TryGetEntityFromId(conceptId) 
  	  	: 
  	  	null;
}

When methods are named “Try”, it means that it might not be successful and can return “null”. In this case, TryGetEntityFromId returns null if you pass in a conceptId that doesn’t exist. So you should check for null, and do something different, maybe display an error message to the user. Although Derek decides to simply try again. Why would it work the second time?

(frustrated tone) I’ve spent 5 days on the same work item
Derek after 13 days trying to implement a simple feature. For the previous 3 days, during the daily stand-up meetings, he was saying he is “nearly finished and it’s pretty much ready to check in”

Conclusion

As you can tell from these stories, Derek made some bizarre choices and generally seemed deluded. There were times where I thought he had to be trolling, because there is no way you would write so much redundant code. I understand you can have bad days and have the occasional moments of madness, but it was a daily occurrence with Derek. For more stories about Derek, click the Derek tag.

YAGNI / This Code Could be Useful In The Future

I remember the days of being a junior software developer where you write code because “it might be useful at some point in the future”. The thing is, this means you end up writing way more code than is necessary, and could lead to overcomplicated designs.

There is a concept called YAGNI: “You Ain’t Gonna Need It”

Swizec sums this up well here with a strong hint of sarcasm…

And that brings us to the other YAGNI: Building stuff you think you’ll need before you need it.

You think of a wonderful abstraction. A wonderful little feature that’s gonna help oh so much in the future.

You build it.

Months pass. You tweak the code here and there. Nobody’s using it, but you have to maintain otherwise it’s gonna break. Whenever you implement a feature, you have to think about how it impacts this code you’ve got.

It’s slowing you down. Making you think. And you aren’t even using it.

But one day! One glorious day, your PM gives you a task. A task from the gods. You are about to use this code you predicted 6 months ago!

You are a god of prediction! You knew it all along!
https://swizec.com/blog/dry-is-a-footgun-remember-to-yagni/

A simple example I came across recently was this:

public static bool IsModalDialogOpen()
{
  Boolean isModalDialogOpen = Application.AnyModalDialogOpen;
  if (isModalDialogOpen)
  {
     throw new DialogException("A dialog is already open");
  }
  return isModalDialogOpen;
}

So they have written a method that checks if a dialog is already open. If there is, then an exception is thrown (depending on their error handling, it would either crash, or display some kind of error to the user), otherwise it returns false. Since this is new code, the only places that called this method was in his new code. However, he never checked the return value of the method. Since it is defined as a Boolean/bool then it could only return true or false, but his logic only ever returned false… and all the calling code never used the value. Our exchange went like this:

Me: Why do we have a return type for this method when the return value is not used above?
Developer: If any one in future want to know the value it will be useful because of that I have added.
Me: https://rules.sonarsource.com/csharp/RSPEC-3241
       Also: it will never return true
Developer: OK, removed the return type.
Me: It needs an appropriate method name now

So at first he says it will be useful in future – which is definitely false – YANGI definitely applied in this situation. Then when he agreed that the boolean value wasn’t useful, he left the method name as IsModalDialogOpen which implies it should return a boolean. In the end, we were left with this:

public static void ThrowExceptionIfModalDialogOpen()
{
   if (Application.AnyModalDialogOpen)
   {
     throw new DialogException("A dialog is already open");
   }
}

In another recent change, there was this humorous exchange between two Senior Developers, George and Paul:

George
This looks like dead code...

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

George
How did you test it?

Paul
We spoke to the Data guys and they said it wasnt used, so no testing required. I am sure.

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

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

George
It won't be

So Paul had been migrating a database “into the Cloud”, and so went through all the features where that old database was used. He found some old code which he believed to be obsolete, but made the change there anyway. George then flags it up, and Paul says he changed it just in case it was actually used, even though other developers also agreed it wasn’t used. He didn’t test it because it wasn’t used, but the code is there if someone decides it should be used, but it might not actually work because he never tested it.

You could make a variation on that Swizec quote at the start of the blog. It’s extra code that you have to maintain and slows you down, but the day that code is finally used; Paul has saved people time! (Apart from all the developers who will end up looking at the obsolete code in the meantime).

Coding Tales #1

I haven’t written many blogs talking about code for a while, but my notes are stacking up; so time to post some blogs discussing code! I’ll try to explain them the best I can for the non-programmers, or those that find C# hard to understand.

There was one bug fix where the developer took several attempts to fix it. Taking several attempts to fix a bug illustrates that you either – don’t have a good understanding of:

the requirements,
or the codebase that you are changing.

If you make a change and it introduces a bug, then you make another change and it introduces another – instead of playing “whack-a-mole”, it’s worth considering if you are actually writing the correct fix. From my experience, you can often just take a step back, ask for help, and come up with a much simpler, and less risky fix.

In one of their changes, I saw evidence that it was possible that they didn’t understand what they were changing, or taking care in their work.

case ParentRecordRelationship.Grouped:
//when a record is grouped, the associated text is NOT altered - so this method should not be called
   throw new ArgumentException(string.Concat("Invalid parent record relationship supplied: ", parentRecordRelationship));

So we have a comment, explaining that the existing requirement is that the “associated text” is not modified, so it is completely invalid to call this particular method which would change the associated text (other code not shown). Therefore, we throw an exception which would cause the system to show an error to the user.

The developer changed the code to this:

case ParentRecordRelationship.Grouped:
    newAssociatedTextValue = CreateAssociatedTextValue(string.Format(_groupedWithFormatText, parentrecordOriginalTerm));
     break;
//when a record is grouped, the associated text is NOT altered - so this method should not be called
//throw new ArgumentException(string.Concat("Invalid parent record relationship supplied: ", parentRecordRelationship));

So now he has allowed the text to be updated in this situation. Unless that really is a change in requirements, then this is the wrong thing to do. Also, instead of deleting the code that throws an exception, he has just “commented” it out (which is why it now shows in green), and also left in the actual comment saying that this scenario is invalid; which now adds to the confusion when the next developer reads it.

To me, that shows that the developer is uncertain it is the right thing to do, otherwise you would remove the comment and the code. Just “commenting” the old implementation out basically acts as a reminder that you might want to restore it. (You could easily just look at the history of the file to see the old code anyway, it isn’t like it is lost if you actually delete it in the first place.)

He also added to the confusion with this change

//take the 1st 12 characters of the format text
itemValueStartsWith = _combinedWithFormatText.Substring(0, 12);

it is now

//take the 1st 12 characters of the format text                      
itemValueStartsWith = _combinedWithFormatText.Substring(0, 9);

This is a good example of why writing comments for simple code is bad. The comment explains a requirement that it should take 12 characters, but you can see that it takes 9.

To me, this shows a lack of care, and lack of attention to detail – which are basic traits that I think are important for good software developers.

AI picture generators

I’ve been playing with one of those AI picture generators; stabilityai.

I was trying to think of ideas that are a play on words, or scenarios that you wouldn’t imagine a character to do – similar ideas to what Jim’ll Paint It would do.

I had this Wizard of Oz idea where the Lion is Simba, Tin Man is Iron Man, and the Scarecrow could be Scarecrow from Batman. The AI seems to like Iron Man, and has created some horrific concept of Simba in an Iron Man suit, but I don’t know what is going on with the rest. The ground looks quite sandy, so has the yellow idea but no bricks.

What have you never seen Iron Man do? ride a horse. May as well be competing in a horse race. I like how in the last one, he has somehow caused an explosion and his horse is no where to be seen.

I thought I’d try one with Batman. I was thinking how he seems to work at night and has all this tech. Then I decided I wanted to see him just working on the software. The AI decided to draw some kind of comic.

When working, Batman is sometimes like a detective. I wanted to see him team up with another detective. The first image is brilliant. The third is funny because Sherlock has turned up with a Batman cowl.

Every so often, people remember that Mario is a plumber. You see him pretty much do everything but plumbing!

I chose another game character and came up with a simplistic play on his name. It’s strange how the AI has taken Crash’s design and environment colour scheme; but then decided to make it really surreal.

Another simple play on the name. Seems to love Daniel Craig.

Indiana Jones and Tomb Raider are fairly similar. I wanted to see them together. The bottom two images are just Lara clones though. It’s interesting how it has chosen similar outfits which makes the top 2 images look like a real crossover.

I did have a brief thought about how Link from Legend of Zelda gets attacked by a horde of Cuccos (chicken-like birds) if he attacks them. I thought I had more chance of generating someting good if I used normal chickens, and decided that Kentucky Fried Chicken’s Colonel Sanders should lead them.

A good play on the Lord Of The Rings character’s surname has generated these amazing images. I love this one.

You can see the pain and torment the actor is going through.

Film director solving a Rubix cube, or maybe even making them

Halo’s Master Chief participating in a cookery competition

The Pokémon Pikachu living up to his name

Farcical Development: Templates

The Plan

A developer in my team, Isobel, was free to pick up some work, and she knew another team had too many items assigned, so their Team Lead contacted us to ask if we could pick up the work instead.

I wasn’t opposed to the idea, but we didn’t know what the enhancement was at the time, so didn’t know how long it would take and didn’t know if our Testers would be available to test it, or would the other team test it? It’s unclear, but their Team Lead wanted me to commit and accept reassigning it to us.

When I looked at the enhancement details, there was loads of confusion because it had loads of code changes linked to it, and was logged about a year and a half ago. It turns out, a year ago, a developer had done the work, checked the code in, but then got told to “roll it back” due to lack of testing resources. Then instead of going into the next release, it was somehow delayed a year… Now we have a lack of development and testing resources!

There was this comment too:

“The change will then need to go out as an urgent/emergency release. As the functionality is deemed potentially unsafe, we only have 7 days to get it out, it’s actually 6 now as it was logged yesterday.“

1.5 years later…

There was another item I was assigned recently, and that had similar chaos with how long it sat on our backlog

10th November 2020 - bug logged
15th September 2021 - assigned to the team

No wonder our users are often reluctant to report bugs because we don’t seem bothered about fixing them. Then our release process is also really long so sometimes there’s a 6 month lead time after we fix it.

I'll try and quickly explain the feature: Users can create these Templates which are composed of components. There's these special "calculation" components which use data added to the record and give you a score. Users can add data-entry components to the template which can be used by the calculation components. However, it's not clear which data is used, and we can change the calculation formula at any time; which makes it “unsafe”. You can also group several components together to make a Group Component. So the plan is basically to stop users from adding these calculator components, and they have to use our own Group Components which will have the calculator and the prerequisite data-entry components with it. For existing templates, we just have to show a message, telling the users their template isn’t recommended to be used.

I was invited to a meeting along with Isobel, and the managers tell us that all we need to do is take the old code, update some user-facing text which the UX team will confirm, then it just needs to be tested, but it should all work since it was ready for testing over a year ago. So in terms of development work, it sounds like we’d spend more time in meetings and generally discussing the work – than actually doing the work. (I later find out this is not true).

Assigned to Isobel

I tell Isobel to go through the changes and make sure they really do meet the requirements. A few days later, Isobel says she is on annual leave for 2 week but the changes are fine. The next day, I’m told I should look at it instead.

Assigned to Me

After an hour of testing it out, I find that there was:

A control partially truncated
Some extra spacing in one of the error messages
Some components that were disabled that shouldn’t be
Inconsistency in behaviour between “Templates” and “Templates Pro”
Group Item logic was completely wrong
Blank warnings were appearing for all other components

So how did the original developer think this was ready? Why did Isobel think it was ready?

So I start to fix the issues and I find copy-and-pasted code, redundant code, unclear code, code which could easily be unit tested but isn’t. I spent around 3 weeks sorting it out and it still wasn’t perfect. Meanwhile, I was invited to other meetings to say they changed their mind about some features. I had to undo some changes, change more UI text, and disable a few more components. In hindsight, I think I may as well have binned it, and started it again from scratch.

When I thought it was ready, I had the Pull Request created in the first week of January, ready to be checked-in for testing, however, there were no testers free, so it sat there for a month.

Eventually, the testers begin testing it and find a few problems with it. I fixed 3 out of 4 issues, but the last one seemed to be impossible to fix due to another bug which really needed to go to the specialist team that dealt with that area.

The actual template knows where it came from, but the Group item inside doesn’t. There was this interesting variable name that made me smile.

isOneOfOurs

I showed it to a colleague

“seems like Britain First wrote this”
Bants from colleague

There was some code where we set the originID to either the user’s or our organisationID. However, it set it to 0 which then assumed it was one of ours. I tried looking at one of the other properties which was a different type of ID; a GUID, but it was blank, so it was broken there too.

I couldn’t see a simple way to fix this. It would be far too risky for me to change, and I definitely didn’t have time. So I got told to abandon it and it would be reprioritised.

I think it was around 6 weeks later, it was assigned to another team. So it is now it’s with its 4th team, approaching 2 years later. Maybe we can call this a “pass the parcel” enhancement.

Assigned to Kumar

I was aware that the development re-started (along with some other requirement changes) when I saw a Pull Request for it. It was from Kumar, a developer in India that is absolutely rubbish. Not only that, it is quite hard to help him because his English is fairly poor. I also can’t tell if he is trolling, or if he really is that bad.

I would have thought that Kumar would have been told to speak to me about the work so I can “hand it over”, or at least he should have seen my name and comments on the item and shown some initiative and asked me about it. As it goes, this new change was something I already had fixed, it just wasn’t checked in. I could tell his fix wouldn’t work just by reading his changes. I message him telling him this.

He later responds with this conversation:

Kumar  10:11
Hi Mate
10:12
With the changes i have raised as a PR, I created a page as Group item and section as well with itself containing the components as Group as well as non Group items and it seems working  .
can i share you the screen mate ?

Me 10:14
so a user authored page/section Group item which contains one of the components - shows the message
and a Officially-authored page/section Group item which contains one of the calculators - does not show the message

Kumar 10:14 Exactly mate Kumar 12:34 Hi Mate, Shall i comment out that the fix has completed the scenario we discussed here ?

I didn’t believe him, so I checked his code out, built it and tested it out myself. Obviously broken as expected.

Me  12:50
just trying it now and it looks broken to me
got an Officially Authored Group Page and all the components have warnings next to them

Kumar  13:10
Ok dude, I will have a look on it.
Kumar  13:31
Hi Mate, As u said it was showing up all the warning
I might have not removed the Pages and section flag check for showing warnings
will debug the code mate,

The next day

Kumar  07:12
Hi Mate
Good Morning Mate

I look at the latest Pull Requests and see his new changes. Instead of taking my code that I know works, he has come up with his own solution. I think it might work, just harder to read.

Me  08:59 did you test my changes or just write that yourself?

Kumar  09:11
no mate i have took it from latest service branch of my team
was everything fine mate ?
i tested that locally and working fine

Me  09:13
it is similar to what I had done, just more lines of code

Kumar  09:14
haha, ok mate i will make the changes as suggested in the comments
Thanks Mate

It’s not really funny. You just wasted a full day’s work because you didn’t just use my code.

I explained to him that this item is a bit of a nightmare since there’s multiple places you need to change due to “Templates” and “Templates Pro” which doesn’t share much code. Then there’s some existing bugs, and many different combinations of templates you can create. He doesn’t seem to test his work at all, so I think he had no chance of getting this working. I was trying to emphasise how much testing needs doing with every change to try and get him to put some effort in. Unit testing would help alleviate some of the manual testing. The next day…

Kumar 13:27
Hi mate, Good noon
It is becoming a night mare as u said :joy:, i have completed with the unit test case also have fixed other area like Warnings were happening in the “Admin org” too. i have fixed that now. Kindly review and guide me to proceed further

Me  15:58
<send him picture> that should have the main yellow banner at the top shouldn't it?

Kumar  15:58
If it was a “Admin org” org, it should not have mate

Me  15:59
it's not

Kumar  15:59
Then it should have i believe
haah, one more PR patch upcoming ..?
:exploding_head: literally with this work

Me  16:17
I suggested 2 unit tests. You have only done 1. I think it is the other scenario where it doesn't work

Kumar  16:32
Ok , but if the method will not execute if the template was Group template mate, so do i need to do that as well ? in turn it returns empty

Me  16:40
isn't your requirement that it shows the message regardless if it is Officially-authored or User-authored

Kumar  16:41
yes, that is the requirement
i will check on the code once again mate
haah lil confusing

Although the overall work is confusing, this is one of the simplest parts of it: Show a message regardless of who created it. If the method is “returning empty” then that is the bug, it should return a message.

There was a line of code like

if (!configEnabled && activeSubscription)

and he changed it to

if ((!configEnabled && activeSubscription)
||
(configEnabled && activeSubscription))

so I wrote: “so just activeSubscription then“

Kumar: I am not able to get ur point here, kindly guide me

Me: “true or false” is always true isn’t it?

Kumar
yes mate, so shall it be framed like this
if ((configEnabled || !configEnabled) && activeSubscription)

NO! That still says “true or false”. I was trying to think of how I can write a response without telling him he doesn’t understand the very basics of programming. This is like Day 1 of learning how to write code.

Kumar: haah, I got it, just “activeSubscription” is enough isn’t it?

I was glad he seemed to understand in the end, because I was tempted to tell him to change his career…then he adds:

“correct me if I am wrong”

He has zero confidence.

The very next day he then tells me there is another requirement to remove the banner that he has been changing, so “is there any point carrying on?”. He sends me a screenshot of the requirement rather than giving me a link. He is definitely sent here to troll.

I’m sure he must have known about this, and I don’t know why he either:

didn’t make the deletions first (it would reduce the amount of code and reduce confusion)
not change any code he knew he was going to delete

So he made changes trying to fix a feature he knew he was going to remove. I had invested time reading the code, manually testing it, and all this back and forth communication. What a waste of my time.

Unassigned?

The good news is that he is leaving so he is going to be another company’s problem. The bad news is this enhancement is going to be reassigned to someone else.

I’ll probably write another blog about Kumar; I’ve got some more notes on previous development work he has done. If this enhancement ever goes live, maybe I will write a follow-up blog on it too.