Tag: Software Development

Incompetent Developer Tales Part 2

Although Junior developers can be useful, I have been against my employer’s over-reliance on them. If you get someone cheap with high potential, as long as you reward them, you end up with someone that knows your system, loves the company and is a great developer.

The problem is that we love hiring them but not rewarding them which means the best ones leave and the bad ones remain. The focus on cheap wages has led to more and more offshoring which has led to the rapid expansion of our Indian office. How do you hire so many people quickly? lower the standards. So now you have a high amount of incompetent people but they are cheap.

It’s not the fact they are Indian that is the problem, it is the fact the demand is high and the standard we had in hiring is low. The problem this has in the work culture is that it is easy to see a discrepancy in quality between the UK and Indian developers as a whole; which means you end up seeing them as inferior, despite some of them actually being good. The good ones tend to be the ones we hired in Senior positions, so they would naturally have higher wages anyway.

So building on from my recent blog on one particular developer, Here’s a collection of things other people have done:

Rollbacks

James has just rolled back someone’s changes who merged into the wrong folders.Don’t they think something isn’t right when it is showing [add] next to the main Database folder. Looks like they copied the folders up one level so now it is re-adding everything as a duplicate.

Dean 16:27:
haha
Me 16:28:
that change by Portia is mad when you look at the changesets
original patch
change to patch, 
fix db patching errors, 
rollback, 
rollback other changes,
rollback from xml file,
then Chris comes into undo the rollback
Dean 16:30:
it's just wrong that we've got people who don't know what they're doing
Me 16:30:
but it's cheap
Portia went wild on that second “rollback” and manually reverted the files.
removed 8 patches and added 1, instead of removing 1
it's amazing how many rollbacks happen these days
Dean  16:40:
Rollbacks worry me in general

“used for identification”

In general, SQL databases are designed to reduce redundancy. So if you have a table storing a list of “job roles”, then if another table references this information, you can link it together via an ID of the row. What you shouldn’t do is copy the data into another table. This means if the data needs to be updated, then you need to remember to update both, and this will double the storage space too.

I saw that a developer was doing this. It was only one column of text, but why were they copying it over into their new table instead of just referencing it?

Me
Is there a reason why this isn't being taken from JobCategory?
It is never returned in a stored proc call so there is no need for it

Vignesh
JobCategoryName used for identification of JobCategoryID and not used in stored proc. Thanks

Me
Regardless if the system uses the data, or if it is there for our Staff to read in the database; you would just write a query that joins onto the JobCategory table.
what if the JobCategoryName in JobCategory is updated? The names in your new table won't be accurate

Vignesh
JogCategoryID only used in stored proc/code, JobCategoryName is just an identification for JogCategoryID in the table. Thanks.

Me
So it needs to be removed?

Vignesh
JobCategoryName was removed since it is used in code or stored proc. Thanks.

Refresh Link

private void llbl_RefreshList_LinkClicked(Object sender, LinkLabelLinkClickedEventArgs e)
{
IEnumerable<ExecutionScheduleDetail> runningItems = _service.GetAllScheduleSearches(AuthenticatedUser.Organisation.Guid);
int count = (from runningitem in runningItems select runningitem).Count();
if (count>0)
{
LoadRunningSearches();
}
else
{
MessageBox.Show(
"This run has just completed and details can no longer be viewed.",
"Running Searches",
MessageBoxButtons.OK,
MessageBoxIcon.Error);
LoadRunningSearches();
}
 
}

Instead of just getting the count from the Ienumerable, they are selecting all the items they already have. Then since they are calling the LoadRunningSearches method in both parts of the IF, it may as well be moved out. So then the code would just be

if count == 0 , show message box.

when a review comment was left about not specifying the method call twice, he then just moved the GetAllScheduleSearches to a field, which meant it would no longer get the latest searches every time you clicked. Since the link is to “Refresh”, it wasn’t doing what it was supposed to.

If Constraint Exists

 if exists (Select * from sys.check_constraints where name = 'DF_Templates_AgeRange_AgeTo ')
 alter table Templates.AgeRange drop constraint DF_Templates_AgeRange_AgeTo;

I noticed there was a redundant space at the end of the constraint name so I thought it was likely that the check would never be true (unless SQL ignores it automatically?)

Me 4/28/2022
Is this space intentional?    
Vignesh 4/28/2022
Removed
     Joel 4/28/2022
Do you actually need the select check? I think you should be able to use the dependent patch mechanism instead.
     Vignesh 4/28/2022
I think it is not needed, we checked the condition to avoid any error. if its true it will execute
     Joel 4/28/2022
The constraint is added in patch 7.809, on which you've marked this patch as dependent. So this patch will literally only run if the constraint was created successfully.

 We have an attribute in the xml so you can state dependent patches, so will only run if the prerequisite patch has run. Vignesh was aware of it because he had used it, but then he also had this guard clause that possibly didn’t even work.

When told he didn’t need it, he then agrees that it isn’t needed, yet, put it in there so it would “avoid error”. Does he mean there was an error? Or just being overly cautious?

Spelling

inActive ? ResourceStatus.Inactive : ResourceStatus.Active

Sometimes, it’s the little things like this that annoy me. How can you write “inActive”, and not realise that you either:

  • have spelt “inactive” wrong,
  • or alternatively – someone else has when they created this enum

Therefore why did they not fix it? There’s clearly an inconsistency there.

In a similar fashion, I saw this recently:

//Recieved and Transalted

Both words are spelt wrong. It was also copy and pasted from another file. It does pose a good question though, if you copy and paste, do you think you should correct the spelling or leave it for maximum laziness? I guess the advantage is if you search for that text to try and find the original code, it’s better to match it as much as possible.

throw new NotSupportedException("Can't able to fetch template consultation details!");

Indians always seem to write “Can able” and “Can’t able” instead of just “can” and “unable”.

Untested code

string.Format("{0} {1}", _isRegistered ? "Not Registered:" : "Registered:", statusCode)

The logic was consistently backwards. It wasn’t a case that they typed it wrong and didn’t bother testing it. There were several files with the same type of logic. I pointed it out and they rewrote the entire thing.

Reviewing AxoCover, the Code Coverage Tool

A while ago, I wrote a blog about the Merge Ready Checklist, which was a process we have to prove we have followed to be able to complete and release our software project.

The process was created by a some experienced former Software Testers, now basically Quality Assurance Managers.

As part of the checklist, they then insist on having Test Coverage of 80% which I always think is an unreasonable ask. When I joined Development, we had Visual Studio Enterprise licences which have a Test Coverage tool available. However, we have since downgraded to Professional. So I asked what Test Coverage tools we can use because it needs to be something that IT have approved to download, and that we have a licence for. We were told we could use AxoCover, but I found it wasn’t compatible with Visual Studio 2019 or above, which was an inconvenience.

Ok, so let’s run it and see what happens. Firstly, you are greeted with a phallic symbol.

Test execution started.
      ___
     /   \
     |   |
     \   /
     /   \
    /     \
___/       \___
/               \
|    _______    |
\___/       \___/
AxoCover Test Runner Console

It’s supposed to look like their logo, which looks more like a fidget-spinner.

Here are the metrics it produces, before; and after.

CoverageUncoveredTotal
Classes11.4%30403433
Methods14.4%1778420770
Branches12.7%2908533308
Lines13.4%98844114114
Baseline figures. Code from the Main branch before my changes.
CoverageUncoveredTotal
Classes11.9%30343443
Methods13.3%1774720473
Branches12.0%2902032985
Lines13.4%98786114118
Codebase with my Project merged in.

Then I can’t even make sense of the statistics? Is that saying that I have removed methods (total has gone down!)? I have added a few classes with several methods each (these obviously contain lines of code and conditional statements so I expect all values to be higher (but despite more Classes, the number of methods and lines has decreased). I had also added some Unit Tests but maybe would have expected 30% on new code.

I asked Tech QA to explain the figures to me, and they were like “we dunno, we aren’t developers. We just look for the 80% number“. Then I point out that they were supposed to be judging the 80% coverage on NEW CODE only. This is for the entire solution file. So this doesn’t give them the evidence they want, and it’s not accurate either and cannot be trusted.

After running it several times and adding/removing code to see how the numbers changed, I then was suddenly low on disc space. Turns out Axo Cover reports are 252MB each! Yikes.

Testing AxoCover

Since the numbers were nonsensical. I decided to create a simple test project and run it on simple examples. Let’s see how it judges what is a line/branch/method/class.

namespace Axo
{
	public class ExampleClass
	{
	}
}

0 method 0 branches 0 lines

So a class definition with no methods and actual lines of code results in zeroes all around. It must ignore boilerplate class code.

namespace Axo
{
	public class ExampleClass
	{
		public ExampleClass()
		{

		}
	}
}

1 method 1 branches 3 lines

So now I have a method but it is empty. Seems the idea of ignoring boilerplate code doesn’t apply to methods. It must count the method definition plus braces inside the method for the line count, but it doesn’t make sense to count braces since that’s just to group related code. 1 branch is weird too, that should be for IF statements which we will test soon.

namespace Axo
{
	public class ExampleClass
	{
		public ExampleClass()
		{
			var a = 3;
			var b = 4;
			var result = a * b;
		}
	}
}

1 method 1 branches 6 lines

So now I have added 3 lines to the method. The line count has increased by 3 so it seems like it make sense. 

namespace Axo
{
	public class ExampleClass
	{
		public ExampleClass()
		{
			var a = 3;
			var b = 4;
			var result = a * b;

			if (result > 10)
				result = 0;
		}
	}
}

1 method 3 branches 8 lines

I’ve added 2 lines, where 1 is an If Statement. So now we have increased the branches but it has increased by 2. This must be the “implicit else” where the “result” is either greater than ten or it is less than 10, so there’s 2 paths. I’d still say that is 1 branch though.

namespace Axo
{
	public class ExampleClass
	{
		public ExampleClass()
		{
			var a = 3;
			var b = 4;
			var result = a * b;

			if (result > 10)
				MethodA();

		}

		private void MethodA()
		{

		}
	}
}

2 method 4 branches 10 lines

I’ve replaced one line with a method call to a new method. Method count increasing by 1 makes sense. Given the previous examples, adding a new method adds 1 to the branch count for some reason. In the empty method example, we got +3 to the line count, but now we only get +2, so that seems wrong. I don’t even think an empty method should increase the line count or the branch count, so the figures are becoming increasingly nonsensical.

namespace Axo
{
	public class ExampleClass
	{
		public ExampleClass()
		{
			var a = 3;
			var b = 4;
			var result = a * b;

			if (result > 10)
				MethodA();
			else
				MethodB();
		}

		private void MethodB()
		{
		}

		private void MethodA()
		{
		}
	}
}

3 methods 5 branches 13 lines

So now instead of an implicit else, I’ve made it explicit, and created another Method. Method count makes sense. Branch count has increased by 1 which I think will be for the new method and not the else. We have +3 to the line count but should we have 2 for the else, then up to 3 for the new method.

namespace Axo
{
	public class ExampleClass
	{
		public ExampleClass()
		{
			var a = 3;
			var b = 4;
			var result = a * b;

			if (result > 10)
				MethodA();
			else
				MethodB();
		}

		private void MethodB()
		{
		}

		private void MethodA()
		{
			{ }
		}
	}
}

3 methods 5 branches 15 lines

I was intrigued if it really was including braces. Some random braces gives +2.

	public class ExampleClass
	{
		public ExampleClass()
		{

		}

		public ExampleClass(string test)
		{

		}
	}

2 methods 2 branches 6 lines

I thought I’d reset and try again. So we have 2 methods, which as we have discovered means 2 branches with AxoCover’s metrics. It seems to count both methods as +3 lines.

namespace Axo
{
	public class ExampleClass
	{
		public void ExampleClassMethodA()
		{

		}

		public void ExampleClassMethodB(string test)
		{

		}
	}
}

2 methods 2 branches 4 lines

Looking back through the examples, I wondered if it is actually counting an empty Constructor as +3, but an empty method is +2. So this example has actual methods rather than constructors, and it seems to confirm my theory.

Discussion

When it comes to counting metrics in code, I think there is some degree of subjectivity to it. What even is a line of code? You could technically add many “statements” and put them all on one physical line. So you could look at the line numbers and see 1 line, but when actually reading the line of code, you can read multiple instructions. The analogy could be that you expect a list to be one item per line but someone could write a list on paper as a comma-separated list on one line. One is more readable than the other but it’s still valid either way. If someone asked you how many items were on the list, you could count them either way and end up with the same number. Therefore, I think the line count should actually be “statement” count.

I do think AxoCover’s definition of a branch seems wrong, and what they interpret as lines seems inconsistent and a possible bug in their logic.

On a larger and more complex codebase, the statistics it produces seems really nonsensical. So I think I have proved this tool isn’t worth using, and we definitely shouldn’t be using it to gatekeep on our Merge Ready Checklist.

Is Changing Text A Difficult Thing To Do?

Even though our UX Team has been around for a while, they never seem to understand what is possible in our software, so end up designing something that we cannot accurately recreate from their Figma designs.

I often think their standards change over time so it’s hard to predict what they would come up with. 

The UX Team asked what kind of formatting is possible in a Tooltip. You’d think they would know what is possible, and have plenty of old designs to refer to.

They said they had some upcoming projects that required tooltips containing large amounts of text; often with legal statements. They shared an example which had 3 sentences, then a Name, ID, Phone number, and address. So was a large amount of text in a tooltip, and some words were formatted.

I thought it wasn’t good UX to have loads of info inside a tooltip. Also, wouldn’t it be better to have that address somewhere where the user can copy and paste it? Seemed like a useful thing to have.

I often think it is good to evaluate the UX designs and give your own opinion on what’s possible to implement, but also suggest what would improve the user experience. You’d think the person employed in the UX team is the expert on user experience, but it’s best to not blindly accept it.

Cory House also seems to share this thought:

As a developer, I know I’m not a designer. But that doesn’t mean I should blindly implement designs.

I push back on designs that are:

  • Insecure
  • Confusing
  • Incomplete
  • Inaccessible
  • Inconsistent
  • Not performant

Assuring a good user experience is everyone’s job.

Cory House

More specifically, if we have an existing dialog, and the UX team decides to change what it says; you would think this is the simplest change possible. However, there could be a bit more to it than you would think.

I was explaining this concept to a Junior Developer. I was saying how I loved working with a Product Owner called Rob, who always asked “is that a hard thing to do?” no matter how trivial something sounded. He understood that there could be all kinds of crazy designs in the code.

In theory, it should never be hard. But sometimes adding more words means the words need to wrap onto the next line, and if the dialog hasn’t been coded to resize, then it might be a manual resize job. But if the “design view” is broken, then that makes it even more complicated.

The text might not just be set to specific words in the file where the control is. It could be dynamically generated then passed into another method, or maybe it even is set and read from a database. It’s still easy to change, but if you tried to search the source code for a specific word/words then you might not find it if it is dynamic or in the database instead.

I’m sure there have been times where, after investigation, you are like

“Rob, can’t we just keep the words as they are, I don’t have the skills to add a few more words!”.

Avoiding another boolean check

A developer wrote some code like this:

	public void RunScheduledJob()
	{
		if (_loggingEnabled)
			Log.Write("Main Job Complete", _category, _severity, _customData);
	}

	public void LogWrite(string message)
	{
		if (_loggingEnabled)
			Log.Write(message, _category, _severity, _customData);
	}

I’ve removed some extra code to make the example clearer, but the RunScheduledJob would do something then write to a log if the feature is enabled. The LogWriteMethod writes to a log if the feature is enabled.

Although it’s not a major improvement, the obvious thing to do would be to use the LogWrite method in the RunScheduledJob method like this:

	public void RunScheduledJob()
	{
		LogWrite("Main Job Complete");
	}

	public void LogWrite(string message)
	{
		if (_loggingEnabled)
			Log.Write(message, _category, _severity, _customData);
	}

So the reviewing developer Jim, pointed this out to Rich:

Jim: Could you call the LogWrite method here?
Rich: I could do but it would then evaluate _loggingEnabled twice for no reason.

Now, Jim and I were baffled what he meant. Even if it did have to check _loggingEnabled, it is a simple boolean so would only waste 1 millisecond to evaluate again. There’s no question of performance here; only clarity.

Rich then suggested this code as an improvement:

	public void RunScheduledJob()
	{
		if (_loggingEnabled)
			LogWriteNoCheck("Main Job Complete");
	}

	public void LogWrite(string message)
	{
		if (_loggingEnabled)
			LogWriteNoCheck(message);
	}

	private void LogWriteNoCheck(string message)
	{
		Log.Write(message, _category, _severity, _customData);
	}

So we have lost a bit of clarity.

It’s weird how sometimes developers have moments of madness and over-complicate simple things. This particular developer has 30 years programming experience!

Hidden Scanning Portal

Many years ago, my colleague Andy came up with a great software hack to fix a bug. I didn’t understand the fix at the time, so don’t remember the details, but the bug manifested as a red box replacing a UI control whilst the user was scanning a paper document.

Andy implemented a solution dubbed the “Hidden Scanning Portal,” a dialog box that remained invisible until the scan was complete, after which it was disposed of.

After a few months, another developer, Joe, convinced they had discovered a more permanent solution; removed Andy’s Hidden Scanning Portal. This action inadvertently introduced a new bug, so the Hidden Scanning Portal was swiftly restored, averting further complications.

Our Team Lead, Matt revealed that failing to fix the original issue could have resulted in a fine of £16,000. This revelation cast Andy’s quick fix in a new light, attributing a significant value to what might have otherwise been seen as a mere temporary solution. 

Andy’s reaction to the situation was a mix of pride and frustration. Despite his contribution to saving the company from a hefty fine, he lamented the lack of recognition in the form of a modest pay rise.

“and they didn’t even give me a measly 3% pay rise”

Andy

Quick fixes might not be ideal, and increase “technical debt”, but they can provide immediate relief, avoid hefty fines, and great stories to reminisce about.

Crazy Code Design

Here is a selection of poor code design that I have come across over the years.

Opacity Slider

Sometimes, programmers write simple programs to help them, or their team. When I first joined this company, they were using a more basic version of a source control like SVN, and someone had designed a program to help with Code Reviews. On the tool, they had a slider that changes the opacity of the dialog, so you can make it see-through. It seemed like a case of the developer simply doing it because they could, and not because anyone actually found the feature useful. I suppose you could be more creative if it’s only used internally, and not for a customer; but still incredibly pointless.

Law of Demeter

Law of Demeter, also known as the principle of least knowledge, advocates for a design where objects are loosely coupled and only communicate with their immediate acquaintances. This means that a method in a class should only call methods on:

  • Its direct components.
  • The object itself.
  • Objects passed as parameters.
  • Any objects it creates.

In practice, this means you shouldn’t have a massive chain of methods/properties.

One example of breaking this law that I saw in our code looked like this:

responseMessage.acknowledgements.conveyingTransmission.controlActEvent.reason[0].detectedEvent.code.code

Awful.

Partial Record

One of our modules can potentially show a large amount of data. The data can come from the local record, or if there is a sharing agreement, it can come from other companies that they share with. There are certain screens where you don’t need all the information, so to try and cut down loading times, we have this concept of a Partial record and a Full Record.

	public IRecordPartial Partial
	{
		get
		{
			return IsFullLoaded ? Local : EnsurePartial();
		}
	}

The logic, and wording gets confusing real fast. The property above is called Partial, but there is a check for IsFullLoaded which implies that Local can be Full, and not just Partial like the property says. When you look further into the code, Local might not even be local because it can contain shared. Mindbending.

PercentNull

Coming up with descriptive names is always a challenge in programming, and there is occasions where naming can be ambiguous. However, I have no idea what a method called PercentNull does here:

transactionOut.Surname = helperFacade.PercentNull(customerDetail.Surname);

If it assigning the result to Surname, then it should be returning text. Nothing immediately obvious comes to mind if you are passing in Surname to a method called PercentNull and getting a Surname from that. So it’s not like it is returning a percent number if it can, or Null if it cannot. Or returning the percentage that the text contains whitespace. 🤷

How High

	public enum SeverityScale
	{
		None = 0,
		Low = 1,
		Medium = 2,
		High = 3,
		ExtraHigh = 5,
		VeryHigh = 6
	}

We have this enum to represent the Severity. It makes sense until you get further than High. Extra High makes sense to be more severe than High, but should Very High be higher than Extra High? Extra and Very sound like synonyms. You need something more extreme like Extremely High to make it immediately obvious what the ranking is.

Focussed or not?

	public void SetSearchBox()
	{
		SearchFocused = false;
		SearchFocused = true;
	}

When you see code like the above, it is most likely that the code is doing something weird, and because no one worked out how to fix the original issue, you then end up writing more weird code to work around that. These “hacks” just stack up and are hard to remove. If you try to do the right/honourable thing and remove a hack, then you will probably see some strange behaviour which then means you remove more code, and maybe more code when you find more problems. Repeat until you are left with the original issue which you then have to fix.

So how can setting a property to false, then immediately setting it to true actually do something? Probably if the property has loads of logic and side-effects. Probably best not to look. 🙈

Random Parameter

		public Organisation(bool randomParamToChangeSignatureForFasterConstruction)
		{
			// Had to create a new constructor that doesn't initialise internal state, param isn't used.
		}

The thing that is scary about this strange code, is that it was written by one of our smartest developers. I have no idea why he had to create an extra constructor. Presumably the parameter is there because the “default” constructor already existed. Intentionally not initialising data sounds like a recipe for bugs though. Maybe it needs a redesign.

Slow Request

I remember my Dad telling me a story of a software team putting a delay into their code. Then each month, they would simply reduce the delay a bit and tell their managers they have been working hard on performance improvements. 

if (Helper.SlowRequest)
	Thread.Sleep(15000);

I found the above code in our codebase but it relies on a configuration value being present in the config file. It was present and set to true by default for Developers though so would always be slow. Changing the value to false would speed it up, but you have to know that it exists to change it.

<add key="SlowRequest" value="true"/>

Although it doesn’t affect our customers, there’s always the chance something will go wrong one day and it could affect them.

Boolean “Mode”

If you want to handle many options, you often use an “enum”. Using a “Boolean” which represents 2 values (true/false) is a very weird choice…

 <param name="mode">If set to <c>true</c> then [mode] is a delete operation, else add, edit and none operations</param>

so

true = delete

false = add OR edit OR none.

If you put the wrong value, then there’s an if statement after. So if you say it’s a delete and it isn’t, then things don’t get deleted.

		if(!mode)
		{
			if (@event.updateMode != vocUpdateMode.delete)
			{
			}
			else
			{
				if (@event.updateMode == vocUpdateMode.delete)

Not Supported Property

	public virtual String UploadedBy
	{
		get { return _document.Observation.EnteredByUser.DisplayName; }
		set { throw new NotSupportedException("Setting UploadedBy is not supported"); }
	}

Isn’t that Set a complete dick-move. If you call it, it will crash. If the setter wasn’t there at all, you would know it shouldn’t be set.

I guess it could be designed with the expectation that you would override the property. However, I thought it wouldn’t be set correctly, because the “get” returns a massive chain of properties. So it’s not just the case of setting a variable, it’s actually document.Observation.EnteredByUser.DisplayName that needs to be set, and that is breaking the Law of Demeter anyway.

Mutual Exclusive

This is gonna end in tears

private Boolean _isSingleClick;
private Boolean _isDoubleClicked;

Not only are there better ways than detecting clicks, when you have multiple variables tracking similar concepts like this, you can easily end up in invalid states. If _isDoubleClicked is true, then you would expect _isSingleClick to always be false. But it is easy to make a mistake in the code and not set it which then leads to a bug.

Notifications

		public bool IsNotificationConfigEmailEnabled()
		{
			if (!_configSmsEnabled.HasValue)
				_configSmsEnabled = NotificationConfiguration.IsEmailEnabled;

			return _configSmsEnabled.Value;
		}

The fact that this code had been there years means it should work. But when the property is supposed to be checking if Email is enabled but the code only looks at SMS enabled; then who knows how it works.

Resizing

int parentSize = Parent != null
                ? Parent.Width
                : Screen.PrimaryScreen.Bounds.Width;
 
var availableSpace = parentSize - _clientLocation.Y - _yOffset;

What a nonsense calculation that is! We want to calculate the height of a pop up box and make sure it can fit within the window. So we look at the width of control, or maybe the width of the monitor that they might not be using (if they actually have the program on their secondary monitor), then minus the Y coordinate of the window which would be related to height, and not width.

Splitting

Sometimes programmers like jamming as much code as they can on what is technically a single line. It is an absolute nightmare to debug chained logic like this:

return
	RequiresSingleItemOrder(item, customerRequestsSplit)
	?
	null
	:
	batchOrders
	.FirstOrDefault(
		orderInstance =>
			(orderInstance.IssueMethod != IssueMethod.Electronic || orderInstance.Items.Count() < 4)
			&&
			!orderInstance.Items.Any(m => RequiresSingleItemOrder(m, customerRequestsSplit))
			&&
			orderInstance.IssueMethod == issueMethod
			&&
			orderInstance.OrderType == OrderType
			&&
			orderInstance.IsUrgent == isUrgent
			&&
			(!ShouldSeparateControlledItems
			||
			orderInstance.AnyControlledItems == item.IsControlledItem)
			&&
			orderInstance.Warehouse == Warehouse
			&&
			!Authorisation.Separator.ShouldBeSeparated(authoriser: authoriser, orderAuthoriser: orderInstance.Authoriser)
			&&
			(!ShouldSeparatePrivateItems
			||
			orderInstance.IsPrivate == isPrivate)
			&&
			MatchesForRepeatOrdering(item, orderInstance)
			&&
			NullAndNonsequentialEqual(separationTypeIds, orderInstance.SeparationTypeIds));

Funny Youtube Comment

I saw this comment on a programming video on YouTube. It is remarking on how you can write really confusing code as a way to increase your job security because you can be in charge of code that only you can read:

And remember, kids – if you nest multiple null coalescing operators into a single line of poly-nested ternary operators, that’s called “job security” – cause ain’t no one wanna maintain that code when you’re gone.

return a> b ? a < c ? a != d ? e ?? f ?? 0 : f ?? g ?? : 0 e ?? g ?? 0;

Typescript Example

Years ago, we started rewriting our C# program using Web Technologies. Since everyone was new to Javascript and Typescript, everyone wrote awful code. But then you didn’t know if it was good or bad. One of the first bits of code I saw when I joined their team was this:

export const Actions: { [key: string]: <T extends {}> (value: T) => IAction<T> } = {
 setModuleName: <T extends {}>(value: T): IAction<T> => CreateAction<T>(ActionTypes.setModuleName, value),
};

Don’t ask me what it says, because I have no idea.

Contradiction

InsertUpdateResourceImmutableProperties

It’s so frustrating to read, it makes you want to punch someone. Immutability means it shouldn’t change after it has been created. So how can you Update something Immutable? Luckily it has a comment explaining it:

   -- If we are about to insert the most upto date version of the resource, then we should update the
    -- resources immutable properties, because they might have changed in the source system. (Even though they shouldn't).

Well, I am still none-the-wiser.

Boolean Extensions

If you have a boolean variable and you want to check it is false, you can just write:

			bool example = false;

			if (example == false)
			{
			}

Now with this handy extension method:

public static class BooleanExtensions
{
	public static bool IsFalse(this bool boolValue)
	{
		return !boolValue;
	}
}

You can now write:

bool example = false;

if (example.IsFalse())
{
}

What’s the point in that? No advantage really is there?

Not Set Up

		get
		{
			if (_setup)
			{
				_setup = false;
				return true;
			}

			return _noDocumentsInError;
		}

In the get property, we check the value of _setup. If true then we set it to false, and the overall result returns true. However, if we immediately call the same property, it will just return the value of _noDocumentsInError instead. It’s bad design to have side-effects in a get. Gets are supposed to return a value, and not set things. Then we seem to have different fields tracking different concepts which just looks like it will be prone to errors.

Reload

	public void ReloadTriggers()
		{
			// To be implemented
			return;
		}

This code doesn’t even need a return statement. It is just there for the bantz. When this code is in a module that is very error prone, and you have a method that is not implemented and does nothing at all, then doesn’t give a good impression does it?

Dialog Events

            _msgBox.ShowDialog();
        	return _continueWithSelection ? DialogResult.OK : DialogResult.Cancel;
}
 
private void Cancel_Selection(object sender, EventArgs e)
{
       _continueWithSelection = false;
       _msgBox.Dispose();
}

You can get the dialog result when the dialog is closing. However, this developer has decided to create their own event handlers for the button clicks, then using the variable to decide if it is a DialogResult.OK or Cancel.

NoEncryptionEncryptor

new NoEncryptionEncryptor()

Is this an Encryptor or not?🤔

SillyException

You throw an Exception when something bad has happened. However, in our code there is a concept of SillyException where you are supposed to ignore it because apparently, it isn’t a real error. It’s only used in a specific part of our codebase though so goes undiscovered for ages. A colleague Michael found it again and we were reminiscing on a bug I found.

Michael 11:33: 
if (mapped is SillyException) return 0;         // Avoid throwing the SillyException 

Me 11:33: 
I love sillyexception 

Michael 11:33: 
/// <summary>Represents a data provider exception that doesn't indicate an actual error, 
/// and should be ignored.</summary> 
[Serializable] 
public class SillyException 

Me 11:34: 
I remember David going on about SillyExceptions and it took me ages to realise he was referring to an actual thing 

Michael 11:35: 
"I keep getting this SillyException" 
what kind of exception is it David? 
IT'S A SILLYEXCEPTION 
YES, BUT WAT KIND 

Me 11:35: 
yeah, it did go something like that 

Michael 11:35: 
haha 

Me 11:37: 
I think it was around the time he spent weeks investigating that bug I found that no one else could recreate 
and I thought he had worked it out when he said he was getting a SillyException

 lame async call

var result = client.BeginUploadFile((Stream)payload, endpoint);
bool uploadCompleted = false;
while (!uploadCompleted)
{
	uploadCompleted = true;
	if (!result.IsCompleted)
	{
		uploadCompleted = false;
	}
}
if (!uploadCompleted)
{
	throw new Exception($"Failure occurred while uploading the data");
}
client.EndUploadFile(result);

weird that they set it to true, then set it back to false. Then it will never get to the exception, it will just be an infinite loop because the check is outside the loop that will only exit if it is complete.

Is this just a reimplementation of a lame async call?

Incompetent Developer Tales

We had an Indian Developer who wasn’t very good and he wasn’t that great at communicating in English. I don’t mind people starting out as a Junior and improving over time, but sometimes I think you need to have a mindset for quality and care about your work, and Manikandan didn’t seem to have that.

Text

Making a change which involves updating text is one of the easiest changes in software development. This particular area of the code had unit tests to check the message we were displaying to the user. So if the requirement changed, you would have to update the unit tests, then change the actual logic. Manikandan changes the code, but for some reason puts an extra space at the end, and doesn’t update the unit tests.

“As suggested the changes were made and PR got patched.” 

Manikandan

Another change he did involved changing the file path of where a specific file (sqlite) was meant to be. He ends up changing the file path for some other file instead. I pointed it out and he claimed he had tested it.

“I have dev tested this yesterday, it looks like i have missed sqlite folder path.

Mistakenly i have taken up the cache folder.

Will update and patch the PR. Apologies as i have misplaced the path location mistakenly and have corrected it now and done dev testing”

Manikandan

Corrupt Files

When viewing files that have been attached by users, some of these we natively support like text and images, whereas specialist files we allow opening externally. Some we blocked completely. 

A new change would allow files to be saved regardless if they were unsupported or we suspected them to be corrupt. We will inform the user of this though, so this was the 2 requirements:

When you attach:

This file “xxxxx.tif” has been successfully attached. However the file format or file extension is not valid. Verify that the file has not been corrupted and that the file extension matches the format of the file.

When you select “open file”:

This file “xxx.tif” cannot be opened because the file format or file extension is not valid. Verify that the file has not been corrupted and that the file extension matches the format of the file.”

He writes the following note in the bug report:

Made a proper message information need to be displayed to the user as well as restricted further attachment of the corrupted file into user record history.

Manikandan

So that sounds like he is blocking suspected corrupted files, which is not the requirement.  When I looked at his code changes, it seemed like he had done the wrong thing.

So I ask him:

“Have the requirements changed since the product owner’s comment? It looks like you have the message that should be displayed, and it sounds like the attachment should still be saved.”

Me

he replies

“Corrupted Document will not be attached to the record. Once again checked in the database to confirm”.

Manikandan

So I reply again:

“Who has decided that it shouldn’t be attached to the record? The Product Owner said it should be attached and there should be a specific message shown to the user.”

Me

“Apologies for that, will check once again with product owner”

Manikandan

…

Got the update and have made the changes as per Document gets attached to the record irrespective of the document was corrupt or not. Will show a message to the user regarding the file corruption but allows the user to attach the doc irrespective of its correctness

Manikandan

So it seems he finally understands the requirement. However, when he submitted his change to review, it will show a message on Loading (but not the one the Product Owner asked for), but will still crash on Save so he hadn’t really changed anything.

Finishing my work

I had made loads of changes for a new set of validation, but didn’t have time to finish it off as I got moved onto a different project. I had to hand over my work to him. He made some changes and submitted it to me for review. One file I had already wrote the code to show the new message, but then he had added another IF statement, and added the exact same message, however it had some extra spaces in it! So redundant code, and it was wrong.

Another requirement was 

“This Warning should be available irrespective of Admin Org flag...”. Yet he had an If statement to check for the flag.

else if(IsAdminOrg)
 {

There should be no IF statement because it should happen regardless if the flag is on or off.

In another code block, his logic was based on a check :

specialisedCode.Equals(SpecialisedCode.HDS3)

This equals check wouldn’t actually ever equate to “true” because the comparison was against different object types. And even if it did work, won’t this display the same text twice? He already added code to add it to the list, then had some code to add it again if the If statement was true:

 if (matchedItems != null)
{
	var displayName = matchedItems.GetAttribute("displayName");
	var specialisedCode = matchedItems.GetAttribute("code"); ;
	if (!allComponentNames.Contains(displayName))
	{
		allComponentNames.Add(displayName);

		if (specialisedCode.Equals(SpecialisedCode.HDS3) || specialisedCode.Equals(SpecialisedCode.HDS4))
		{
			allComponentNames.Add(displayName);
		}
	}
}

In another part of the code, he had this code

var hasExceptionalComponents = HasExceptionalComponents(sectionDefinition);

if(!data.IsSystemLibraryItem || (data.IsSystemLibraryItem && hasExceptionalComponents))

So if the first part of the if statement wasn’t true (“!data.IsSystemLibraryItem”), then it would evaluate the second part, but data.IsSystemLibraryItem would always be true (since it is just the inverse, and the opposite of “false” is “true”). So that bit could be excluded; so yet more redundant code from Manikandan. This would mean you could write:

if(!data.IsSystemLibraryItem || hasExceptionalComponents)

but what was he doing if this statement was true or false? Here is more of the code:

if (!data.IsSystemLibraryItem || hasExceptionalComponents)
	FormListFromTemplateSectionDefinition(sectionDefinition, allComponentNames);
else
	FormListFromTemplateSectionDefinition(sectionDefinition, allComponentNames);

That’s right, he was doing the exact same thing, so that code block can actually be one line:

FormListFromTemplateSectionDefinition(sectionDefinition, allComponentNames);

So the majority of the code he writes is just redundant. How can you be so incompetent?

When I flagged it to Manikandan, he said

or i am i missing it somewhere, sorry. lil bit confused on this, as i am alone working on this project.

Manikandan

This isn’t a misunderstanding of the requirements, it’s just a lack of care and thought.

Email Regex

When it comes to data entry, you can perform validation using Regular Expressions which defines a pattern/structure. Certain identifiers have rules, for example; an English postcode (I think) is 2 letters, 1 or 2 numbers, usually formatted with a space, then a number, followed by 2 letters. eg  HX1 1QG.

Software developers often come up with convoluted rules to try and define what an email address looks like, but I think they are inconsistent even between email providers that certain special characters can be excluded.

In our software, a developer had decided to change the regular expression in our email address validation. It was complex before, and it looked more complex after the new developer’s changes.

This was what it was:

return (Regex.IsMatch(emailAddress, @"^[A-Za-z0-9_\.\-&\+~]+@((\w+\-+)|(\w+\.))*\w{1,63}\.[a-zA-Z]{2,6}$"));

And he changed it to this:

return (Regex.IsMatch(emailAddress, @"^([\w-]+(?:\.[\w-]+)*(?:\'[\w-]+)*)@((?:[\w-]+\.)*\w[\w-]{0,66})\.([a-z]{2,6}(?:\.[a-z]{2})?)$"));

Now, glancing at that, I have no idea what the advantage is. Some developers just assumed it was a good change and approved the change. What I noticed is that the end part was changed from [a-zA-Z] to [a-z], which means the last part of the email eg “.com” can only be written in lowercase, but previously we accepted “.COM“. As far as I am aware, email addresses don’t care about casing, so although it would be nice if users only entered them in lowercase, it seems an unnecessary restriction. Was it intentional? What else in this validation is different?

If he had unit tests to show the valid emails, and prove his change wasn’t flagging valid emails as invalid; then I would be more comfortable approving it. But there were no unit tests, and I didn’t understand the change.

Another developer pointed out that the advice for emails is usually just to go for the simple *@*.* validation which translates to; “it must have  some characters, followed by an @ sign, more characters, then a period followed by some characters“.

Email validation is surprisingly complicated, so it’s best having a simple restriction just to filter out data-entry errors. Having complex restrictions can then exclude people’s valid email addresses and you definitely don’t want to do that!

The Curious Case of Paul’s Holiday: A Software Saga

This is a story of how you can find a bug before users report it, but don’t end up fixing it due to other priorities or communication breakdown.

I was trying to investigate a bug with our software, and ventured into a related module to try and configure it. However, it crashed. After looking at the code where the crash occurred, I found the developer that had likely caused it, and it was a change he made in the previous month. I sent a message directly to Paul to alert him to it. There was a chance the change hadn’t been released yet, and he could fix it in time. Or if it had been released, then it probably needed to be fixed urgently because the feature looked unusable as far as I could tell.

Paul replied stating that he had realised his change had caused a bug elsewhere and this was essentially the same mistake, so he would add it to his list of things to fix. However, he was going on annual leave in a couple of days so would need to hand it over to his team.

He sent an email to his team and copied me in.

Recently my change had caused a bug in Resource Manager. If you archive and unarchive a template it causes a crash. It became evident that there is the potential for two more crashes to occur that are related.
  
Schedules (clicking on any folder, demonstrated by Praveen) 
Assessment (found today, but we believe this only is used by 1 site) 
  
Root cause is that the control that I changed is the same in these other areas, however these “resources” do not have a file size.  
  
I have created a bug for it, and raised a draft change which I believe would fix the issues (I have done some local testing but more required).
  
This could be picked up by someone else in my absence if it needs to be progressed asap (code is live and not behind a feature flag).  
  
These discoveries have been through our own efforts, not through Support.  
  
Needless to say, this feature has had its fair share of issues, and I will write up a report/lessons learned document upon my return.

A few days later, Paul returns back from his annual leave.

[15:13] Paul Marshall 
you know that bug in assessments module you highlighted the other day?  
Major Incident raised. Only 2 complaints so far, but we have found more occurrences when browsing the error logs. 
 
[15:35] Paul Marshall 
I raised the PR (draft) but did not progress it as I was literally going on holiday that day. I emailed a bunch of folks and gave the full picture to them, including all the evidence and PR but it wasn’t moved on. 
When I was back I raised it and it sat there not reviewed by the team even though I asked repeatedly to have it reviewed. 

Sorry, bit of a rant there....   
its moving now 
 
[15:40] Me 
let's write a blog post for Viva Engage about how we came close to preventing a Major Incident 
others foiled our attempt

Emails were flying, managers were sighing, and somewhere, a developer was crying.

AI
The Curious Case of Paul’s Holiday: A Software Saga

Once upon a time, in the not-so-far-away land of Software Developmentia, there was a developer named Paul. Paul was a diligent worker, known for his sharp eye for detail and his love for datagrids. One fateful day, Paul made a change to a datagrid that was nothing short of genius… for a certain group of people. For another group, however, it was a disaster waiting to happen.

Paul, with his developer senses tingling, caught the potential calamity before it unleashed chaos upon the unsuspecting second group. He crafted a fix as elegant as a ballroom dance and sent out an email to his manager and team, detailing the issue, the solution, and the testing needed. It was a masterpiece of communication, a symphony of technical jargon and urgent pleas. And with that, Paul packed his bags and set off on a well-deserved holiday, visions of bug-free code dancing in his head.

But alas, in the twist of all twists, Paul’s team, perhaps too mesmerized by his email’s eloquence or too entranced by their own holiday plans, did nothing. The fix sat, lonely and unimplemented, as Paul’s toes wiggled in the sand and his out-of-office reply cheerfully told people he’d be back soon.

Upon his return, Paul found his inbox flooded with the digital cries of users caught in the datagrid debacle. The issue had gone live, and chaos reigned. Emails were flying, managers were sighing, and somewhere, a developer was crying.

What can we learn from this tale of holiday woe? Perhaps it’s that communication, no matter how clear, is only as good as the action it inspires. Or maybe it’s that one should never go on holiday without ensuring their work is done… or at least assigned to someone who isn’t equally holiday-bound.

So let us raise our mugs of coffee to Paul, the developer whose holiday will forever be remembered in the annals of Software Developmentia. May his next vacation be free of datagrid disasters and full of completed tasks.

And to all the Pauls out there, a word of advice: double-check your team’s to-do list before you hit that out-of-office button. It might just save your holiday… and your datagrids.

DataGrid & The Custom Row Selection

We have one module where you select rows from a grid, then can perform actions on multiple entries. The first column in the grid contains checkboxes. So whatever is ticked is what the actions should apply to. However, if you check the box, it also highlights the row. So there are actually 2 things visually that show the user what is selected.

Clicking anywhere on the row should highlight the row, but since highlighting and the checkbox shows what is selected, then we need some custom code to also check the box when the row is highlighted.

My team had made a tweak to the underlying control, which had broken the checkbox being ticked when the row is selected.

When we recreated the bug, we realised that – because there’s 2 ways of selecting (highlight and checkbox), when you click buttons to perform actions, it actually checks against what is highlighted. So even though we had introduced a bug that it no longer checks the box, it doesn’t actually cause any problems at all because the checkboxes are merely a visual thing.

A simple fix would be to just remove the column since it is redundant. Then a lot of code could be cleaned up.

“One thing we have noticed is the tick box isn’t actually needed, highlighting the column gives you all the same functionality as selecting the tick box.” 

Tester

However, our Product team said that even though multiselect behaviour has always existed, many users weren’t aware and so clicked each checkbox one by one. So it sounds like some users like clicking rows, some like using checkboxes, and some like using keyboard shortcuts.

The keyboard behaviour seemed rather strange too and caused extra complications with the code. You can press down/up which selects/deselect the row below/above the row that is currently focussed. However, there is no visual indicator of which row actually has the focus. Other shortcuts include pressing Spacebar which toggles the checkbox on/off. Pressing ctr+up/down jumps to the top/bottom respectively and checks the box (but if it is already checked, then it doesn’t uncheck it; which is inconsistent with up/down without the Control key). You can also press ctrl+a which selects all rows, but you cannot deselect all rows.

It really illustrates how something basic can be complicated to make all users happy. Then the more custom code you add, the more likely there’s bugs and other inconsistencies.

I was noticing inconsistencies with the shortcuts. So when I had implemented my fix, I was convinced I had broken them.

private void ListData_KeyUp(Object sender, KeyEventArgs e)
{
	if (listData.CurrentCell == null)
		return;
	
	if (e.KeyData == Keys.Down || e.KeyData == Keys.Up)
		HandleGridRowSelection(mouseActionOrSpaceKey: false);
	
	if (e.KeyData == Keys.Space)
	{
		_isSpaceKeyPressOnCheckBox = listData.CurrentCell.ColumnIndex.Equals(_checkBoxColumnIndex);
		HandleGridRowSelection(mouseActionOrSpaceKey: true);
		_isSpaceKeyPressOnCheckBox = false;
	}
	
	if ((e.Modifiers == Keys.Control && (e.KeyCode == Keys.A || e.KeyCode == Keys.Up || e.KeyCode == Keys.Down)))
		HandleCheckBoxHeaderClick(combinationKeys: true);
}

I thought I had broken the functionality because sometimes I saw it highlight all rows but it didn’t select the checkbox. Then when I was looking at the code again (which I hadn’t modified), I noticed it was called from Key Up. The ListData_KeyUp code would expect you to let go of A whilst still holding down Control. Although most people would do that due to their hand positioning, sometimes I was finding I was basically “stabbing” the keys so was releasing them roughly the same time. So in the times I had actually released Control first, then the IF statement doesn’t pass and therefore the checkboxes aren’t selected. I think the standard implementation is to check OnKeyDown and not UP.