Finding a way to obtain some volume of water using a large and small bucket

Question

I have a set of if/else statements that trigger certain actions. As a first step, I was thinking about moving the logic in each if statement to be its own function, but are there any recommendations on what design patterns can help clean this code up?

I guess I am trying to see if there is a cleaner way to determine what the next action/command should be based on a set of criteria. I see people recommend the Command pattern, but how does one abstract the logic in the if-statement to begin with?

function WaterRetriever(largeBucketSize, smallBucketSize, waterSize) {
    var buckets = {};

    if (waterSize > largeBucketSize) {
        throw new Error("The buckets are not large enough.");
    }

    buckets.firstContainer = new Bucket(largeBucketSize);
    buckets.secondContainer = new Bucket(smallBucketSize);

    this.getWaterFromLake = function () {
        var maxSteps = 25,
            step = 1;

        buckets.firstContainer.fill();

        while (step <= maxSteps && buckets.firstContainer.getCurrentAmount() !== waterSize) {
            if (buckets.firstContainer.isFull() && buckets.secondContainer.isEmpty()) {
                buckets.firstContainer.transferTo(buckets.secondContainer);

            } else if (buckets.secondContainer.isFull() && buckets.firstContainer.hasCapacityAvailable()) {
                buckets.secondContainer.empty();
                buckets.firstContainer.transferTo(buckets.secondContainer);
            } else if (buckets.firstContainer.isEmpty() && buckets.secondContainer.hasCapacityAvailable()) {
                buckets.firstContainer.fill();
            } else if (buckets.firstContainer.isFull() && buckets.secondContainer.hasCapacityAvailable()) {
                buckets.firstContainer.transferTo(buckets.secondContainer);
            }
            console.log(buckets.firstContainer.getCurrentAmount() + ", " + buckets.secondContainer.getCurrentAmount());
            step = step + 1;
        }

        if (step > maxSteps) {
            console.log("could not find a solution within the steps limit...");
        }
    };
}

Answer 1

I found your conditions to be a little confusing. It was hard for me to understand the rules of what is going on by just a quick glance. In this simple case, you don't necessarily need to create more abstraction, just be more clear about presenting the rules.

One way to make your code more readable is to try and avoid an if/else maze if a series of ifs could suffice instead. There's a certain amount of personal preference to that, however, when there are many if/else's, your brain has to stack up and keep track of all the previous conditions. It's better to check a condition, then handle it immediately and return to a higher scope instead of nesting further. By not nesting further, you can "forget" about it and simply move on to the next condition.

A trivial example example:

function describeHeight(person) {
    var height = person.height;
    if (height < 4) {
        console.log('short');
    }
    else if (height >= 4 && height < 6) {
        console.log('average');
    }
    else if (height >= 6 && height < 8) {
        console.log('tall');
    }
    else {
        console.log('giant!!!');
    }
}

Versus:

function describeHeight(person) {
    var height = person.height;
    if (height >= 8) {
        console.log('giant!!!');
        return;
    }
    if (height >= 6) {
        console.log('tall');
        return;
    }
    if (height >= 4) {
        console.log('average');
        return;
    }
    console.log('short');
}

Not the best example, perhaps. But I feel like the second version is easier to read. See how the first example has more nesting, and the second has less?

In your example, I would try to simplify the rules. The logic may not be correct here, because I don't exactly understand your rules, but something like this may be easier to read:

while (step++ <= maxSteps) {
    // Check if container has the appropriate amount of water.
    if (buckets.firstContainer.getCurrentAmount() === waterSize) {
        break;
    }

    // If first container is empty, then fill it.
    if (buckets.firstContainer.isEmpty()) {
        buckets.firstContainer.fill();
        continue;
    }

    // If first container is full, fill up the second container if there's room.
    if (buckets.firstContainer.isFull() && buckets.secondContainer.hasCapacityAvailable()) {
        buckets.firstContainer.transferTo(buckets.secondContainer);
        continue;
    }

    // If second container is full, dump it out, and try to fill up the first container if there's room.
    if (buckets.secondContainer.isFull() && buckets.firstContainer.hasCapacityAvailable()) {
        buckets.secondContainer.empty();
        buckets.firstContainer.transferTo(buckets.secondContainer);
        continue;
    }
}

Answer 2

In this case, in every step you could do:

buckets.secondContainer.empty();
buckets.firstContainer.transferTo(buckets.secondContainer);
buckets.firstContainer.fill();

Then inside each function you could test if there is anything to do and if not, return.

void Empty()
{
    if(this.isEmpty()) return;
    ...
}
void TransferTo(Bucket dest)
{
    if(this.isEmpty() || dest.isFull()) return;
    ...
}
void Fill()
{
    if(this.isFull()) return;
}

That should give you the same behaviour without all the if/elses. The checks could also be performed in the main loop but I assume you are already checking for them inside the functions.

Answer 3

I created an action handler class that just executes the next step.. It may be overkill, but this project is for learning purposes anyway...

Would something like this be more readable than my original code?

function calculateSteps() {
    var maxSteps = 100,
        step = 1;

    while (step <= maxSteps && !isCalculationDone()) {
        bucketActionHandler.executeNextStep();
        createSnapshot();
        step = step + 1;

    }

    if (step > maxSteps) {
        console.log("could not find a solution within the steps limit...");
    }
    amplify.publish(events.calculationCompleted, snapshots);

}

function BucketActionHandler(containers, size) {
    var buckets = containers,
        waterSize = size,
        waterSizeSmallContainerOffset = waterSize - containers.secondContainer.getCurrentAmount();

    function shouldFinalRetrievalByDumpingExtra() {
        return buckets.firstContainer.getCurrentAmount() === waterSize && buckets.secondContainer.isFull();
    }

    function shouldTransferToSmallBucket() {
        return buckets.firstContainer.isFull() && !buckets.secondContainer.isFull();
    }

    function shouldFillLargeContainer() {
        return buckets.firstContainer.isEmpty() && buckets.secondContainer.hasCapacityAvailable();
    }

    function firstContainerTargetOffsetReached() {
        return buckets.firstContainer.getCurrentAmount() === waterSizeSmallContainerOffset;
    }

    function shouldTransferFromLargeToSmallBucket() {
        return buckets.secondContainer.isFull() && buckets.firstContainer.hasCapacityAvailable();
    }

    this.executeNextStep = function () {
        if (shouldTransferToSmallBucket()) {
            buckets.firstContainer.transferTo(buckets.secondContainer);
            return;
        }

        if (shouldFinalRetrievalByDumpingExtra()) {
            buckets.secondContainer.empty();
            return;
        }

        if (shouldFillLargeContainer()) {
            buckets.firstContainer.fill();
            return;
        }


        if (firstContainerTargetOffsetReached && buckets.secondContainer.isEmpty()) {
            buckets.secondContainer.fill();
            return;
        }

        if (firstContainerTargetOffsetReached() && buckets.secondContainer.isFull()) {
            buckets.secondContainer.transferTo(buckets.firstContainer);
            return;
        }

        if (shouldTransferFromLargeToSmallBucket()) {
            buckets.secondContainer.empty();
            buckets.firstContainer.transferTo(buckets.secondContainer);
            return;
        }
    };
    buckets.firstContainer.fill();
}