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?
Stranger Coding Tings 