Friday, January 17, 2014

A log fought battle

While I'd be the first one to say static methods are the root of all evil, I wouldn't be far from the truth if I did.

In case you aren't aware already, static methods reduce flexibility, testability, and take the Object out of your Object-Oriented code.

It's a wonder that so many people don't see this, when it's in the name - and yes, it deserves it. Forget for a moment that statics are simply namespaces and global state; a static method or a static value is the opposite of a runtime value or a runtime method. I don't mean its more type safe, I mean its cement. It's the anti-if-statement. It's stealing the code from another class and pretending that doesn't violate the single responsibility principle.

Using something static means...force exactly my desired behavior on others that use my code, as if that weren't the opposite of flexibility.

If you don't know what I mean, take this example:

class PasswordVerifier {
    public boolean verify(String password, Byte[] hash) {
        return Crypto.md5(password).equals(hash);
    }
}

Now say a few months pass. I no longer want to use md5, since sha256 is much safer, and I found out that I should use a unique salt for each user to prevent rainbow table attacks. And, hashes being irreversible, I still need to support the old database that used plain md5. The old implementation of PasswordVerifier is just plain wrong.

Isn't that normal, though? Changing requirements lead to changing code. Of course, now this function must accept a salt, and maybe a User, to detect if we use md5 or sha256.

But a changing method signature means changing all other code using it. It means I can't merge the change into the version one branch after solving the bug in master.

On the other hand,

class PasswordVerifier {
    public boolean verify(String password, Byte[] hash, Hasher hasher) {
        return hasher.hash(password).equals(hash);
    }
}

Now I can change how the code operates at different times when it's used, not just you changing how it operates at the different times that it's written.

In my bug fix on master, I just do

boolean valid = verifier.verify(password, hash, new Hasher() {
    @Override
    public Byte[] hash(String input){
        return user.getIsLegacyPassword()
            ? super(input)
            : Crypto.sha256(user.getSalt() + input);
    }
});

And here you will see the other problem with static methods: so far this post has focused on why not to use them, yet I just did with Crypto.sha256(). This is actually intentional, and demonstrates why not to write static methods. It's simple; if you write them you will force others to use them.

I have fought a long battle over the (in)convenience and (in)flexibility of static methods, and in that time I've learned that my argument is more likely to be heard if I "concede" on at least one point. I can use that point to explain why a line is drawn and where I draw it. After all, it's a hard sell to say static methods are always bad.

The example I freely gave of a "good" static method is a logger. After all, loggers are never the condition of a test, never complicate writing tests, never change APIs, and never need to be done in multiple ways, right? And in the rare case they do, it's easy to refactor and definitely won't get merged into the v1 branch.

Every time I said loggers should be static I cringed just a bit, because I knew, the static implementation is still flawed and unnecessary. And still I forced myself to believe it, to be even slightly moderate.

Now please, if you will, import the static logger android.util.Log;

A week ago the logcat app on my android test workstation started acting strangely. It would crash unexpectedly, clear when I didn't want it to, and it decided not to print exception traces ever again. Here I am running tests on an android library, and reading the debug output requires unplugging my android motherboard from my monitor, plugging it back into my development workstation, opening a terminal, and running the adb logcat command.

I eventually rewrote every Log.v(Tag, String) call to print on the interface, a change that could have been as easy as subclassing the app's logger instance. Of course I cannot merge these changes into master as it is for test purposes only. And I thought nothing of it.

It's strange how quickly afterwards I lost another day over this static logger.

Today I began unit tests on that framework. To keep the tests fast enough that they'll run on every build, I took the code with no relationship to the android framework. Here nobody needs a slow android emulator, I wrote a test that could run anywhere that has installed java. 

    1 test failed, 0 tests passed, 1 test total.
    MyTestClass.myTestMethod: Stub!

Because the component I am testing is written for android, even the code that is pure business logic and has nothing to do with android, will use android.util.Log. But android apps don't compile against the real code. Google instead gives you a special .jar file in which every single method call throws an exception, including Log.v(), until you run it in a real device.

In the end I found that I could include Robolectric to generate bytecode that overwrites the "Stub!" implementation of android.util.Log, but not without bringing in a huge compilation dependency, tearing my hair out, and craving a systemwide refactoring.

It would've been a hell of a lot easier if these classes accepted a non-static Logger instance as a dependency, and I could pass in my own NoopLogger or InterfaceLogger, and go home on time.

It's a log fought battle, but I will keep fighting it, and I hope you do the same.

No comments:

Post a Comment