Coding at the wrong level of abstraction

As projects mature and grow, sometimes it becomes harder for developers to keep the codebase clean and modular. This leads to new features implementation and bug fixes being more of a juggling exercise of touching different sections of code, than an actual designed solution that follows good design patterns such as SOLID and DRY.

And excellent book to learn how to tackle these kind of problems in a codebase is Clean Code by Robert C. Martin. The book provides a lot of techniques and best practices to improve the state of our code and be more confident on our development/refactoring process.

The Problem

Let set up the context of our problem. Due to some requirements changes, one of the API endpoints in the product needed to be updated to reflect these changes. In essence, it was needed to add a check to validate that an admin user was able to add permission to other users under certain conditions.

As shown in the code below, the SetPermission API endpoint defines the Exec method, which receives three std::wstring parameters as input data and then proceeds to validate them. If any of the validations fails, it returns a common error message associated with the specific failure, otherwise it proceeds to execute the changes in permissions and audits the process.

// SetPermissionFn.h
class SetPermissionFn
{
    void Exec(std::string id,
              std::string username,
              std::string type);
}

And the method implementation in the .cpp file

// SetPermissionFn.cpp
void SetPermissionFn::Exec(std::wstring id,
                           std::wstring username,
                           std::wstring type)
{
    if (type == L"Scenario")
    {
        // Some validation checks. If failure then logs and returns

        int perm = ... // Get PermissionLevel from method call

        if (perm == -1)
        {
            // Failure. Logs and returns
            result = GlobalError::InvalidInput();
            return;
        }

        // Execute and return
    }

    int recordId;
    {
        // Find record id
        std::wstring result = Check(id, type, recordId, PermissionLevel::Full);

        if (!result.empty())
        {
            // This means it fails. Log and returns.
            result = GlobalError::InvalidObject();
            return;
        }
    }

    User user;

    {
        // Find user from username
        user = GetUserFromSomewhere(username);

        if (user.IsNull())
        {
            result = GlobalError::InvalidRecord();
            return;
        }
    }

    // Several other validations and data gathering
    ...

    // Rest of the method, including changing permisssion and audit logging.
    ...
}

Issues

The Exec implementation have several issues that I set myself to solve as part of the refactoring:

The method is way too long. A good rule of thumb would be that the method fits completely in the screen.
There is a misalignment between the name of the method (expected behavior) and what it does in reality. More than half of the method body deals with validation of the input data and gathering dependent information than actually performing the permissions changes.
The method is working at multiple abtraction levels. A good boy scout rule of clean code is that a method should not descend more than one level of abstraction. Thus, our Exec method should not concern itself about validating the input data; it should instead delegate that activity to some other object(s).

After having identified these main issues, I spent the bulk of my allocated time to this bug trying to figure it out the best alternative to improve this particular section of the codebase, yet trying to keep it as simple as possible.

Initial Iteration

The simple, yet powerful advice of extracting a new method from an existing one, applies reasonably well here. We could extract a free function and move all the input validation and data gathering to our new shiny function and call it within the Exec implementation. Since the method returns some data, we can create a struct to hold all the required info. Something along the following lines should do the trick:

struct Output
{
std::wstring result;
bool valid;
User user;
int recordId;
};

// SetPermissionFn.cpp
void SetPermissionFn::Exec(std::wstring id,
                           std::wstring username,
                           std::wstring type)
{
    auto output = ValidateAndExtractData(id, username, type);

    if (!output.valid)
    {
        result = output.result;
        return;
    }

    if (type == L"Scenario")
    {
        // Execute, set result and return
        ...
        return;
    }

    ... Rest of the method, including changing permisssion and audit logging.
    // It now uses output.user and output.recordId to access the those parameters.
}

Output ValidateAndExtractData(std::wstring id,
                           std::wstring username,
                           std::wstring type)
{
    Output output;
    output.valid = true;

    if (type == L"Scenario")
    {
        // Some validation checks. If failure then logs and returns

        int perm = ... // Get PermissionLevel from method call

        if (perm == -1)
        {
            // Failure. Logs and returns
            output.result = GlobalError::InvalidInput();
            output.valid = false;
        }

        return output;
    }

    int recordId;
    {
        // Find record id
        std::wstring result = Check(id, type, recordId, PermissionLevel::Full);

        if (!result.empty())
        {
            // This means it fails. Log and returns.
            output.result = GlobalError::InvalidObject();
            output.valid = false;
            return output;
        }

        output.recordId = recordId;
    }

    User user;
    {
        // Find user from username
        user = GetUserFromSomewhere(username);

        if (user.IsNull())
        {
            output.result = GlobalError::InvalidRecord();
            return output;
        }

        output.user = user;
    }

    // Several other validations and data gathering
    ...

    return output;
}

Refactoring is always a work in progress, but even these tiny changes makes us feel better about the state of code, and at least for me, they facilitate my understanding and ability to reason about what is happenning.

Unfortunately, we cannot rest just yet. Although we have improved the code, there is still work left to do. The current version has still some room for improvements, mainly the possibility (or the lack thereof) of reusing the validation code for other API endpoints. An option could be breaking the ValidateAndExtractData into smaller functions, each one doing one specific validations, but that will only take us so far. What happens if we want to reuse our validation code in unrelated API endpoints?

Second Iteration

After finishing with the first iteration of changes, and tested it of course :), I sat back and thought about how to extract these common validations and design them in an extensible manner for reuse across our codebase. Drawing from my previous Rails experience, I came with the idea of designing having a Validator object per API endpoint. In turn, each validator will be composable from many small and specific Validation objects which may be reused as part of other API’s Validators.

So let’s start breaking this approach down. If we take a look again at the ValidateAndExtractData again, it is clear that we have 3 different validations under the hood:

If the type is a Scenario, then it does some permissions checks and finishes.
If the type is not a Scenario, then it proceeds to find the corresponding recordId that matches the id received as parameter.
Finally, it checks if it can obtain a User record from the database.

Thus, we could design our set of validations as follows:

First, our abstract base class with only pure virtual method (i.e. interface).

// IValidation interface

class IValidation
{
public:
    IValidation();

    virtual ~IValidation() { }; // Very important for polymorphism

    virtual bool Validate() = 0;

    virtual std::wstring Result() const = 0;
};

Follow by the derived validations declarations.

// Validations header

class ScenarioTypeValidation : public IValidation
{
public:
    ScenarioTypeValidation(std::wstring type);

    virtual ~ScenarioTypeValidation() override;

    virtual bool Validate() override;

    virtual std::wstring Result() const
    {
        return result;
    }

private:
    std::wstring result;
    std::wstring type;
};

class UserValidation : public IValidation
{
public:
    UserValidation(std::wstring username);

    virtual ~UserValidation() override;

    virtual bool Validate() override;

    virtual std::wstring Result() const
    {
        return result;
    }

    User User() const
    {
        return user;
    }

private:
    std::wstring result;
    std::wstring username;
    User user;
};

// RecordIdValidation class is defined similarly.
...

And their corresponding implementation:

ScenarioTypeValidation::ScenarioTypeValidation(std::wstring type)
: type { type },
  result { L"Ok" }
{ }

ScenarioTypeValidation::~ScenarioTypeValidation()
{ }

bool ScenarioTypeValidation::Validate()
{
    if (type != L"Scenario") return true;

    // Some validation checks. If failure then logs and returns

    int perm = ... // Get PermissionLevel from method call

    if (perm == -1)
    {
        // Failure. Logs and returns
        result = GlobalError::InvalidInput();
        return false;
    }

    return true;
}

UserValidation::UserValidation(std::wstring username)
:
    username { username },
    result { L"Ok" }
{ }

UserValidation::~UserValidation()
{ }

bool UserValidation::Validate()
{
    // Find user from username
    user = GetUserFromSomewhere(username);

    if (user.IsNull())
    {
        result = GlobalError::InvalidRecord();
        return false;
    }

    return true;
}

With the validations code in place, we can build the Validator object. The validator will act as the public face for the validations; it will compose them in the appropriate order, pass the required input parameters to validate and prepare the output.

Below is the SetPermissionValidator declaration:

// SetPermissionValidator.h

class SetPermissionValidator
{
    struct ValidatorOutput
    {
        User user;
        int recordId;
    };

public:
    using output = ValidatorOutput;

    SetPermissionValidator(
        std::wstring id,
        std::wstring type,
        std::wstring username);

    ~SetPermissionValidator();

    bool Validate();

    output Output() const
    {
        return output;
    }

    std::wstring Result() const
    {
        return result;
    }

private:
    output output;
    std::wstring id;
    std::wstring type;
    std::wstring username;
    std::wstring result;
} ;

And the implementation:

// SetPermissionValidator.cpp
#include "SetPermissionValidator.h"

SetPermissionValidator::SetPermissionValidator(
    std::wstring id,
    std::wstring type,
    std::wstring username
):
    id { id },
    type { type },
    username { username },
    result { L"Ok" }
{ }

SetPermissionValidator::~SetPermissionValidator()
{ }

bool SetPermissionValidator::Validate()
{
    ScenarioTypeValidation typeValidation { type };

    if (!typeValidation.Validate())
    {
        result = typeValidation.Result();
        return false;
    }

    RecordIdValidation recordIdValidation { id };

    if (!recordIdValidation.Validate())
    {
        result = recordIdValidation.Result();
        return false;
    }

    UserValidation userValidation { username };

    if (!userValidation.Validate())
    {
        result = userValidation.Result();
        return false;
    }

    output.recordId = recordIdValidation.RecordId();
    output.user = userValidation.User();

    return true;
}

Now that we have successfully designed our new Validator object, let’s update the Exec body.

// SetPermission.cpp
#include "SetPermissionValidator.h"

void SetPermissionFn::Exec(std::wstring id,
                           std::wstring username,
                           std::wstring type)
{
    SetPermissionValidator validator {id, username, type};

    if (!validator.Validate())
    {
        result = validator.Result();
        return;
    }

    if (type == L"Scenario")
    {
        // Execute, set result and return
        ...
        return;
    }

    auto output = validator.Output();

    ... Rest of the method, including changing permisssion and audit logging.
    // It now uses output.user and output.recordId to access the those parameters.
}

Future Improvements

Although I think our code looks better now, I fairly believe that it can be improved upon. Those improvements will come in future iterations as newer features or refactoring exercises get in touch with our API layer.

As of now, I can see several lines of work that can spawn from this initial endeavor:

Utilization of templates to facilitate the reuse of existing validations for different objects.
Implement a validators hierarchy to abstract common functionalities.
Introduction of more functional features, such as std::unary_function and std::function to allow creation of validations on the fly.

Conclusions

Let’s make our final takeaways from the post. We reviewed a section of code that, although functionally correct, it lacked in terms of proper application of SOLID principles. We started small and slowly build a set of independent classes to share the validation workload. First, we moved all the input checking into a separate function. Then, we realized that the improvement wasn’t enough and we could get bigger benefits with more separation of concerns. Thus, it became very clear that breaking down into classes the function was the next steps. Finally, we outlined several future lines of work to improve further upon this refactoring exercise.

Thanks for reading till the very end. I hope you learnt something useful and enjoyed the analysis. I did it for sure. If you have anything to share, I would love to hear in the comments. See you soon and stay tuned for more!!