Project: Batch Approval

This long blog documents what I have been working on for the past year. I had made lots of notes with the aim of writing a blog, in addition to taking extra notes from chat logs.

We actually estimated the project would take around 5 months, but then an extra 2 months for testing and go through our slow rollout process. It actually took closer to a year. I’d say it was a combination of:

realising the feature was more complicated than anticipated
the UX team had little knowledge of the actual user experience
managers changing or trying to change team members
our slow release process
90/10 rule of project management

We were told the project was important, yet we were only assigned 2 developers (as in myself and one other). As the project came to a close, we were being integrated into our new team, therefore other developers could help out during the final stages.

Here is a list of all the people involved over the project’s lifetime:

Name (Core team in bold)	Role
Me	Developer (Team Lead)
Daniel	Developer
Dean	Developer (Temporary)
Dennis	Developer (Temporary)
Tina	Tester
Tim	Tester
Colin	Technical Manager
Mary	Technical Manager
Olivia	Product Owner
Owen	Product Owner
Carl	Customer Representative
Adam	Architect
Andy	Architect
Grace	Safety & Legal Governance
Ulrika	UX
Ursula	UX

I’ve made the names start with a letter to represent their job title, apart from Colin because he is a recurring person in my blogs. I’ll put reminders throughout the blog so it is easy to follow.

Current Software

To protect anonymity, I need to come up with a different theme for what the software is for. Let’s say customers request various restricted items of different severity. So a request could come in for a Hunting Rifle, and the user needs to know if they have the adequate licence to possess firearms and they are deemed medically safe in a recent time-frame. Possible warnings are shown which the user can dismiss/acknowledge e.g. “licence is up for renewal in the next 3 months”, “recent purchase of other firearms”. Standard users can create “Awaiting Approval” tasks and assign them to users with authority to approve. To approve them, the authorised users open the task list, view the details, then click approve. Many tasks have either no warnings, or low-severity warnings, so users often just glance at the info and click Approve. The system then sends the approved request to a central system, then loads up the next task. There’s a couple of seconds delay due to the “digital signing”, a couple of seconds for sending, then loading up the next record. To sign loads of tasks, it’s a very slow and laborious process. It’s a major source of complaints from our users.

Unsafe/Unofficial Automation

Carl [Customer Representative] sent a link to a video where someone was demoing a commercial automated tool that autocompletes the tasks. It waits for the system to load, clicks the approve button, then repeat. So you could set it running, then walk away from your desk.

I thought it seemed ridiculously irresponsible and would cause people to be sacked if they got caught using such a tool:

A) The program is now the one authorising the tasks, not the qualified user. What’s the point needing to have qualifications if you aren’t even going to read what is on-screen? If a task was wrongly approved, then the user would be accountable.

B) if you walk away from your desk, you are leaving your PC unlocked, along with your physical Security Key.

The creator had actually put a bit of thought into it though. If there are any Warnings that require another click to dismiss/override, then the automation is paused.

The video claimed that some users have up to 500 tasks to sign after a weekend. They charge a fixed yearly fee of £295, plus 7p per customer on the system per year.

“the robot does not get bored, does not make human errors, and crucially is a lot cheaper than the user’s hourly wage”
Promotional video for the Automation tool

Probably just makes robotic errors instead!

I said we should change the names of the buttons to try and screw them since it probably uses something like that to locate the button to click. It would be quite funny to make them dish out refunds.

The existence of the automation tool shows how much the users desire a better solution.

UX User Feedback

Given the existence of such an automated tool, it is no surprise that one frequently requested feature is Batch Approval. Our UX team put together some kind of interactive prototype and invited a few users to provide feedback on two designs. The alternative design was actually produced by Mary [Technical Manager] who has no UX qualifications. I’m not sure how that came about and why UX agreed to trial her design, but the feedback was actually extremely favourable to her design.

This caused her to be quite smug and maybe caused some animosity as we will see later. The ratings out of 5 were:

(Option A) 4.3 for Mary’s design

(Option B) 2.3 for UX Team’s design

For additional comments, one user commented:

“I prefer Option A by a country mile – Option B feels even worse than the existing system!”

Another commented:

“Option B feels more clunky, less user friendly than option A. A lot of clicking involved”

One even gave a threatening response:

“Option A or you’re gonna lose me and my franchise”

Shortly, there was a write-up from a conference where the feature was announced:

“This item is one that really did steal the show – this is something that our customers have been very eager to see us implement and are very excited to learn that we are busy developing this solution.”

“Busy developing this solution” made me laugh, because at the time, all I had was a dialog box with a couple of lines of text and a button.

Proposed Change

The general idea, is that the user is presented with key details from the tasks in a data grid.

They can click checkboxes to select which tasks they want to approve.
These are added in a queue to send in the background.
The user can continue working as they are sending.
The “digital signing” has to take place on the user’s computer so a large part is done client-side.
The user has to remain logged in until the process is finished.

This project had actually been discussed for years, but because there wasn’t much of a “commercial drive” for it – we would be giving users this feature for free – it was always low priority.

Product Owner: Owen

I think the initial planning was done by a different Product Owner but then when the project fully began, we were assigned a new Product Owner, Owen, who was new to the company, but he also gave me the impression that he was new to the role…but also didn’t seem very clever in general.

Here are some quotes that happened in various meetings (mainly Sprint Planning and Refinement).

Owen: "which work item is it?"
Me: “the one right at the top"
Owen: slowly scrolls...chooses 2nd item

Me: "it's not a Must, it is a Could"
Owen saves it with Must tag
Tim [Tester]: "No, Owen, you tagged it wrong, go back"
Owen: "Which WI is this?"

saves it with the Must tag again
Then goes back into the work item and gets confused
then goes back into it again. I think he needs rebooting

Me: "you need to set the state"
Owen clicks to close
Me: "you need to set the state, go back"
Owen is confused
Me: "left hand side Owen!"
Owen hovers over the right
Me: "left hand side Owen!"
Owen moves down

Me: "leave it as it is"
Owen "Which one shall I take out?"
I'm sure he is intentionally 30 seconds behind to wind us all up

Owen changes Story Points from 3 to a 5 without any discussion.
"shall we keep it at 5?"

For another item, I was talking about how the requirement is either obsolete, or needs a completely different approach from the initial proposal. 
Owen: "So how many points shall we add?"

"The system crashes when entering incorrect PIN and clicking 'OK' on error prompt"
Owen: "what was the behaviour before we fixed this?"
team: "It crashed"

We were discussing how we logged a bug a few months back but haven’t seen it occur since, so it will need some investigation to try work out what the recreation steps are.

“Assuming the bug still exists, how long will it take to fix it?”
Owen

Estimating software changes is hard, but I always think bugs are even harder to estimate. It’s only possible if there’s clear recreation steps, otherwise it is stupid to ask – we can’t fix it if we don’t know what the problem even is.

“depending on Grace’s [Safety & Legal Governance] feedback, do you know how long it would take to fix?”
Owen

Translation: can you predict what Grace would say, and given that she did say it, can you come up with an estimate for it?

I logged a bug about suggestions on how to improve a dialog. It would be up to Owen or UX to decide on the approach to fix it. Owen then asks questions along the lines of: “what do we need to do for this? do we need it?” I said it would be nice but it’s not my decision. Then he still asks “do we need it?” “can we close it?“

What’s the point asking me these questions, when I logged it with the aim of asking him to decide?

When the project deadline was looming, we ended up having multiple meetings to decide if there’s any features we could scrap, or defer to a later release. After the first meeting where we decided scope, he may as well have said “You know those items you said we need to do and couldn’t defer them, are you sure we can’t defer them”, because he was arranging subsequent meetings to go back over them. When we came up with estimates which showed that we would need at least another month, he was then arranging another meeting to re-estimate them.

The Architects

An important project started around the same time ours did. Our architect, Adam [Architect], was reassigned to the new project. Andy [Architect] joined our team as a replacement. He wasn’t completely new to the company but wasn’t familiar with this area of the system. Additionally, I don’t think he even looked at the software or even requested a demo.

Any question we asked him, he ended up making an excuse that he was busy and will get back to me later. Then when he did answer, I then sent a message to the original architect, Adam, and he said Andy had asked Adam about it and simply relayed the message back to us. So basically Andy wasn’t doing anything. We had him officially assigned, but it was Adam [Architect] that was answering the questions but via a middle-man.

The July Cancellation

There was a bit of disruption when our project looked to be cancelled, but there was apparently some mis-communication.

Hi All, a decision has been made by Directors to stop Batch Approval and to move resources across to pick up Project France instead. Therefore I will be cancelling the Batch Approval meetings.
Project Manager

1 day Later

The directors had decided to move you to the new project so I cancelled the meetings, but then I find that there wasn’t a firm decision from the Directors.
Project Manager

Brian has asked us to proceed with Batch Approval as originally planned. Sorry about the chaos dudes. They must be smoking some good drugs upstairs.
Olivia [Product Owner]

It was off the table, then someone put it back on the table, then someone else swept it off the table, then someone picked it up off the floor and put it back on the table.
Andy [Architect]

Coding Tales

Colin [Technical Manager]: "What sprint are you in?"
Me: "I dunno"
Colin [Technical Manager]: "you are the team lead, you should know"
Me: "No one in the team knows"

Put it in a new tab but make it behave like a dialog

The original UX designs looked like it fit nicely in the existing Task Framework. The requirements were that Batch Approval had:

Its own folder but is a sub-folder of Approvals
Opening a task opens it in a new tab

After looking at the code though, the framework didn’t actually support a sub-item. But we found a basic workaround to make it look like it did. However, there were quite a few features that we got “for free”, but we didn’t want them because they weren’t appropriate for a sub folder. So I had to disable the features by hacky code.

If you double click a task, then it opens in a new tab, which is what they wanted. However, they then didn’t want you to be able to navigate away into other parts of the system, and the Task Framework didn’t support that. With a bit of a workaround, I got that working, but the tab was designed to view one task only, and we are displaying a Batch of them. A few weeks went by and I managed to cobble something together, but the code was awful.

I took a step back and thought about it.

We have a tab that the users surely would expect to be able to move away from to view other tabs.
I’m using this “tab” which is designed for a single task, and I want multiple. So I had to make my own custom page.
We have hacked a sub folder and had to basically fight against the codebase to get it all working…

So why don’t we just have a button on the main folder, and it launches a modal dialog?

It would take a couple of days to get working,
the code would be neat,
and I think it’s what the user would expect.

After speaking to UX about it, they were happy with my proposal. I had wasted about 3 weeks trying to get it working like they previously wanted. Also, we are again telling UX what a good UX design is.

Scrollbar

The UX was also clear that we didn’t want a scrollbar to appear, and instead we use pagination. I didn’t see anything obvious in the standard DataGridView Winforms control, although I’m sure this is a common problem/requirement.

I ended up writing my own logic to add controls to the grid, keep track of the size, then stop adding when the size exceeds the height of the control. However, if there is only 1 very large task, we have no choice but to use a scrollbar.

The problem we encountered was that sometimes a scrollbar did appear when it shouldn’t. I made some tweaks to the calculation and it seemed to work fine. But then a Tester found a combination of task sizes where it still appeared. I couldn’t work out what I was missing in the calculations but it seemed about 4 pixels off, so I just added that into the calculation. Again, all seemed fine for a few days, but then the Tester found a combination of sizes where it still appeared.

Olivia [Product Owner] suggested that we detect when there is a scrollbar then disable the Approve button until the user scrolls down.

I said if we know when the scrollbar is there, why don’t we just remove the last task and check for the scrollbar again, repeat until the scrollbar has gone. I thought the code would be messy, and I’d end up writing a stupid code comment like “mate, something has gone wrong with the calculations here, so we’re gonna have to do some jiggery pokery to get out of this mess”.

Adam [Architect] did suggest some alternatives and they were just as wildly wrong.

Dean, a developer in another team agreed to help, and after a couple of days, he says “you can just set the vertical scrollbar to be disabled”.

But if the scrollbar is appearing so you have to scroll to view the content, then surely disabling the scrollbar will mean content is off the screen?

I tested his idea, and it worked fine! What must be happening is that the vertical scrollbar appears and takes some of the horizontal space… which causes the text to wrap and creates the need for more vertical space. Therefore the scrollbar is required and so remains. But if you tell the scrollbar it cannot appear, then the controls are added, and my calculations meant it fit perfectly in the grid.

It’s a self-fulfilling prophecy!

Olivia [Product Owner]: Do we have concerns about the unknowns?
Tim [Tester]: It's just the unknowns that we don't know about
I feel like you need to know the system inside and out to be able to safely implement this

Conflict With The UX Team

UX: “We want to minimise pop-ups”
Also UX: “Add a pop up after closing the dialog”

Ulrika [UX] had to take time off to deal with some personal problems. Ursula [UX] agreed to join the meeting we arranged on the Wednesday.

“I don’t work Thursday/Friday and have to leave early on a Wednesday to get the kids. I’ll get back to you next week”.
Ursula covers for Ulrika but then also has time off.

When she got back to us, she seemed to overlook how users access this restricted part of the system, and it turned out none of the UX team actually had this knowledge. So halfway through the project, we were discovering new requirements because they hadn’t designed the user flow.

Don’t Have Time

In early January, we were waiting for UX to give us some approved text but they seemed to be taking their time. I asked Olivia [Product Owner] what was going on, and she said that we don’t have time to make any more changes so they “needed to stop requesting changes”. Even though I pointed out that I was the one requesting changes, she said “we don’t have time to test” (even though it only involved quickly checking some text has changed on a message box). Nearly 2 months went by before we actually began to release.

After more protests from me, she says:

“The text is fine for now. We don’t have time to be changing it.”
Olivia [Product Owner]

When it came for the final review, reviewers questioned why we had dialogs with some ToDO comments on it saying “ToDo: Awaiting UX approval“. Even if you don’t have comments like that, I have seen developers question the user-facing messages if the grammar isn’t correct or sounds unclear. It definitely wasn’t clear because we just wrote the first thing that popped into our heads at the time; knowing the text would be replaced.

I think what had happened was that Mary [Technical Manager] and Olivia [Product Owner] had fallen out with Ulrika [UX], and then was refusing to authorise her changes. Remember, tensions will have been building since users criticised Ulrika’s design and wanted Mary’s design, and Mary’s arrogance about it wouldn’t have gone down well.

It’s just part of the process though – all text needs to be approved by the UX team; otherwise what is the point of their team?

Conflict With The Architect

When we implemented Adam [Architect]’s suggested invalidation logic, we thought the criteria was too restrictive. Adam was off on annual leave for a few weeks so we couldn’t consult him. So we made our own decision to change it, and got Carl [Customer Representative] and Grace [Safety & Legal Governance] in agreement. However, when the Architect saw it, he said it was unsafe. In many meetings, I got the impression Grace wasn’t really listening and she tended to agree with what we said. Not exactly great when your job involves telling the team what is safe and legal, and then get overruled by the Architect.

We came up with a compromise, and implemented it. Then when it came to the Code Review, Adam suggested removing one more of the sub-rules which I think would be perfect, but then Olivia [Product Owner] was reluctant for us to make more changes.

Then a week later, Olivia said she would arrange another meeting to discuss the rules because she felt it might be too restrictive. OMG. However, she then seemed to have personal grievances with Adam, so told me not to make the simple change, even though it would be what we want. She used the excuse of lack of Testing time.

Adam [Architect]

We shouldn’t be knowingly introducing bugs. Olivia [Product Owner] This is not a bug. It’s a change to the criteria and we are not going to change it a week before we finish. I am speaking to Carl [Customer Representative] about changing the criteria, and we’ll look at it then. Adam [Architect] A bug is any deviation from requirements. Why are you planning on changing it if it is not a bug? Olivia [Product Owner] That’s not a bug. You are right in the sense that we need to change it…we’re just not changing it now. I was happy to leave it as it was to get this out of the door. That’s my call to make. Mary [Technical Manager] There's a lot that's not right. But how long do we keep going until we give it to the customers?

A summary of how this situation appears to me:

There is a process, but if you declare you want to move the process to the next release, then it is fine.
It will take too long to change a few lines of code, so we ain’t doing it. Apart from when it is a comment on the Code Review, then we are doing it, apart from those that we aren’t.
It takes longer for Olivia [Product Owner] to argue against it than to fix it.

The CEO had recently posted:

“The most important thing we do every day is keep our users and their customers safe by managing risk effectively. I know you all know this, but it warrants repeating: safety is our number 1 priority all day, every day – regardless of anything else that is going on. It trumps everything. Please always remember that.”
CEO

Our Managers are like:

“Next release”

The Technical Manager change

Colin [Technical Manager] complains that Daniel [Developer] and I haven’t handled the project well – and it overran by over a month at that point. A week or so later, the team was on a call with other stakeholders and he said

“you guys have done a tremendous job”,
Colin

then said the delay “was caused purely by scope-creep and nothing to do with the developers at all”.

“Mary is in charge of the team since yesterday”
Colin [Technical Manager] with his timely announcement

I got the impression that Mary just wanted to get rid of the project, because it was dragging on for far too long.

The Testers had nothing to do since us Developers were working on the last few bug fixes. Tina [Tester] said she was just re-testing old features to pass the time, but also get extra confidence there are no remaining bugs. Mary [Technical Manager] replied:

“should we be doing testing when changes are ongoing?”
Mary

Well, in that case, this statement means testers should only be hired for a couple of weeks right at the end of a project – since changes are constantly ongoing. I think she might have intended it to mean like “you’d better not find more bugs!”, but if there are bugs, then you definitely want to find them before our users do.

On the last day of the Sprint, Tina [Tester] took annual leave. She had left her assigned items in the “To Test” column of the Kanban board. There was no evidence she had tested the item, so I don’t think it wasn’t a case of just forgetting to move to “PO Approval” column. Olivia [Product Owner] and Mary [Technical Manager] then decided to just close the items. No evidence, no demo – just close them so the Sprint looks good, and looks ready to release.

What annoys me is that Mary had criticised how we had run our team and suggested we don’t follow the process. She stated that she perfectly follows the process – which leads to her successful projects. Then I see her cutting corners like that.

Just like Colin, she criticises me to my face, but then when we are in a group she states:

“I think you’ve done a fantastic job given that there’s only 4 of you”
Mary

A few days later, I had finished what I was assigned, but there was a bug on the backlog which Mary [Technical Manager] seemed to want to defer (again, she just wanted to release the project as soon as possible). I thought it couldn’t be released without this fix. I stated that I would like to look at it and she said:

“don’t do any development work”
Mary

Seems I have the day off then. What is the point in me sat around doing nothing? If I fix it, we can decide if it goes straight in, or deferred for the next release. Or maybe I won’t even find a solution. She just seemed desperate to finish the project so wasn’t considering the seriousness of the bug, or thinking logically at all.

The Backstab

I didn’t actually sit around doing nothing. I worked hard and found a solution. I knew that there was no chance Mary would accept my changes, so I needed to come up with a way of convincing her. My plan was to get the testers to informally test it, then I can say that I have a fix, and the testers are happy that there’s low risk of introducing more issues – so she would be stupid to reject it.

Testers Tim and Tina were in agreement that the fix should definitely go out in the initial release, and they agreed Mary was making a bad decision to consider releasing without it.

Tim said he would “have to check with Mary if he was allowed to spend time testing it” since they got told not to test anything. I said “there is no way she would approve it, that’s why we are doing this informally/secretively”. If Tim and Tina test it and find a bug, my plan has failed and Mary never needs to know that I attempted it.

It’s a perfect plan, or it would have been, but Tim then goes and tells Mary that I asked them to test it.

“You gotta start being better with your comminications – it’s not just yours and Tim/Tina’s decision if something gets put into the release – it’s a whole team decision but ultimately mine and Olivia’s. You’ve messaged them directly asking if they can get it tested, and as much as they’ll also want to get it done, it then puts them under pressure. This is how you’ve all got to a stage of being all over the place and burning yourselves out, it’s got to stop please.”
Mary’s chastisement

I shouldn’t have to go behind people’s backs and make my own decisions, but the entire non-management side of the team thought it should go in, and only the managers thought it shouldn’t. As a team we care about quality, but managers are just focussed on deadlines.

I also didn’t appreciate that she is accusing my decision making of adding stress to my team.

80% coverage

As the project got towards completion, I recalled our stupid “Merge Ready” process that no one seems to care about other than the small team who came up with it. You have to justify the likes of Code Coverage, and ours was at a low figure like 10%.

I’ll write some blog posts about my reasoning on when tests are good or bad in the future. A simple explanation is that Units tests are good when covering requirements, but often developers write them to cover implementation i.e. verify a particular method is called; but not that the particular method actually works. When you switch implementation, you have to rewrite a new unit test, slowing you down. Unit Tests are supposed to help you refactor, but in this case, it is a hindrance to refactoring. We did a lot of prototyping early on, and knew there would be large re-writes, so Daniel [Developer] and I decided to worry about Unit Tests later on.

When I declared the low number of Unit Tests, Olivia ended up raising it to the Directors for some reason. Why was it their concern? Do they even know what Unit Tests are for, and what the coverage actually means?

It could jeopordise my chance of payrises (I was correct, I got 0% this year) and tarnishes my reputation.

When Mary joined the team, she berated me over this decision and made the dramatic statement:

“We can’t go on like this”
Mary

She then asked a couple of her favourite developers to write some Unit Tests for my project, completely undermining me.

The thing is, both Dean [Developer (Temporary)] and Dennis [Developer (Temporary)] spent way longer than they estimated, and they didn’t do as much as they hyped, then when it came to make the last few changes, it slowed us down.

We ended up around 22% in the end, and the managers decided that is fine.

That’s the problem with us though… Do 80% coverage because it’s important. But actually it’s not that important, so you don’t need 80%. But TRY get 80%, Why?, Dunno, but the Document says.
Tim [Tester]

On track

Dennis [Developer (Temporary)] was also asked to helping out address the Code Review comments. In some ways, this kinda slowed us down. I told him I had a branch with some changes in already and sent him a link to it so we can work together. When I caught up with him the next day, he said that he had been working on a few of the ones I already had done because he hadn’t looked at the link. What a waste of time.

When Mary asked for a progress report, Dennis reckoned it would take 1 day to go through 20 comments, but he had done 8 easy ones the day before, and we had the hard ones left. So I said it would be more like 4 days, but could take longer if they are surprisingly complicated. I was correct.

Manager: How's the project coming along?
Developer: It's on track.

The track: pic.twitter.com/kTWIE4CHZE
— Molly Struve 🦄 (@molly_struve) August 17, 2021

Final demo

On the final Project Demo, Carl [Customer Representative] was saying the sending process was far too slow. He had been on most of the demos from the start and saw the progress across the project.

The original version I showed him was incredibly slow, but I had managed to speed it up significantly. So despite him witnessing the project months ago, he said the performance was a concern and maybe users would think it wasn’t a significant improvement.

We had all kinds of people turn up to this final demo. People from support, training etc. We should have had those guys on the early meetings. They were prodding holes in the requirements and asking important questions. Although we gave good answers for most of them, I couldn’t help but think our changes might not be as useful as we thought.

If only we got more users involved throughout the project, rather than just some UX mock-ups before we started, and then a year later – give them the update and hope for the best.

I’d like to reiterate just how hard the team has worked. They have worked their little socks off
Olivia [Product Owner]

Conclusion

We were told the importance of the project, but because there wasn’t a direct commercial aspect to the project, I felt it wasn’t backed up by the number of developers assigned to the project. With only 2 developers, then key staff like Architects and Product Owners switching throughout the project; it just slowed us all down and made us all feel it was actually a low-priority project.

There were other morale-reducing aspects like when we were told the project was on hold, then Mary berating my decisions, and implying the failures were down to me.

There wasn’t a great understanding of the feature in many ways, illustrated by

how many requirements we discovered throughout the project,
the UX team being clueless about many aspects,
one Product Owner so clueless – it seemed he struggled to use a computer,
then switching to a clueless Architect that just went straight to the original architect.

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.