loader gif

KISS: Keep it simple and selfish!

October 31, 2016

Here I’m going to talk about the KISS principle. For those who saw the title and came to correct me about what KISS stands for, thank you, but the title has its purpose. I obviously know KISS stands for “Keep it simple, stupid!” (or at least this being the most common) as you can see on the amazing frame my girlfriend made me:

img_20160903_173833

I’ve been having the feeling that sometimes simple, or what is understood as simple by some people, tends to make code very coupled and ends in spaghetti code. That type of code where you find yourself dealing with some variables at a specific level of abstraction that don’t belong to them, probably mixing higher levels with lower levels in the intermediate level. Because sometimes, when writing code, that looks like the simple thing to do.

Evolving code

Let’s say you need an Alarm to be started when a Runner runs more than a given distance. You may write this code (assuming Java-like syntax for simplicity):

class Alarm{
    private float distanceThreshold;
    private Boolean DistanceAccomplished(Runner runner){
        float distance = runner.GetTotalDistance();
        return (distance > distanceThreshold);
    }
    private void StartAlarm(){ /* code */ }
    
    public void Update(Runner runner){
        if(DistanceAccomplished(runner)){
            StartAlarm();
        }
    }
};

This looks very simple and straightforward, right? Yes, it is, for now… You are doing Agile and, in a few sprints, the product owner asks you to implement this new amazing feature. As a consequence, GetTotalDistance won’t return a float anymore and it will return an Odometer object instead, because the software needs it to work that way. So you modify the Alarm class accordingly:

class Alarm{
    private Boolean DistanceAccomplished(Runner runner){
        Odometer odo = runner.GetTotalDistance();
        float distance = odo.GetDistance();
        return (distance > distanceThreshold);
    }
    private void StartAlarm(){ /* code */ }
    
    public void Update(Runner runner){
        if(DistanceAccomplished(runner)){
            StartAlarm();
        }
    }
};

A small change here and there, not a big deal. The sprint ends succesfully and everyone is happy. Some time later the product owner comes again saying that users need to be able to configure these alarms. Depending on the setting, the alarm has to be started with the runner’s distance or with the runner’s time. Also, both thresholds need to be user configurable. At the end of the sprint, the Alarm class looks like this:

class Alarm{
    private Boolean DistanceAccomplished(Runner runner){
        Odometer odo = runner.GetTotalDistance();
        float distance = odo.GetDistance();
        float distanceThreshold = AppSettings.GetValueOfAlarmByDistance();
        return (distance > distanceThreshold);
    }
    
    private Boolean TimeAccomplished(Runner runner){
        TimeCounter timeCounter = runner.GetRunning();
        float time = timeCounter.GetTime();
        float timeThreshold = AppSettings.GetValueOfAlarmByTime();
        return (time > timeThreshold);
    }
    private void StartAlarm(){ /*code */ }
    
    public void Update(Runner runner){
        if(AppSettings.UseAlarmByDistance()){
            if(DistanceAccomplished(runner)){
                StartAlarm();
            }
        }
        else if(AppSettings.UseAlarmByTime()){
            if(TimeAccomplished(runner)){
                StartAlarm();
            }
        }
    }
};

This code can still look simple enough when seeing only this snippet, but this is a simplified example for the article. In the real world, this would be more complex.

Simple changes bring a complex picture

Natural evolution of code can bring you to these kind of scenarios. It’s natural and it’s normal, you want to add this new functionality and it only requires some small changes. Now assuming you don’t work alone, there will be small changes on the code base made by you, small changes made by your co-worker next to you and small changes by that guy/girl over there. You don’t want to spend 4 or 5 days refactoring and re-architecting some parts of your software only because of this one feature that you can implement in 4 hours. In teams without a clear architect role, this can be a total mess.

Maybe one day you happen to be adding a unit test to the Race class — wait, didn’t it have unit tests before? It seems it didn’t. You start coding the test and find yourself dealing with some strange dependencies without really knowing why, only because you need them to write the test and make the class work. And here you are, dealing with highly coupled code. Maybe that’s why it didn’t have tests before. Maybe you’ll give up adding this test also.

63120402

I love pasta and especially I love spaghetti. But I want to eat it, not to deal with it on the code I’m working on.

Being selfish

For the health of everyone, someone needs to spend some time refactoring this mess. Questions needs to be asked, lines need to be set and not crossed. Does the Alarm class need to know about AppSettings? Does it need to know about Odometer, TimeCounter or even about Runner? Can the Alarm class be split into two or more classes? Is it good to do it?

For some time now I’ve been thinking that code should be simple in the way that classes take selfishness to the limit, or try to. In some cases this can easily be solved with dependency inversion, especially the ones related with DB or filesystem access. But sometimes dependency inversion won’t make it any better. For instance, in my opinion, Alarm class should know absolutely nothing about AppSettings. Neither about the Runner class. “But it’s an Alarm for the Runner!“, some would say. Yes, it is, but that doesn’t mean it needs to know it, right now it only needs an Odometer and a TimeCounter. But even more, it doesn’t need to know both at the same time!

That’s why inheritance was created. We could make an AlarmBase class and have AlarmByDistance and AlarmByTime classes inherit from it. But we can still take this to the next level and make them implement an AlarmInterface. In both situations, either AlarmByDistance or AlarmByTime would be instantiated depending on the user settings. We have moved that responsibility to someone else. We’re being selfish because we don’t want that responsibility, this is good in this context.

Note that in the next example, the threshold values are passed to the class constructors. It’s the only thing the Alarm classes need to know about the setting — the threshold value.

interface AlarmInterface{
    public void Update();
};

class AlarmByDistance implements AlarmInterface{
    public AlarmByDistance(Odometer odometer, float distanceThreshold){
        mOdo = odometer;
        mDistanceThreshold = distanceThreshold;
    }
    
    @Override
    public void Update(){
        if(DistanceAccomplished()){
            StartAlarm();
        }
    }
    
    private void StartAlarm(){ /* code */ }
    
    private Boolean DistanceAccomplished(){
        float distance = mOdo.GetDistance();
        return (distance > mDistanceThreshold);
    }
    
    private Odometer mOdo;
    private float mDistanceThreshold;
};

class AlarmByTime implements AlarmInterface{
    public AlarmByTime(TimeCounter timer, float timeThreshold){
        mTimer = timer;
        mTimeThreshold = timeThreshold;
    }
    
    @Override
    public void Update(){
        if(TimeAccomplished()){
            StartAlarm();
        }
    }
    
    private void StartAlarm(){ /* code */ }
    
    private Boolean TimeAccomplished(){
        float time = mTimer.GetTime();
        return (time > mTimeThreshold);
    }
    
    private TimeCounter mTimer;
    private float mTimeThreshold;
};

With this new behaviour, the next code snippet needs to be somewhere else. But it’s important to note that it’s not inside the Alarm class anymore. Who does it depends on your project architecture, maybe also a new class.

AlarmInterface activeAlarm;
if(AppSettings.UserAlarmByDistance()){
    float thresholdValue = AppSettings.GetValueOfAlarmByDistance();
    Odometer odo = runner.GetTotalDistance();
    activeAlarm = new AlarmByDistance(odo, thresholdValue);
}
else if(AppSettings.UseAlarmByTime()){
    float thresholdValue = AppSettings.GetValueOfAlarmByTime();
    Odometer odo = runner.GetTotalDistance();
    activeAlarm = new AlarmByDistance(odo, thresholdValue);
}

Besides of people’s behaviour, I think code should be selfish, really selfish. A class should only know about and be responsible of the attributes it has and belong to its abstraction level. I want to remark I didn’t say “the attributes it uses” because the Alarm class was using AppSettings before, but it didn’t have it. Not only does it not need to know anything else, but it has to avoid any reference to them.

Next time you write code, try to KISS it: Keep it simple and selfish!

Leave a Reply