The code may be suboptimal from a design or architecture viewpoint. I could also violate minor house style issues, or not match the style of the surrounding code.

Sometimes ugly code is the result of solving an ugly problem. This could result in smelly code that is actually correct for the situation.

This is why we have subroutines and modules.

Remember code that does not exist, has no bugs.

This is a symptom of a Single Responsibility Principle violation.

This does not refer to a structure, where a series of related pieces of data are in a single hash or other structure.

Subroutines with too many parameters are usually violating the Single Responsibility Principle.

This is usually a sign that the subroutine needs to move.

It makes the code much more difficult to deal with, due to Action at a Distance issues.

Several of the procedural CPAN modules do this. For example, Data::Dumper has a few package variables that control formatting.

One place where this can work well, is Dependency Injection. This should really only be used to facilitate testing.

Instead of getting a relatively good understanding at a glance, any reader of the code is going to need to parse the conditional, reason through the logic, try to determine the edge cases handled, and (in a way) simulate the conditional in their head to understand it.

If you can't come up with a good name, that suggests that the function is not as well defined or understood as it should be.

Any time the same term is used for different items or two different terms are used for the same item, a reader or maintainer of the code is likely to be confused.

When code does something, it knows it's doing it. It can communicate that to a function to-be-named-later, who will know with certainty exactly what it's supposed to be doing. A concrete example is a subroutine that uses globals or package variables to return extra data separate from the normal return.

In many cases, there should be constraints (and possibly behavior) associated with the actual idea that are not available with the primitive type. A value object would encapsulate the meaning in a usable name.

This can be caused by oddities in a language's design, but more often it's caused by fundamental misunderstandings about the language on the part of the programmer.

Code Junk is language specific. Some syntax is optional in one language but required in another.

I developed this list while working on coding standards for cPanel. I sought input from cPanel developers on the smells themselves as well as the names we wanted to use.

You will often see this overused by old C programmers.

This makes the construct harder to maintain, since anyone reading the code pretty much has to look at the whole loop carefully to determine if there is any funny business going on with $i.

As such, the maintainer can see, at a glance, that the list is being processed in one direction only, and while it is possible that items might be skipped (using next or last), we are not doing anything weirder with the list order.

The main difference between this construct and the C-style loop is that there is no possibility that the code could be changing the order in which you are processing the list, which is actually pretty easy with the C-style.

We can actually do quite a bit of the same kind of manipulation by modifying the index list to start with (grep for even indexes, reverse to do the list in backward order, etc.) Since all of those manipulations are in one place, the code is still easier to read.

Since these two styles are more idiomatic, the maintenance programmer is more likely to spend the extra time trying to figure out why the original programmer decided to use the C-style loop. This is wasted time and effort every time a programmer needs to read this code. Additionally, the extra Code Junk in the C-style for loop almost requires reading the entire loop to be certain you know exactly what the loop is doing.

The most common surprise I have seen is in subroutines that end with an if or unless or a C-style for loop. In all of these cases, the last expression executed is the conditional. Another case is having a print or close as the last statement, making the return value be something that we normally ignore.

Worse, minor changes in the code can result in unexpected changes in return values. For example, putting a print statement at the end of the subroutine changes it's return code.

From a quick look at the code, I'm not sure what the return is supposed to be. Is it supposed to be a number or a boolean? Is 0 supposed to be the same as undef.

Other than a programmer trying to be too clever, the most likely causes of this code smell are either the Long Method/Subroutine Code Smell or a violation of the Single Responsibility Principle applied to subroutines.

At first glance, this appears like a good idea. If we want to use the result of this routine in a for loop or other list context, we get the list directly. If we assign to a scalar, we get a reference.

1. Pass result as an argument to a subroutine.
2. Use result as a value when building a hash.

In both of these cases, we are actually in list context and the result is not usually what the caller expected.

Since it's a reference, it will always be true. From the fact we are in a conditional that's probably not the expected behavior.

This smell is a special case of Schizo Return.

Since the type of output changes drastically depending on the context where it's called, this code smell causes problems if the developer and the language have different ideas of the actual context in which the code is called. This can result in confusion and wasted time troubleshooting code that should be unambiguous. Worse, the failure modes are not always obvious at a glance. It would probably be better to avoid the special logic and just return one type.

That clever thing is almost always odd enough that programmers using the code will be regularly surprised as the code does something unexpected. That surprise is a direct cause of maintenance issues.

The real impact of this construct is in the confusion for maintenance programmers that are trying to use the code. Without a really good understanding of context and the decisions made inside the routine, this code is likely to be misused at least part of the time. Any construct that leads us to generate errors is definitely smelly.

There are very few ways to actually use wantarray in a way that is obvious to the maintenance programmer. One place that I have seen this done well is as an optimization. If I had a function that returned a sorted list, I could see using wantarray to avoid the sort, if we are not called in list context. This would avoid work that is just going to be thrown away in the case that we are calling the code (for instance) in a conditional.

In fact, it is a strong sign that someone is doing something bad in the grep block. More importantly, given greps only real purpose is filtering the list and we are throwing it's output away, the usage is almost certainly a bug.

At best, this would be an expensive no-op. At worst, we expected the output to go somewhere and the code no longer reflects that fact.

In fact, it is a strong sign that someone is doing something bad in the map block. If the block has side effects, we have another code smell (List-modifying map Expression). If the block has no side effects, then the map is an expensive no-op.

At best, this would be an expensive no-op. At worst, we expected the output to go somewhere and the code no longer reflects that fact.

This may not be obvious to a maintenance programmer and would violate the Principle of Least Astonishment. If you need to modify the original list and filter at the same time, a foreach loop would be clearer.

This may not be obvious to a maintenance programmer and would violate the Principle of Least Astonishment. If you need to modify the original list and create a new list at the same time, a foreach loop would be clearer. If you just want to modify the original list, you can use map to modify generate a new list and assign back tot he original array.

This means that using any one of these variables causes a decrease in performance in every regular expression in the entire program.

These variables have far-reaching performance impacts. Using even one anywhere in a program causes all regular expressions to be slower.

I think this cost may have been removed in recent versions of Perl.

Another advantage is that lexical handles are automatically closed when they go out of scope. Except in a very small number of cases, you should always be able to replace a bareword filehandle with a lexical filehandle.

If the expression begins needing multiple statements or multiple lines, this smell indicates that the expression may need to be refactored for ease of maintenance. If the code really needs to be that complex, converting the code to a named subroutine gives the advantages of being more readable in the map expression and more testable.

If the expression begins needing multiple statements or multiple lines, this smell indicates that the expression may need to be refactored for ease of maintenance. If the code really needs to be that complex, converting the code to a named subroutine gives the advantages of being more readable in the grep expression and more testable.

Unfortunately, there can be a tendency to make this expression very complex for some kinds of sorts. If the expression is complex, but not very expensive, replacing the in-line expression with a subroutine would be the cleanest solution.

Since the complexity of the sort operation is effectively O(n log n), it's possible to greatly slow down a sort by performing the same complex expression multiple times for a given element in the list. If the expression is expensive, even if it is not too complex, the Shwartzian Transform is an approach that can reduce the expense.

The problem with this code is that the hash construction and lookup are pure overhead. The purpose of using the hash is to avoid scanning the entire array to find your item. Since we walk the entire list anyway, we don't get any benefit from the lookup hash.

Some people might argue that this could be a premature optimization, but in actuality it is the opposite. The main problem with a premature optimization is that it makes the code less clear for no benefit. In this case, we are simplifying code to make it faster and clearer.

If there is an actual need to make this faster, if you can use List::Util::first or a foreach loop to exit early. That may yield a little more performance at the cost of code that is slightly less clear. So, actual performance should be measured before making this change.

This construct is actually quite a bit slower than the correct solution. A quick benchmark ranging from 5 to 78 items showed a factor 10 - 15 improvement, with the larger improvement being in the longer lists. By the way, that was 10 times to 15 times faster, not 10%-15% faster. This also highlights the difference between a premature optimization and a premature pessimization. Not using the One Shot Hash generates a factor of 10 improvement with an improvement in the readability of the code (a premature pessimization not to make the change). Adding code to exit early instead of using grep is approximately a 50% speedup, at the cost of slightly less readable code (a premature optimization unless the speedup in this case is worth the reduction in readability).

Using these package variables in other places in your code would cause them to change any time you use a sort. Additionally, if you declare $a or $b as lexical variables in a subroutine using a sort block, the block will be looking at the lexical variables, which mask the ones modified by sort. This will not end up doing what you want.

If a subroutine is called with the & sigil without any arguments, it is called with the exact same arguments as the calling subroutine. Since this is not obvious in any way from the call (and violates the Principle of Least Astonishment), it is confusing to read and maintain.

This was obviously a shell script, converted by someone who doesn't know Perl. This could be more easily done without a single system().

This worst version of this I've every seen had Perl code that built text files and shell scripts to process the input files, and then passed control to the shell scripts.

Most senior programmers have certain constructs that they avoid due to bad experiences in the past. Code smells are just a way of naming some of these practices.