2017-11-06

PowerSMells: PowerShell Code Smells (Part 1.5)

2017110602

Intro

I am writing this next blog post in this series much sooner than I had anticipated. I wanted to do so in order to clarify a few things about PowerSMells and to address some of the negative feedback I have received. I will still include a PowerSMell, but this is mostly to address shortcomings in the Part 1 of the series.

Table of Contents for this series


PowerSMells are Not About Best Practices

One of the misunderstandings from my previous post was about the core nature of PowerSMells. It appears some thought my intention was to preach on high about what you should and should not do in PowerShell. That is certainly not the case, at least not in this series. You will know when I'm doing a series on best practices as I will use a bunch of "must", "must not", "thou shall", "thou shan't" in my verbiage. I will not shy away from such assertive phrases.

No, the point of PowerSMells is to help point out readily apparent pieces of code and what hidden problems you might find around them. That's not to say that any particular PowerSMell is "bad coding" or "the wrong way".

Take the PowerSMell002 plus-equals for example. It is not wrong to use plus-equals with collections. It is a perfectly legitimate use of the language. However, plus-equals tends to be used more often with standard arrays. The reason is that coders who are familiar with ArrayList or generic List<T> tend to use the Add() method instead of plus-equal and that the Add() method on a standard array will cause an exception. Using standard arrays is not a problem in some scripts, especially dealing with small collections. But, if you are dealing with large arrays and doing lots of array additions and removals, standard arrays will start to cause performance problems. Because plus-equals tends to be used with standard arrays more often, they are a code smell in that they clue you in to where a performance issue might exist.


PowerSMells are About Identifying Issues (Hopefully) Before They Occur

Code Smell is helpful for maintaining code. If you are not in a team environment or you don't help out in PowerShell forums often, you might not find this information very useful. The intent is not to help you write code, but to identify problem areas in existing code that you may not have written yourself.

Let's say your company has a scheduled script that does an information dump from Active Directory every day. You begin to notice the script is taking longer than a day to complete. You open up the script editor and you see a ton of plus-equals. They are easy to spot in a sea of code, but maybe not as easy to spot as the single array initialization. The plus-equals is not the problem, it's a clue to where the problem lies.You have a script that is taking longer and longer to run and you see a bunch f plus-equals. You think back to the source of PowerSMell002 and remember that standard arrays are a performance drag and that plus-equals is often used with standard arrays.

That's a good example of discovering the cause after the fact, but the same applies to code reviews or just looking at a script to see how it works. If you work in a team with code reviews, a plus-equals might be a good indicator to check that your co-worker is using a type that is better suited to expansion than arrays. You don't have to hunt down the coder and have them replace the plus-equals in their code. You just have to look at the collection type being used and maybe recommend an ArrayList or generic List<T> if a standard array is being used to prevent scaling issues before they happen.

Again, the PowerSMell002 plus-equals is not the problem, just a hint that there may be one. They are a tool to clue you into actual trouble code.


PowerSMells Don't Always Mean There is a Problem

There is not a 100% certainty that if you see a PowerSMell, there will a problem. A veteran coder may very well love to use semicolons. They may know full well that the semicolons aren't necessary and aside from their existence in the code, everything else may be impeccable. PowerSMell001 will kick in, but you will find the smell is just the neighbor making some amazing curry, and not hoarding a sack of rotting onions.

I'm certainly not saying that if you do any of these in your code you are doing something wrong.

Most of these PowerSMells I have developed a keen nose for from my very own code. I often have to go back and work on code I have written months and years ago. I have also sharpened my sense of smell against the grindstone of helping PowerShell users in various forums and chat channels. These have become a kind of shortcut for me to identify problems with existing code, be it my own or others. But, it's not assured. It has a very high chance of hinting at a problem, but can still end up being a red herring.


PowerSMell004: Commas in a Line Separated Array

Now back to some funky code. This is one of several PowerSMells that deal with line continuations.

The Smell

Here is an example of commas in a line separated array:

$Array = @(
    'Item1',
    'Item2',
    'Item3',
    'Item4'
)


The Cause

This PowerSMell hints at an intermediate level coder. Intermediate level coders are sometimes more dangerous than novice coders. I believe the phrase is "knows enough to be dangerous". The user knows about line continuation with an array, but has chosen to leave commas in even though they are unnecessary. Again, these commas are not a bad thing in and of themselves. But they do stick out like a sore thumb on the screen so they are easy to spot. Check the surrounding code for intermediate level mistakes and shenanigans, such as abusing expensive .NET APIs, manual scope manipulations, or excessive object creation. To use a gaming analogy: Look for all the cool spells you started to play with when you got out of the training level but abandoned after the second boss fight in favor of a more streamlined set of spells.

Also, don't look at my code. You will find this one everywhere, and yes, there are plenty of code landmines buried around them. I type this with one hand with my nose firmly pinched by the other. peeeeeee-eeeewwwwww.


Conclusion

I hope this clears things up. I honestly wasn't expecting any negative feedback. I will try to do a better job of being more clear in my intentions in the future. I'm sure that was Blog Smell. :)

Join the conversation on Reddit!

3 comments:

Paul Broadwith said...

Sad to see you got so much negative 'feedback' on these posts Mark. I don't agree with everything you have highlighted as a smell but I do get where you are coming from and appreciate the time you've taken to smell them out (see what I did there? :))

Christoph said...

How about this smell: using properties on an object that is a non-strongly typed variable and will later cause a problem. A good example is e.g. an array in `$obj = Get-Element`:
If the cmdlet does not return anything then `$obj[0]` will blow and if Get-Element returns just one element, then it will return the first character because PowerShell converts the object to the simplest possible type. My advice is to use `[System.Array]$obj = Get-Element` in thoses cases but maybe there is a better way of doing it?

Mark Kraus said...

Hi Christoph!

There are several ways to normalize output and ensure an indexable objects.

The most common is this:

$Widgets = @(Get-Widget)

Another option is this:

$null = Get-Widget -OutVariable 'Widgets'


In any case, numbered indexes are more "source" than a "smell". They are harder to spot. One smell that hints at them is a lack of looping when a collections are involveded.