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.

Women In Tech: Software Developer Transition To Manager

Many years ago, I was listening to a podcast where a group of women were talking about their experiences in Software Development. I think Person A had started their own company so now didn’t do much development because they were now the CEO. Person B had switched to teaching software development and was going to take up a role as a “Developer Advocate” which I think is kind of a teaching role; making tutorials and promoting via social media. Then Person C seemed happy being a software developer.

In other blogs, I’ve briefly mentioned my observations with women in software development. I find there’s a much higher percentage of women that will desire to go into management, whereas many men seem to love the idea of a career constantly coding.

I’ve followed a few of the women from the podcast, intrigued where their careers would go. Person A and B are still CEO and Developer Advocate, respectively. Person C, who was happy being a software developer had apparently got their dream job at a big tech company early 2021.

“I’m so lucky I get to do something I love for a living”

Neary a year later, they announce they are taking a year out due to maternity. A few months later, they state how being a new mother has given them new inspiration as a software developer. I wasn’t sure if she had been coding in her free time, or was just posting for attention.

She claimed that she could:

“tap into new-found inspiration and creativity”
“think about more nuanced edge cases”
“Be more efficient”
“better at asking for help”
“better at asking the right questions”

The justification was that you have extra accountability, have to maximise how you spend time/money, ask for assistance when you struggle to support your child. It was quite tenuous and when people asked her to elaborate on how it really helps coding, she just accused them of “toxic masculinity”.

Only 2 months later, she announces that she applied to switch roles to become an Engineering Manager. Wait, what!? What happened to all that boasting about securing her dream role at her dream company? What about this new-found inspiration to be a better developer?

How can that mentality shift in such a short space of time?

“I kinda just fell out of love with coding”

People often say that social media gives people a skewed perception of people’s realities, because it is a filtered view: people only post the good stuff, and sometimes even modify the photos. If someone goes on holiday, you see the beautiful sunny beach, and the exciting scuba diving session. You don’t hear about the argument they had with their partner, or how they were bedridden with illness on the other days.

So was she lying about her love for coding? Were the development teams at this company not well suited to her mindset or ability?

When she did return from maternity leave, she then said he loved being a manager “way more than I liked being a software developer.”

I do find it odd that her mentality has always seemed to be focussed on promoting women in tech, and calling out “bro culture”, but then she has ditched being a developer and followed the stereotype of being a manager instead.