Category: Software Development

Shots Fired On Inexperience: Leaked Transcript

Intro

Even though all software developers should take part in code reviewing their peer’s code, I find most developers I work with aren’t that interested, so ultimately it comes down to a few high-ranking developers, plus me.

Recently, we have been rejecting a lot of bad code from inexperienced developers. It’s fine being inexperienced as long as you have a team to support you and help you eventually produce an effective solution. I think what happens is – there’s not enough seniors (or seniors that are actually good), so the code gets into their Project Branch. Then, when it is time to merge in for the Main release, experienced developers review the code and reject it.

I often specifically get asked by the Software Delivery managers to review code to ensure it is in a good state for the release. We had a group conversation where we were discussing the current bad state we are in. Software Delivery managers had asked Darren, Dave and me to review a project.

Participants

Darren, Dave – Principal Developers

Mark, Michelle – Software Delivery Manager

Me – the silent observer

I’ve given the developers names that start with D, and the managers names that start with M, so it might be easier to follow.

The Conversation

Darren  08:36

Whole lot of Nope in this Code Review.

Mark  10:52

Judging by your review comments, this doesn’t look to be something that can be resolved in the next couple of days. Does their implementation need to step back and restructure?

This might need another call

Have any of you guys been involved from a high level view for this Project?

Darren  10:55

The changes to the Database are totally inappropriate, and possibly way off the mark of what is needed.

Changes to the Database filing are non-trivial, if people don’t understand it, they need to reach out for help, not just smash code in.

Our product is going to turn into a legacy codebase very quickly if this behaviour carries on.

Dave  11:11

We do keep getting Code Reviews to Main with unworkable solutions. It’s highly frustrating, and a big waste of our time. Most of the time, the code or even the work item has not been reviewed by an expert-level person before a review to Main is requested. On occasion, someone gave correct advice in the first instance which was then ignored.

We need to fix this, and have help from you guys raising the problem.

Darren  11:14

The solutions feel like a very reactive approach to programming… Very little thought for design.

Mark  13:02

So have any of you guys been approached about this Project before this review  was raised?

Because I think this ties in with the whole idea of needing an Subject Matter Expert during the construction of the project rather than at the very end – something that we’re trying to drive forward

Dave  13:07

only about 2 days ago 😖

All our Subject Matter Experts are on the New product 😛

Darren  13:08

I reviewed the previous incarnation of it, without the concerning the Database changes.

“All our Subject Matter Experts are on the New product” Even so, I might have made time to discuss this with someone if asked.

Dave  13:09

yeah same

My point is that the day-to-day design and building of the projects is led by inexperienced devs. The developers are not being mentored while developing the code – but instead, at the end of their code writing; which should be something a lead developer is doing

It’s good to have the view throughout the dev process, or at least the start, as well as quality checks at the end – which should be a straight-forward “yeah, you’ve not done anything crazy, it meets the coding standards and is what we agreed”.

Mark  14:43

Yeah. This is all the good stuff which me and Michelle are trying to address as well. Trying to have a Subject Matter Expert assigned to each project during construction just for that overarching view, and to give a steer when needed.

I.e. a month ago this could have been caught at the start by saying “don’t make these changes within the Database itself” haha.

Darren  14:51

To be fair, you don’t need a Subject Matter Expert to tell someone not to dump a load of copy and pasted code from one module into another. Just someone with some experience in developing software to a higher standard than “Hello World”.

Michelle  14:52

“Hello world” just triggered some terrible school flashbacks.

Dave  14:52

Agreed, but as a business, we need Seniors to point out when people are constantly doing silly things to their line manager.

Michelle  14:57

It’s the experience aspect that is the biggest challenge, if there was a better ratio of experienced people to inexperienced people areas where skills are not good enough, they would start to improve (or at least that’s the theory).

Everything just seems stretched way too thinly at every turn. Hell, it isn’t just development that has the problem.

Darren  15:00

I’d also add: knowing anything about the teams working on the code would help.

I can’t know everyone, but I don’t even know what the teams are, what they are working on, who the seniors / leads are to discuss with.

E.g. if someone has a problem with code my team is doing, then I’d expect to be brought into the loop (either to explain our position, or to understand the other sides)

Dave  15:06

Who do we talk to, to at least pass the buck on the issue and raise it?

Michelle  15:11

I think it is more for the Development Director. It’s something me and Mark have brought in fashion everytime we come across an example. Difficulty has always been what to suggest to fix it within the limitations of what we got

Dave  15:21

Certain business types don’t understand the value of good engineers and will simply get the cheapest money can buy, in the hope the decent engineers will act as quality gates and stay with the company.

It must be soul-destroying, working away in an office across the globe, with ‘experts’ in the UK saying if your work can or can’t join the rest of the product. It’s not the right thing to do.

They need infrastructure mirroring ours, and separate projects would be a great way to achieve it

Darren  15:25

Last I knew, the other office still have Development and Testing as separate departments

Dave  15:25

This just reinforces the stereotype that Ben bangs on about: the company is ruled from the UK. Normal companies have different product sectors at different locations, because it mitigates these problems

Michelle  15:27

All valid points. What Dave is saying are points raised verbatim by more than a few people who have been in this world for a while. As for soul destroying: 100% agree it has to happen to catch this stuff; but each time it’s caught it will knock you back that little bit more

Just needs some kind of sensible structure that is adhered to. But then again – if no one is made to follow something, who will?

Dave  15:30

Yeah, for me, it’s responsibility, if you don’t have to pay for your actions, then it drives bad behaviour. But since we maintain our Product, having to pick up the bugs, see the customers complain on Social Media, see the error reports on live and backlogs grow, we’re a lot more invested

it is feeling more and more like, knock this feature out as quick as possible, throw it over the fence at UK law-enforcement, see if it goes through

👮

With responsibility you’re driven to get the right structure in place, you want to place experts/leads with the teams and have them monitor each day, because it soon becomes a very long task to get fixed last-minute. Top-down approach, drives the 9-5 job behaviour, lack of interest and minimum viable work ethic.

Darren  15:32

+1

Which goes back to my point about who the teams actually are. And we really need to do proper ownership. I still own large bits of our Product, because I care about them!

Dave  15:34

Same. Developing and code reviewing isn’t my job, really. However, I don’t want to see the product damaged and go to waste, because it helps people, genuinely.

Darren  15:34

Would be so much nicer to say “I don’t look after this any more, just do what you want and hope nobody gets sued” 😣

And it isn’t any one change that is unsafe, it is the poor quality, lack of adherence to existing designs, just piling code in, coupling stuff together, changes in the wrong place, etc that lead someone else to create a unsafe bug later – usually without anyone batting an eyelid.

OK, some changes are totally unsafe too – a few weeks ago, I have to point out that you cannot calculate an age in months by doing ageInYears * 12

😖

Dave  15:48

So, if I were to write some general comments without mentioning names, and send them to the Development Director, expressing that we’re at our wits-end, would anyone be prepared to back it up or be a part of the revolution?

Darren  15:50

Feel free to add my name to give clout to this

Dave  15:50

sure, I will of course send to you first to check

Summary

  1. Not enough people willing, or have the ability to review code. – I always find this a weird one; given that I reckon 90% of a developer’s job is reading existing code. If you cannot review code, then can you be a good developer?
  2. People aren’t putting enough thought into the architecture, or adhering to well-known standards like SOLID Principles – This is the kind of behaviour I saw in developers I’ve written about like Colin and Derek.
  3. The ratio of Juniors to Seniors is far too high, which not only is a problem now, it is a problem in the future when the Juniors haven’t progressed enough.
  4. The rapid hiring of staff has meant you don’t know who people are, what they are working on, and what their level of experience.
  5. Hiring cheap staff has caused some resentment/uncertainty with UK staff. Pay seems frozen and more jobs are moving abroad. It wouldn’t be that bad if the staff we hire are cheaper and are good, but they often just appear to be cheap.
  6. It would be better to split the offices into separate products so people feel like they have full ownership. There’s other problems too like time-zone differences and general communication problems.

More Bad/Weird Code

This is a follow on from my previous blog. I picked out a few examples that were bad changes from their Code Review, but maybe you didn’t think it was that bad in the grand scheme of things. So here is a collection of other weird stuff from the review.

Example 1: TrimEnd

TrimEnd method can be used to remove trailing whitespace, so if you have some text “Example text “, you can trim it down to “Example text”. You can also pass in an array of characters you want to remove from the end as well. So what happens if you create a completely empty array of characters?

return value.TrimEnd(new char[0]);

I had to test it out to see what it actually did. I was thinking there was a chance it could prevent the method from trimming anything at all. It actually does the same as TrimEnd() so removes whitespace only. So the char array is just redundant and looks confusing.

Example 2: The Weird “Any” check

Give a C# programmer a list called “textToProcess” and ask them to check if the list is Empty, and they will write this:

List<String> textToProcess = GetData();
return !textToProcess.Any();

or alternatively:

List<String> textToProcess = GetData();
return textToProcess.Count == 0;

But when you see this, then questions need to be asked.

List<String> textToProcess = GetData();
var empty = new List<string>();
return empty.SequenceEqual(textToProcess);

If you are a Junior and you are reading a book, working through tutorials, or even Google how to check a list is empty; what is the chance you will come across SequenceEqual? I’m sure I only came across that after a year of C# development. Weird choice.

Example 3: Unnecessary Converting

If we have a variable that stores a number. And I say that I want it to be a number. What are you gonna do to it? If you thought “nothing, it is already a number”, then you are correct.

If you thought something else, then maybe you did what this developer did. You could convert the number into text, then convert it back to a number!

This “number” variable is already a “long” aka Int64. They really are converting it to text, then instantly converting it back to the type that it already was. I really can’t emphasise how pointless this is.

Convert.ToInt64(number.ToString())

What’s weird is that I have seen this a few times over the last couple of months. Why are people doing this?

Example 4: Reinventing a private setter

There was a simple, standard property that you see all the time in C#.

public Validator Validator { get; set; }

And he changed it to

public Validator Validator => _validator ;
private Validator _validator  { get; set; }

So now the property is using this newer property syntax that looks weird to me. But really it is a property with a “get”, but no set. Then the second line is named as if it is a field (preceded with an underscore and first letter is lowercase), but has a property definition (get/set). Properties should be public but this is defined as private. This is incredibly weird.

Here’s how it should actually look (just has the word “private” added):

public Validator Validator { get; private set; }

So I comment on the second line:

this is named like a backing field, but you have defined it as a property. Do you need a backing field? couldn’t you just have changed the set to be a “private set”?

Me

Yes we need this backing field and it won’t allowed to set as private set

Developer

I had no idea what he was trying to say here. It definitely can be a private set. Plus there is no backing field, (just that he named it as such), and I had wondered if that’s what he was trying to achieve.

It isn’t a backing field, it is a Property that you have named as if it was a backing field, then the Validator just calls your new property. I think this change means Validator essentially only has a “get”. So surely you can achieve the same thing with public Validator Validator { get; private set; } then remove “private Validator _validator { get; set; }”

Me telling him exactly what to do.

Yes you are correct, but here we never wanted to do any changes in existing flow since it will give huge impact, so that used the different one without touch any of the current flow.

Developer

Just like the example in the previous blog, I tell him he has changed far too much and tell him to revert it/fix it properly, then he starts telling me how he doesn’t want to make more changes than he needs to. Most confusing.

I don’t understand what “existing flow” means here. Your changes have already removed the public set from Validator so no other class can set it. You’ve just done it in a way that people don’t expect by also changing it to call another property (which is named like a field). This is just confusing.

Me

He finally changed it, but it took some effort.

Example 5: Unit Testing

There was a method that returned an object that you could get multiple properties from. However they had created 2 separate methods that you call instead which was inefficient when you want all data. 

public string GetForename()
{
   return GetUser().Forename;
}

public string GetSurname()
{
   return GetUser().Surname;
}

When GetUser is a server call that reads from a database, why can’t we just grab the user once, then take the info from it as desired. We shouldn’t duplicate the server call.

User user = GetUser();
string forename = user.Forename;
string surname = user.Surname;

Why not just call the server once and use both properties?

Me

We made 2 method calls to get unit test coverage

developer

What is he on about? I could only see 1 test, it was for a completely different method and it didn’t really test anything.

        [TestMethod]
        public void CreateOutputFromDataTableTest()
        {
            DataTable dataTable = new DataTable();
            dataTable.Columns.Add("ID", typeof(string));
            dataTable.Columns.Add("Code",typeof(string));
            dataTable.Columns.Add("Term", typeof(string));
            
            dataTable.Rows.Add("9908", typeof(string));
            dataTable.Rows.Add("44D6", typeof(string));
            dataTable.Rows.Add("test", typeof(string));            

            var output = Transformer.GenerateOutput(dataTable);
 
            Assert.IsNotNull(output);  
        }

They are creating a table with 3 columns. They are then creating 3 rows but only populating two columns. You can tell by the data that they seem to want to create 1 row but with 3 columns set.

Then after they retrieve the output from their method, instead of actually checking if it has specific data they expect, they are checking if it isn’t null. So it could be any type of object: could have no data in any of the properties, it could have the wrong data in the properties. As long as there is an object it returns, then the test passes. If you are going to write such nonsense, why not make it brief like this:

        [TestMethod]
        public void CreateOutputFromDataTableTest()
        {
            var output = Transformer.GenerateOutput(new DataTable()); 
            Assert.IsNotNull(output);  
        }

Example 5: Incorrect Renaming

public bool IsQueryRunnable(Search search, out string message)

to

public bool IsQueryRunnable(Search searchGuid, out string message)

why was search renamed to searchGuid?

Me

The parameter name in the interface and in the implemented class should be same.

Developer

I think Code Analysis does flag that as a rule. But if the “interface” had the declaration with “Search searchGuid”, then surely the logical change would be to change it to “Search search”, because as I pointed out

but it isn’t a Guid

Me

Conclusion

This person is probably a junior. When I was a junior, I would ask for help when I was unsure – I would never claim incomplete code was production ready. I would also make sure my team members reviewed my code and gave me good feedback before I would want other teams to see my work. This work just seems careless to me.

Blindly Following Code Analysis

Recap

We have a Code Analysis report which gives metrics on code quality, and we want to improve the overall quality of our code over time. Managers hype up this report and put great importance on the Code Analysis report. I’ve found that in an attempt to please managers, Developers are going out of their way to try and address these issues, but are making a mess. Often they just tweak the code which fools the Code Analysis rules, but it’s actually far worse to read and understand – and that’s the opposite of what Code Analysis aims to achieve.

The Problem

I was asked to review a project but it was an absolute mess since the developer had gone out of their way to change files they had no reason to change. So it’s just scope creep, increasing the need for more manual testing, and increase risk of introducing a bug.

I am all for refactoring code in order to improve it. It needs to be done properly, and best done when the testers were already going to fully test that functionality anyway – then you aren’t increasing scope.

Example 1: ToDo

Sometimes developers may put in reminders in the codebase to go back and fix/investigate a particular issue. They do this by writing a code comment, and can write the phrase ‘TO DO’. They should address these areas, and remove the comments before they check in. Sometimes this can be missed.

So the rule is “Complete the task associated to this ‘TODO’ comment.”

And the explanation of such rule is: “TODO tags are commonly used to mark places where some more code is required, but which the developer wants to implement later. Sometimes the developer will not have the time or will simply forget to get back to that tag. This rule is meant to track those tags and to ensure that they do not go unnoticed.”

So what you need to do is investigate why the developer added the comment:

  • Do we need to fix a bug?
  • enhance the feature?
  • or is it unecessary and we can delete the comment?

What this developer did was none of those options. They simply deleted the word “TODO”

//TODO: Probably the wrong event?

becomes:

//Probably the wrong event?

and

// TODO - may need to set other properties here

becomes:

//may need to set other properties here

So it doesn’t help the developer at all, but it stops the Code Analysis report flagging it because now it is just a normal code comment.

The report identify only TODO text to remove, so that did like.

Explanation from the developer

Example 2 – As

We had code with loads of if statements that checked the object type. The code had maybe 10 branches. This isn’t a great design as any developer that understands the SOLID principles knows.

This code sample is a simplification, and way more brief. Just imagine it having more branches and doing some exciting things within each branch.

if (animal is cat)
   Cat cat = (Cat)animal;
else if (animal is dog)
   Dog dog = (Dog)animal;
else if (animal is horse)
   Horse horse = (Horse)animal:
else if (animal is baboon)
   Baboon baboon = (Baboon)animal;

The Code Analysis report doesn’t like it because there is one check in the if statement, then a cast afterwards which is basically duplicating effort – it is inefficient.

The developer had changed maybe 5 statements, but had left the other 5 statements as there were – so it was just a mess of a design.

var cat = animal as cat;
var dog = animal as dog;

if (cat != null)
//do cat stuff;
else if (dog != null)
//do dog stuff;
else if (animal is horse)
Horse horse = (Horse)animal:
else if (animal is baboon)
Baboon baboon = (Baboon)animal;

So I challenged him and asked why change cat and dog, but not horse and baboon? I also felt there wasn’t even an advantage doing it this way anyway. We now have several variables where only 1 will be set, and the rest are null. I felt we should either keep it as it was, or spend more time refactoring it out to get rid of the branching, and adhere to the SOLID principles.

I just simply resolved some existing Code Analysis issues which not raised because of our changes. It is a kind of help to resolving existing issues, It is very difficult to resolving all the issues as of now

Explanation from the developer

But he has gone out of this way to change things that weren’t part of what he was working on at all! If it is difficult, then why only partially “fix” it? Obviously I had to respond to that to point out his stupidity.

if you don’t want to change code that isn’t relevant to your work, then simply don’t change it. These changes aren’t useful to a developer. The aim of Code Analysis is to write good code, and not reduce all the numbers down to 0 on a report.

Me

I was quite proud of that one.

It does frustrate me that I want the codebase to be high quality; but others are more concerned about looking good in front of the managers – even if the codebase suffers as a result.

Recently, I’ve felt like the “Gatekeeper” to the codebase. Luckily the Software Delivery managers trust my judgement to whether code needs to be reworked or not, so I can successfully block changes like this. It’s all the managers in the Quality Assurance that care about the report.

Poor standard and My Inspirational Quote

I saw a Pull Request (Code Review) from a Senior developer. They had created a new property along the lines of HasSetOption but there was already a property called HasOptionSet in the same file which was for the same purpose. I looked at how they were using their new property and they never even set the value; they only read from it; so it wouldn’t have worked anyway. He even had 4 failing unit tests for bonus points.

I told my Apprentice about it to show him that not everyone is actually smart:

“Senior developers, Junior developers. They are all the same really”

Me

“From a learner’s perspective, that is both scary and reassuring lol”

Apprentice

“Yeah. Doesn’t matter how bad you are, you can probably find a Senior that you are as good as :laughing:”

Me

I thought that was quite an inspirational quote. It’s sad that it’s actually true.

Mentoring #4

I am mentoring an Apprentice who has never done C# before and this is his first programming job. So this is a diary of some-sort of his progression and my ability to mentor.

I haven’t written about my Apprentice in detail for a while. After spending a bit of time with him, he then got involved in more form-filling and exams for his Bootcamp company. It seems weird that we probably paid a premium for the Bootcamp company to train him, but yet he has been working with us for about a year now (I think) and we have barely managed to do anything with him. I really hope we don’t hire from those Bootcamp companies again. You may as well just hire someone and train them up yourself.

The good news is that my Apprentice has now “passed his apprenticeship” which means he has no more dealings with the Bootcamp company and now can work with me full-time.

At the moment, I’ve given him some Junior developer tests that employers normally give before an interview, so he is working his way through those. Ideally, he should have done them in an hour, so I guess he is a long way off from a Junior standard since he has had them a few days. But hopefully, I can train him well, so I’m quite excited for that.

Another complication is that my team has been assigned another Apprentice and my manager asked me if I could also train him. I made a great point that we have loads of Seniors in the team, so why aren’t they being asked? He said it was a good point and would look at promoting me. We will see how that goes.

Here’s a small summary of aspects I have gone through with my Apprentice. These are C# or Visual Studio topics:

  • Use some shortcuts like F12 to navigate methods
  • Ctrl+t “Go to type” shortcut that seems way better than Ctrl+f to find code
  • Use the “Rename” feature to easily rename variables/methods/classes setc
  • Break large methods into smaller ones – use Visual Studios “Extract method”.
  • Using breakpoints, the watch window, immediate window, “tracepoints” to debug. This is very important, you can never be a good developer without being good at debugging.
  • What the “Call Stack” is, and how to navigate through the method calls using Call Stack window.
  • How to create a branch in Git, push it to the server and create a Pull Request.
  • Class inheritance and polymorphism. 
  • When to use Abstract classes and methods.
  • The difference between “hard” casts and “as” casts.
  • Using some basic Linq (Select, Where, OfType).
  • Creating basic Winforms, and how to use Dock and Anchor to automatically resize controls as the form resizes.

The Dialog

I logged into our application and was greeted by a new dialog. When I see something new, I normally scrutinise it, and check out its functionality. I guess I’m quite inquisitive.

The thing is, the dialog also caught my attention because it looked really odd. 

My eyes were instantly drawn to the link which was in a strange position, and the font looked very large. I loaded up the code to see what was going on. Usually, we use Tahoma 9, but this dialog was using a combination of “Arial” and “Microsoft Sans Serif”, then the link text is 10.5 rather matching the other 9.5 (which is wrong anyway because it should be 9).

The buttons look a bit dark: they are using “Light grey” rather than the standard “Control Light”.

I also thought the Email label was a bit far away from the text box. The buttons are quite far away too.

So I quickly made a few minor tweaks to illustrate how I thought it should look like.

I probably should have left the text in bold, but I had already deleted my changes after taking the screenshot

It’s not perfect, but closer to what I expect to see. It’s easier for me to judge because I’m used to our application. Moving from the original dialog to another looks jarring since the fonts were clearly different sizes, whereas you readers have no comparison.

How does an entire team of developers and testers not notice these differences? I quickly checked the other dialogs they added and saw similar problems there. Sometimes spelling mistakes, grammar mistakes, randomly double spaces between words, and some uncentered text:

why not centre the text vertically?

It’s a complete lack of attention to detail by the entire team really.

I contacted the team, which was led by Colin who is infamous for his lack of attention to detail. His response was that he “was just following what the User Experience team gave him”. I checked the documentation he shared, and the mock-ups looked like how I imagined them to be – and not how his team had implemented them.

One of the Testers said something along the lines of “well, we debated these issues within the team, but ultimately – the Product Owner said it was fine”. 

When they did “fix” it, the developer didn’t send the review to me, but I noticed it and jumped in. It had already been approved by a member of their team, but was it actually good? Would it meet my standards? 

No. He had barely changed anything. 


The fonts were still the wrong style and size, the buttons were still the wrong colour, they were positioned incorrectly. He had even changed a few things no one actually told him to change – so made it worse.

Winforms Dispose Controls Quirk

Here is an interesting quirk with Winforms in C#. Calling Dispose on a Control has a side-effect of removing it from the container it has been added to.

So you can have code like this:

for (int c = 0; c < MainPanel.Controls.Count; c++)
   MainPanel.Controls[0].Dispose();

which correctly removes and disposes every control, even though you have a fixed index of 0. Since the control at index 0 is removed (as well as being marked as Disposed), the next control then becomes index 0. Therefore, it does call Dispose on every control.

So when you look at this code:

foreach (Control item in MainPanel.Controls)
{
   item.Dispose();
}

It looks correct, and would be the code that I would write had I not known about this quirk. However, it doesn’t work as intended. It will only remove and dispose some of the controls since you are modifying the list as you are iterating through it. 

This is the danger of adding “side effects” to your code. It can cause weird behaviour that you wouldn’t expect, and can be overlooked if you are just reading the code.

Age

There’s been a few occasions now where I have seen developers write code to calculate someone’s age and it has been incorrect.

I was talking to a developer, Alan who was ranting about some incorrect code, so I looked at the review.

string ageInMonths = person.AgeInYears < 6 ? (person.AgeInYears * 12).ToString(): String.Empty,

So there’s an ageInMonths variable. If they are less than 6, then we populate it, otherwise we leave it blank. When we calculate the persons age in months, we simply take their age and multiply it by 12 since there’s 12 months in the year.

Alan begins by pointing out the problem, but Pierre doesn’t understand what he is referring to, so Alan backs it up with more examples…

Alan: Is this correct? If I am 2 months old, I am 2 months old, not 0 month old?

Pierre: yes, it is optional for people less than age of 6

Alan: I don’t think saying someone is 0 months old when they are 11 months is correct. Nor is saying 12 months old when they are 23 months old

Pierre: thanks, will correct it

This isn’t something that is a language barrier. It’s not even a lack of programming experience. It is simply just a lack of common sense. You just have to be human to realise this isn’t how ages work.

He clearly didn’t test it because it should be obvious the correct values weren’t showing. We could instantly see it was a problem just by looking at the code. We didn’t need to see it in action to realise that.

Terrible Developers

Last week, I was reminiscing to myself about the developer that changed a random line of code which clearly wouldn’t have fixed the bug he was working on. It was also the same developer that repeatedly checked in “fixes” in an attempt to fix the build: Using the Build Server to test your code.

I said to my Apprentice, “how do you get into that mindset: where you write code, don’t even check it works then send it to your team to review?” but also “what happens if they did approve it and you are then embarassed when the Software Testers tell you it doesn’t work?“. Or even worse, “what happens if your bad code somehow went live?“

I wasn’t sure if I’d see something like that again, but it seems like we have hired some terrible developers in recent times.

Today, I was having a nosey at Pull Requests (aka Code Reviews) and saw some changes that seemed like the developer hadn’t even built the changes, therefore didn’t run their own unit tests they wrote, or done any kind of manual testing.

When you create a Pull Request, it triggers off an integration build. Then you can only complete your request if you have a successful build, and your team members have approved the change.

First of all, the build was failing, and it was a simple syntax error which Visual Studio would have highlighted, and definitely would have been brought to their attention if they clicked the Build button.

Looking at the code though, the first thing that I thought was funny is that they had a Code Analysis error. When you get these errors, most of the time you have to fix the problem it is highlighting. In the case that Code Analysis is wrong and you are smarter than it, you can add a Suppression. To make it clear why it is suppressed, you can add a Justification parameter and add your own explanation to it. To force people to do this, we have added a Custom Rule “SuppressionsMustBeJustifiedRule“. But the sneaky developer went and suppressed that! 

[System.Diagnostics.CodeAnalysis.SuppressMessage("Custom.Maintainability", "CM3000:SuppressionsMustBeJustifiedRule")]
		[System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Reliability", "CA2000:Dispose objects before losing scope")]

I then looked at their unit tests.

bool isValid = validator.IsValid(input);
Assert.AreEqual("True", isValid);

I was convinced that would give an error and it certainly does.

Assert.AreEqual failed. Expected:<True (System.String)>. Actual:<True (System.Boolean)>.

Comparing the wrong datatypes. A Boolean is a true or false value but it cannot be compared to a String (text) representation of it (“True” or “False”). What they actually need is simply:

Assert.IsTrue(isValid);

If isValid is true, then the Assert passes. If isValid is set to false, then the test fails.

Then the next set of tests were like this:

string successMessage = string.Empty;
List<string> errors = validator.Validate();
if (errors.Count() != 0)
    successMessage = string.Empty;

Assert.AreEqual(string.Empty, successMessage);

So Validate returns 0 or more errors. If there are 0 errors then the test passes because successMessage was initialised to string.Empty and we are comparing it to string.Empty. If there are 1 or more errors, then we explicitly set successMessage to string.Empty (which hasn’t actually done anything because it was already initialised to string.Empty), then we compare it to string.Empty and the test passes. So no matter what Validate does, the test will always pass.

What an absolute waste of time.

I also said to my Apprentice that it is perfectly fine to have a go and writing some code but failing. Then you can say “here is what I have wrote, it doesn’t build but I don’t know why, can you help me?”. Then I can go through it, give him some advice, and together we come up with a good solution.

The worst thing you can do is give up, but then try and get the crap code checked into the codebase.