Thursday, April 14, 2016

Your code smells? Let it!

Code smell is a term used pretty commonly in software development.  It is a way for one developer to express to another that they're experience is telling them something is wrong, even though they don't see it on the surface.  In fact, that's what it's supposed to mean.

But software developers don't do well with "trust your gut" as a methodology, so we make an effort to identify and quantify things like this.  For that reason, over time we have managed to put together lists of things in your code that are "code smells" and explain why we don't like them.

For illustration, I'm going to refer to personal experience and talk about an application I'm currently working on.  A common code-smell that developers will talk about is singletons.  So that will be the basis for my example, but the message should be clear even without the example.

Why does it smell?


The first thing you should ask the developer on your team who's most knowledgeable about the code in question (that may be yourself) when you encounter a code smell is "Why does it smell?".  This is the part I see skipped--A LOT.  This is the most important part of the process.  Let's face it.  There are legitimate reasons for things to smell.  Your mechanic's shop should not smell the same as a bank vault!  Each code base is going to have its own set of smells, and that's ok, as long as the answer to "Why does it smell?" is reasonable.

In a project I've stepped into, singletons are everywhere.  I asked one of the developers about them and his answer was one sentence.  "We have memory constraints."  Oh!  That makes sense.  This application lives on (crappy) webservice connections, and is written in an older language, so it's logical to store those connection objects for reuse later, rather than waiting on a crappy garbage collector to delete them and running out of memory.  This code base smells like they're using singletons as global objects.. Guess what?  They are!  And it's OK.  That was their intent!

Of course, the answer is not always valid.  If the answer is "That's the way it's done" or something else vague like that, it's time to investigate yourself.  It may be that there is a legitimate reason and you haven't encountered it yet, but this is a code smell, and so you need to find the source.  You cannot, and should not go changing code to "fix the smell" if you don't know why it smells.

In my example, if I had started getting rid of singletons to "fix it" and making them instances of the service all throughout the application, I would have surely crashed production!  Don't go fixing things that you don't KNOW are broken.  Code smells are just that: a smell.  Some smells are supposed to happen!

What if it's bad?


So you've identified a code smell.  You asked "Why does it smell?" and you figured out, through some form of investigation, that it's bad.  This is the smell of burning rubber over a box of wires.  It's a bad smell, we need to fix it.

You're not supposed to fix the smell!

So many developers get this bit wrong.  The smell is your marker.  It identifies that there's a problem in your code base.  The smell itself is NOT the problem!  99% of code smells are pointers to an architectural problem!  The smell "goes away" when you fix the root problem.  Saying "x is a code smell" is not the same as saying "x is bad".  The code you're looking at solved a problem.  It is good code.  It just may not need to exist if you fix the part of the code that is bad.  The part that caused the smell to begin with.

As an example, in my current day job, they identified the singletons but knew that "singletons are a code smell".  They made the standard mistake.  They think that the word code smell means "bad".  So they say "singletons are bad".   Then they try to remove the singleton.  This actually happened in this code base.  They did something I'm seeing a lot on blogs.  They wrapped their singletons in a Dependency Injection library and pretended that, since the instance is being passed to their class via dependency injection, it's no longer a singleton.

Don't hide the smell!


Ok, so the new guy is just starting.  He didn't do any of the above, but he saw the singleton pattern and got all excited to help and swapped it out for dependency injection...  What did he do?  He masked the smell.  The smell is there for a reason.  It's like covering up the dog turd in the middle of the floor by putting a towel over it.  Whether this is a kennel, where that dog turd needs to be kept in the dog-turd specific area (maybe a singleton resolver class or some such), or a perfume store, where it needs to be cleared out immediately at all costs, he has made it harder to fix, and hard to identify for everyone.

In a code-base where the smell identifies an actual problem, to fix the smell masks the real problem and keeps you from finding it longer.

In a code-base where the smell points to outside forces beyond our control, the fix makes it harder to maintain, and harder for new developers to step in and learn about the code base.

Never, ever hide the smell.

What about your singleton problem?


With a big problem in an existing code-base like this, I'm starting small.  First thing to do, unmask all the singletons.  I'm creating a group of static classes that wrap up the singletons in the application.  Some day in the future, I'll be able to remove the Dependency Injection framework entirely and start over, using it correctly (for actual dependency injection).  But until then, I've had to think differently.  For now, in this particular code-base, whenever I see that dependency injection is being used.  That, for me, is a code smell.

Friday, April 8, 2016

Dependency Injection - You're doing it wrong!

So I have worked at a lot of places and seen a lot of different styles of programming.  Early on in my career, I became acquainted with the concept of dependency injection.  It was a hard topic to grasp, but I have learned understand it deeply.  Now, when I step into a new application that uses it, I can very quickly see the flaws in the implementation, and there's a common one I want to talk about today: global singletons.  But we'll get to that in a minute.

What is Dependency Injection?


Dependency Injection is exactly what it sounds like.  You use it to inject your dependencies.   The unique part about Dependency Injection though, is that you can do this at runtime.  Now this sounds fancier than it is.  By inject we don't mean they're downloaded for you.  You still have to have all of the parts installed where you want to run your app.

Dependency Injection is somewhat of a complicated topic to a newbie.  Let's start with defining the word dependency here.  Specifically, DI is about classes, but everyone who talks about DI talks about Interfaces.  This is because DI does something unique among software patterns.  It doesn't decrease the amount of code you have to write, it increases it.  That's right, when you start using Dependency Injection, you can expect to write a lot more code, a third as much as you are currently writing or more (beyond 100%) on top of your existing workload.  For every class you write, you need to write a matching interface that provides access to all of the class's public properties, and the class needs to implement that interface.

Then Why Use It?


Like all software patterns, DI is about making your code easier to work with.  The magic of DI is the interface.  Interfaces are simple stubs of classes that essentially mean "any class that implements this interface must have these public properties".  That's it.  It's a set of rules for creating a class.  This makes it easier to write your class later, because you know it only needs public properties X, Y, and Z.  Let's look at a real life example.  I'm pulling this example from a library written by a good friend of mine who's worked with me at some of my big name jobs.  Here's the full library for reference: https://github.com/danielkrainas/squire

First, let's look at an interface he created for storing data.


namespace Squire.Storage
{
    using System;
    using System.Collections.Generic;
    using System.IO;
    using System.Linq;
    using System.Text;
    using System.Threading.Tasks;

    public interface IBlob : IBlobItem
    {
        void SetPermissions(BlobPermissions permissions);

        void Delete();

        void PerformRead(Action<Stream> readerActivity);

        void PerformWrite(Action<Stream> writerActivity);

        void CopyTo(IBlobContainer container, string copyName = "");
    }
}

Pretty simple right?  Basically his interface offers CRUD.  Create, Read, Update, Delete, and a few other minor features.  OK, it's not perfect CRUD, but I'm sure he intended for it to be that way.

If you've written code for storage, you can imagine how having this template makes it a little easier to write the class that implements IBlob, but the value of this interface is the ability to use it for Dependency Injection.  Let's take a look at another Interface (because they're short) that uses the IBlob interface as if it were an object.


namespace Squire.Storage
{
    using System;
    using System.Collections.Generic;
    using System.Linq;
    using System.Text;
    using System.Threading.Tasks;

    public interface IBlobContainer : IBlobItem
    {
        IBlob GetBlob(string name);

        void CreateIfNotExists();

        void SetPermissions(BlobContainerPermissions permissions);

        IBlobContainer GetContainer(string containerName);

        void Delete();

        IEnumerable<IBlob> SearchBlobs(string filter, bool recursive = false);

        IEnumerable<IBlobContainer> SearchContainers(string filter, bool recursive = false);

        IEnumerable<IBlobItem> Search(string filter, bool recursive = false);

        IEnumerable<IBlobItem> Contents
        {
            get;
        }
    }
}

So we can create objects and refer to the types by the interface instead of by the class.  But ultimately, an interface is not a class.  You cannot create an instance of an interface.  You can only create an instance of a class.  What we get is a state where you can rely on any class that implements your interface to have those same public properties.  They call this a Contract.

Wait, this is about inheritance?


No.  This is where we talk about the second thing people talk about with Dependency Injection: Inversion of Control.  The benefit of having these interfaces is that you no longer have to worry about HOW something will be implemented.  You can pass off individual classes even between developers and they no longer have to know what each other are doing.  They have an interface that binds them.  That interface tells you in a very concise way what the architecture, the structure of your application is.  The individual class implementations can be good or bad, but you don't have to know how one class works in order to work with another.  Instead, you only need to know what public properties that class will provide you, and you can build your logic around it.

Inversion of Control is where you decide which class to inject at runtime.  You get control of your implementation at runtime, instead of at compile time.  Your control is "inverted" from the traditional implementation..  If you think about a traditional implementation of the above interfaces, BlobContainer would have an instance of Blob.  Which means that if you change Blob, you can easily break BlobContainer.  But if BlobContainer relies on an IBlob interface, then you can change Blob all day.  As long as it implements IBlob, BlobContainer will be unaffected by those changes.  BlobContainer now has control over the contract it chooses to support with objects that implement IBlob, rather than the other way around.

This is what Dependency Injection gives us.  It creates a clear application structure that we can rely on.  But it has a bonus feature.  Interfaces can be implemented by multiple classes.  Since we can swap out these classes at runtime, which means: we can swap out our implementation.  For this reason, if you're working in a statically typed language, you should always use this Interface pattern.  In fact, that pattern has been around a while.  It's called the Revealing Interface Pattern, and has varying names and slightly modified versions in many languages, even those that are not statically typed.

So what's wrong?


Because you can change your implementation at runtime, you can do some crazy things.  The craziest I have seen is that some people use their DI library to create "global singletons" that they can access throughout their application.  Globals are bad.  Singletons are bad because people treat them like globals.  Wrapping singletons in a Dependency Injection library adds extra weight.  To add that extra weight in order to do something that's bad in the first place is so dumb.

Use Interfaces to make your code easier to follow, and IoC in order to make it easier to change out dependencies (read: classes).  Don't use DI to make your dependencies global.  Dependency Injection: You're doing it wrong.