Responses

You have 2 free member-only stories left this month.

Everyone writes perfect code, right. We all take our time without worry of deadlines and external pressures to create elegant code so well constructed it should be considered art.

Right…

Well the real world is hardly as forgiving. Everyone takes shortcuts and has to choose between the okay, the good, and just get it done. We make sacrifices and bad practices seep in. Some of these shortcuts have a devious way of repeating themselves. We call these anti-patterns or code smell.

These pieces of code make us cringe and feel sick to our stomach. We curse the screen and swear to get revenge against the idiot that commit it.

Who did this garbage:

git blame pom.xml...
efenglu

… oh wait. Let’s just ignore that for now. I’m sure the developer was just having a hard day.

We’ve all been there. Just like any bad habit we need to first identify the habit and correct the behavior.

This is a list of bad programming practices I’ve seen (and done) and hope to never repeat. Hopefully, by identifying the behavior we can address it early on and avoid costly refactoring later.

Spaghetti Code

Unlike a big plate of spaghetti, spaghetti code is neither delicious nor at all appealing.

Example:

A references B which references C and D which sometimes references A or E but mostly A' which provides … Did you get all that? Now add a new feature that uses G but not I.

Spaghetti Code is when through all matter of indirection, abstraction and best intentions we end up with code that tangles and weaves back and forth within itself. Boundaries between different sections of code are blurred to the point of being meaningless. Pieces of code seemingly disjoint are eventually found out to be joined at the hip.

Spaghetti code really rears its ugly face when you try to extract a capability. Just like sucking a spaghetti noodle up it can seem like you’ve succeeded in your task until it slaps you in the face and spills sauce all over your shirt.

To avoid spaghetti code you should identify boundaries and follow the principal of single purpose as much as possible. Keep code size small and dependencies limited.

And use a bib.

Clone and Own

Sometimes a piece of software behaves just the way we want … except we need to modify it slightly. Unfortunately, the source code can’t accept our use case easily and we can’t or don’t want to refactor it. We therefore simply copy the implementation and modify it to suite our needs.

Clone and own is the antithesis of the DRY (Don’t Repeat Yourself) coding principle.

Sometimes the clone and own pattern is actually the “best” approach. But we should use it sparingly. Clone and own of large pieces of code can lead to a maintenance nightmare when bugs are discovered.

When using clone and own only we should only clone very small reusable pieces of code. These should generally be patterns or strategies that don’t make sense to refactor into libraries. It’s also good practice to mention the source of the clone.

At least then you’ll have someone else to blame when you discover a bug.

Hands in the Pants

Would you ever stick your hands into someone else's pants? No. Then don’t reach into their code.

Hands in the Pants is where through all matters of private and even sometimes public API’s we end up touching and reaching into code we really shouldn’t. This breaks the boundary of concerns and exposes knowledge of the internal workings of the code that you really shouldn’t know.

Often times developers will not know that you were using a specific capability and they will refactor it thinking it is completely safe. Other times by reaching into code you may break assumptions made about the internals of an object.

In general, we should obey API boundaries and don’t typecast interfaces into implementation classes. Implementation classes are internal and shouldn’t be referenced. Even better, hide implementations in separate maven dependencies and use dependency injection or, better yet, something like java modules to ensure no one can reference them.

Reflection should be avoided at all costs. Its slow and will almost certainly break in the future. Just because you can do something doesn’t mean you should.

Try refactoring your code to use publicly available API. Setup knowledge boundaries. Eat your own dog food and force yourself to not know implementation details when using your API. If it feels weird or awkward, refactor. You don’t want someone reaching in your pants, so don’t reach into their’s!

House of Cards

It’s elegant, almost beautiful. But not because its well written but because you can’t believe it actually works! The house of cards is often a symptom of other anti-patterns. You will usually discover a house of cards when told to refactor someone else's code.

To avoid house of cards:

• Write concise code
• Write tests of your API and interfaces
• Try to break your code

If given a house of cards it will be necessary to setup a solid foundation before continuing. Focus are writing lots of unit tests of the existing application first. This will familiarize yourself with the existing behavior and ensure you don’t break anything. Begin refactoring in small chunks. First coding to and keeping existing behavior. Slowly you can start to reinforce the API and introduce better patterns.

The One Liner

For some reason some developers feel that there are only a certain number of newlines in the world and they must be rationed as a precious resource. They often delight in their ability to collapse a larger statement into a single line of code. These lines are often very complex, long, and hard to read.

Ever heard of the DeCSS 7 line Perl script?

When coding remember to think about those coming after you. Your code should be easy to read and have a flow to it. Each line should accomplish a single unit of work.

The horizontal scroll bar is NOT your friend!

The ternary operator is a common culprit of one liners.

Do you prefer this:

5 > b ? 3<b ? print "4" : print "2" : print "6" 

Or this:

if ( 5 > b) {
  if ( 3 < b) {
    print "4"
  } else {
    print "2"
  }
} else {
  print "6"
}

In general, don’t use the ternary operator. For simple print statements it may be useful, but NEVER nest them.

Split long chained method calls on to multiple lines. For instance, when using streams do this:

list.stream()
   .filter(...)
   .map(...)
   .filter(...)
   .collect(...)

Same goes for using builders:

MyBuilder.newBuilder()
   .setValue1(...)
   .setValue2(...)
   .build()

The Academic

The academic programmer prides themselves in using code features that others have never heard of. They are gleeful when their code uses some obscure unheard of capability.

A common example is bit manipulation. There is rarely needed in most regular code. So don’t do it.

Ever heard of the XOR swap algorithm? This may have been a weird interview question but for god’s sake don’t ever do it. Memory and registers are not that precious a resource. Just use a third variable.

Instead write code as if someone new to the language needs to read it. Sure it might be longer and more verbose but it is also easier to read. In most cases you really aren’t as “smart” as you think you are.

Every Screw is a Nail

Tools are only good when used for the right job. Just because you learned about this nifty new language or new framework doesn’t mean you get to use it.

When coding slow down and ask yourself.

Am I forcing myself to use this, or is there a simpler way that isn’t as complicated

Specials AST transforms are a common culprit here.

Stick to built in language constructs or features your team has agreed on.

Premature Optimization

This common anti-pattern is very easy to fall into. You see a piece of code and feel the need to write a slightly more complicated version that would be faster. While your intentions are true, you are also taking up valuable time, making the code more complicated and, more than likely, introducing more bugs.

Now we shouldn’t intentionally write slow code.

We should first focus on legible, maintainable code. If performance becomes an issue then, and only then, do a performance analysis and refactor where necessary. More than often it’s not where you would have expected.

Be careful about overuse of caches, or copying large amounts of data into memory. This can seem like a quick win but may not really be necessary.

Besides, the compiler can often do a pretty good job optimizing code for you.

Globals

This includes all sorts of static variables, methods, and import statics.

In the world of containers and services this concept expands to environment variables, urls, system properties, and executables included in your containers.

Globals are rife with pitfalls. They make code fragile as changing one thing has far reaching consequences. It’s difficult to identify where they are used. Globals make testing more difficult as it is nearly impossible to insert mocks for globals. They are also a potential source of security problems.

In general avoid them!

Avoid static methods and public static fields. Use dependency injection wherever possible.

Avoid environment variables and hard coded url’s. Use service discovery mechanisms like Eureka and external configuration like Spring Boot Config.

For utility classes consider making them an injectable bean of an interface. Especially, if their behavior is complex. If you make a utility class keep it small.

Happy Path Driven Development (HPDD)

I’m guilty of this one. I think everyone has done it at some point. You write a piece of code until it works for your ONE case. You may even write a test for it.

And then you’re done.

YOU’RE NOT DONE!

In fact this can be the fun time. It’s time to be evil :)

Try sending it bad stuff, invalid parameters, null, uninitialized state. Attack your assumptions, be ruthless.

When all the dust has settled and your code reins triumphant, now you are done.

This will never…

Like the happy path this is the symptom of a lazy developer.

“This will never be null”

“This will never throw an exception”

Six months later…

“hey our code is broken, who started throwing this exception?”

Code defensively.

Never catch blanket exceptions and blindly throw them away.

Don’t make assumptions about behavior. Behavior can change, often without notice.

If it can be null, assume it will be.

If you find yourself saying “It will never…” know that it almost certainly will. Do yourself a favor, code for it now. Even if its just throw IllegalStateException.

Just Use Maven LATEST version

When specifying your dependencies in maven it can be cumbersome to always set a specific version for each dependency. It can be tempting to just use LATEST as the version.

<dependency>
    <groupId>org.apache.pulsar</groupId>
    <artifactId>pulsar-client</artifactId>
    <version>LATEST</version>
</dependency>

Don’t!

Using LATEST makes repeatable builds nearly impossible since your never know what you built against before and what you will build against now.

Always use a specific version. At a bare minimum a version range but even this can pose problems for developers.

<dependency>
    <groupId>org.apache.pulsar</groupId>
    <artifactId>pulsar-client</artifactId>
    <version>[2.3.0,3.0.0)</version>
</dependency>

When using version ranges maven will only resolve a dependency if it doesn’t have one locally which doesn’t satisfy the dependency. Therefore, if you introduce something new some of your developers will begin complaining about their builds failing locally. This is because they are using a previous version. If you use ranges, always update the minimum version if you are expecting a new feature. But if you are going to that much trouble you might as well just update the version and forget version ranges.

<dependency>
    <groupId>org.apache.pulsar</groupId>
    <artifactId>pulsar-client</artifactId>
    <version>2.3.0</version>
</dependency>

Also, don’t ever use, SNAPSHOT dependencies of modules outside your project. These are a moving target. In fact, I’d recommend you use Maven Enforcer to make sure your libraries use only concrete versions.

<plugin>
    <groupId>org.apache.maven.plugins</groupId>
    <artifactId>maven-enforcer-plugin</artifactId>
    <version>3.0.0-M2</version>
    <executions>
        <execution>
            <id>default-cli</id>
            <goals>
                <goal>enforce</goal>
            </goals>

<configuration>
                <rules>

<requireReleaseDeps>
                        <message>No Snapshots Allowed in releases!</message>
                        <onlyWhenRelease>${enforcer.requireReleaseDeps.onlyWhenRelease}</onlyWhenRelease>
                    </requireReleaseDeps>
                </rules>
            </configuration>
        </execution>
    </executions>
</plugin>

Who needs code style?

EVERYONE!

We can have endless debates on what the best codestyle is but the simple answer is the best codestyle is the one your team can agree on. Figure that out and enforce it.

Use tools like Checkstyle and Sonar. Don’t accept new code until the code falls in line.

Something as simple as code that looks the same no matter who wrote it can have a profound effect on legibility and maintainability of your code.

Characters are Expensive

Have you ever run into code where the developer uses tons of abbreviations or short nondescript variable names. Code needs to be read by people. Don’t worry about using slightly longer variable names. If it makes the code more legible, do it.

Don’t:

int i
int compObjVerT
long delVarId

Avoid terse single character variables.

for (int i;...)
int compareObjVersion
long deleteVariableId

It is acceptable to use “i” or “it” in for loops or lambda expressions. But be careful about nested loops. If the distance between where the variable is defined and where it is used grows beyond a few lines then the variable should have a descriptive name.

Git: Bigger Fewer Commits

I’m totally guilty of this, and I’ve been burned by it more times than I can remember.

One of the best features of git is that it is a decentralized repository. Meaning, your commits don’t actually enter the mainline until you want them to.

Therefore, commit early and commit often.

Use git as a way to track your progress on an idea. It doesn’t matter if you are not done. Go ahead commit.

This allows you to revert back to previous states easily.

Before you try something new, commit.

Before you refactor, commit.

Heck, before you leave for the day, commit.

Don’t worry about your commit history. Once you are actually done you can easily merge squash that garbage down into a nice professional commit message.

Git: Meaningless Commit Messages

Speaking of commit messages have you ever seen this:

Fixing stuff

or this

Updating specs

or this?!?

None of your business

Well, the last one is a bit extreme but you get my point. Your commit message is your way of telling other developer what you are doing. Sure, we can read your code but the message gives us context. The commit message should answer the “why?” and sometimes the “how?” you did something.

• Why where you fixing stuff?
• Why/how was it broken?
• Why where you updating the specs?
• How does this effect our clients?

You don’t need to write an essay. That’s the job of your issue tracker. The commit should be:

• Sshort but not too short
• Descriptive without being repetitive
• If you have a related issue, mention it.

Also follow good commit template practices:.

short one-line messagefollowed by blank line and
long multi-line
descriptive message

Following this pattern makes it easy to scan the git log and even create short release notes:

git log --pretty=oneline

Git: Long Lived Branches

Have you ever had a feature branch that stretched on for weeks or months at a time. Waiting until its perfect and does everything only to realize at the end that because you waited for so long and your distance from master is so far that your branch is now too risky to merge.

+1 I have

Follow the agile manifesto. Break features into small pieces. And merge it. Then the next piece, merge it.

So what if it doesn’t do everything. As long as it works and doesn’t break existing stuff that’s fine. It’s actually better than fine.

You are delivery new capabilities. This is great. Often times this is when you discover that you need to rethink a design decision and perhaps throw away code.

That’s ok. You learned something and you addressed it early instead of late in the development cycle.

If you are working on a branch for more than a couple of weeks, you are probably working on it too long.

Only Using an IDE

We all have our favorite IDE.

• IntelliJ
• Eclipse
• Netbeans…

But don’t let the IDE become a crutch. Get familiar with the best IDE everyone has… the terminal.

This magical black box isn’t that scary. It’s incredibly powerful, always does exactly as its told and is easy to repeat actions. Oh and did I mention its stupid fast. And FREEE!

What I’m trying to say is don’t limit yourself to your favorite graphical IDE.

You should know how to use git on the command line.

git clone https://github.com/torvalds/linux.git
git checkout myNewKernelMod
...
git add .
git commit -m "I am amazing!"

You should know to edit a file using vi.

vi someTime
// i Type some code
// :wq

You should know how to use grep and find.

grep -R "strange Stuff" *find . -name magicFile -exec rm '{}' ';'

These simple tools will make you a very powerful force on any computer. Especially servers that don’t have UI’s to speak of.

You don’t need to be a bash guru. Just get comfortable with it, and RTFM.

If you need help ask a ‘man’. (Ok, bad joke but seriously the man pages are really useful.)

man find

Or just ask the command itself:

grep --help

One of the best things about the terminal is I can easily share a command to another developer and know that it will works:

git reset HEAD~1
git clean -f
git commit -m "Commit the changes"

Try explaining that using a graphical IDE interface.

Modifying Input Parameters

Have you ever passed an object to a method and the method modified the object?

public void trustMe(java.util.Date importantDate); 

Does this method modify the date? Certainly doesn’t seem like it should. In fact you’d probably trust that it doesn’t. But it could.

I’ve seen code that does! Scary shit.

Instead you should:

public java.util.Date safeTrustMe(java.util.Date importantDate);

Now its obvious that a new date is being created. Although, they could still modify the date I passed in but at least I kind of feel safe that they won’t.

To truly protect against this your should only pass around immutable objects.

public LocalDate safeTrustMe(LocalDate importantDate);

Returning null

Null (IMHO) is broken in almost all languages. Although Java is a strongly typed language with every reference there is a key flaw. Every reference “could” have a value. There is no guarantee that it will. This means in general we have to basically not trust anyone. But this means our code becomes littered with null checks. So we take short-cuts and assume most things aren’t null.

This leads to most developers assuming a method doesn’t accept null parameters and methods never return null parameters, ergo the code smell of returning null.

But what about when we don’t have a value.

In earlier version of Java there was no real option. You had to return null. Obviously you could document this but documentation is never really read by most developers.

Ever used this:

new File("somePath").list()

Did you know it can return null?

Thankfully, Java 8 introduced the Optional. For new code there is no reason you shouldn’t use Optional. It is clear that the value returned may not exist. With Optional we can return a non-null reference that describes a null (or missing) value.

Yes, you could use @Nullable or @Nonnull but again these are like documentation. They are not enforced by the compiler or the language.

And for god’s sake NEVER return null Collections!

List<String> happyString() {
   return null;
}

WHY!?!?!? It’s a list, just return an empty one!

List<String> happyString() {
    return Collections.empyList();
}

Catch Throwable

try {
...
} catch (Throwable e) {
  throw new IllegalStateException("Bad stuff happened", e);
}

Do you really think you know what do to if ANYTHING goes wrong?

Throwable is very broad. It includes things like Heap errors, JVM errors, lots of stuff that you have no chance in handling correctly.

What do you do if you run out of heap space?

I’ll tell you…NOTHING. You are SOL, done, die, finished! So don’t act like you can recover from it by catching Throwable or Error or the like.

Don’t do it!

Throwing Without Cause

You’ve caught an exception and you are going to throw a new exception with a more descriptive error message. Awesome, that’s great!

try {
...
} catch (IOException e) {
  throw new IllegalStateException("failed to write");
}

But …wait…what happened. Why did it fail? You had the information right there. Pass it along:

try {
...
} catch (IOException e) {
  throw new IllegalStateException("failed to write", e);
}

Was that so hard?

If you are throw exceptions from within a catch block, please include the cause exception in your new exception.

Logging without cause

This is similar to not re-throwing exceptions.

try {
...
} catch (IOException e) {
  log.warn("failed to write");
}

Again, why? Why did it fail to write? Put the exception in the log.

try {
...
} catch (IOException e) {
  log.warn("failed to write", e);
}

It’s not that hard, add it to the log. You can always filter it out latter in your log config file. But if you don’t even try to log it you’ll never have it.

In general, if you log in a catch block make sure you log the source exception. And if you catch an exception silently, you should at least add a trace log.

try {
...
} catch (IOException e) {
  log.trace("Nothing to see here", e);
}

This way when for some reason it becomes necessary to debug these, “it will never happen” or “it’s not important” blocks of code, these trace logs will become very useful.

System.out Logging

Speaking of logging, System.out is NOT a log. In fact, unless you are writing some sort of console application you should NEVER use System.out.

On my team we go as far as using checkstyle to enforce that System.out, System.err etc are not used anywhere in our codebase.

Always use a logging framework. There are lots to choose from.

System.exit

Are you writing a console app? No? Don’t use it. Use checkstyle to enforce no one uses it.

System.exit will do just that. Exit. Immediately.

This is never what you want.

Instead throw exceptions. Even a type of runtime exception like IllegalStateException.

If you are writing a console app I would still question the need for System.exit. If you throw an exception from main it will propagate up into the JVM and cause an exit code of -1.

Only use System.exit if you must closely control the exit codes returned from your program.

Reinventing the Wheel

Would you ever code your own version of java.util.List?

Or your own SQL implementation?

Probably not.

Before you do almost anything do a quick google. See if someone else has done it before. You don’t need to use their implementation but you could at least learn from it.

Floating Point Numbers for Currency

Currency needs fractional numbers.

Floating point numbers provide fractional numbers.

They are not the same.

A floating point number is a fuzzy number. Meaning it’s not an exact value. It therefore can’t do exact math.

Would you accept it if you bank said, “well we’re off by a few dollars in your bank account.” No!

Then don’t do it in your app.

Use libraries like Joda Money. Or if you must BigDecimal.

It’s not only a good idea, it’s the law.

Time… You’re Doing It Wrong

To us humans time seems like a pretty constant obvious thing. But as Einstein has shown it is anything but.

Without even getting into Quantum theory and time dilation, time in computer programming is very, very, hard.

Lets consider something as simple as 2:00 PM on December 12, 2015. Seems pretty precise.

Wrong!

This time could represent any number of actual instances in real time due to things like timezones.

For an in depth look into time I highly recommend this article, Time and Timezone: Getting it Right

In general, educate yourself on the complexities of time. Fully understand how you are using time and date in your context. Make no assumptions. There is no one right answer.

Inheritance

Have you ever seen an inheritance tree that is 5 or more classes deep?

Inheritance is great and horrible at the same time. It allows an “is a” relationship. And the ability to inherit behavior. This can reduce code duplication.

But we can often overdo this.

Inheritance introduces all sorts of weird problems with overriding behavior, default behavior, typing.

In general composition should be preferred over inheritance. Don’t use inheritance as a way to avoid code duplication. It should only be in true is-a relationships. Even then, interfaces may be better suited.

Uber Functions

It slices, it dices, it breaks and is impossible to test.

We’ve all written functions that have gotten big. They have large number of nested loops and branches. Handle all kinds of different configurations and conditions.

But do we test them all? Did you write unit tests to actually test every permutation of every branch.

There is a measurement that is often used to judge the complexity of a function, Cyclomatic complexity. This metric allows you to assign a numeric value to your functions complexity. Too high of a value is indicative of something that is too hard to test and should be refactored.

Now this isn’t a perfect measurement but it’s a start.

So how do we write “good” code.

As with most things in life, what qualifies as good is entirely subjective. There is no perfect demarkation line between bad and good. Even some of the “anti-patterns” mentioned here could have cases where they may be prudent.

But just as our tastes evolve and we learn to become better judges of food, and behavior and leadership. With experience and sharing of knowledge we can become better judges of good practices in coding. We learn to identify bad patterns and the reasonings behind them. This allows us to make better choices in the future and avoid pitfalls.

I’d also recommend using tooling to enforce “good” programming.

There are lots of choices:

Learn about these tools and integrate them into your build process. Many of the problems I’ve listed here today and more can be blocked from ever entering your codebase by using them.

What are some of your “favorite” anti-patterns or code smells?