Code kata time - FooBarQix with vanilla JavaScript

I was advised yesterday that using normal vanilla JavaScript for a kata might be of a benefit.

You might have come across the FizzBuzz kata before. Well FooBarQix is the grown up brother of that, resulting in numbers that look like:

FooBarBar, Qix, 197, Foo, 199, Bar**, Foo*, 2*2, Qix*Foo, Foo*, Bar*Bar, 2*6, Foo*Qix, 2*8

The requirements are:

  • If the number is divisible by 3, write “Foo” instead of the number
  • If the number is divisible by 5, add “Bar”
  • If the number is divisible by 7, add “Qix”
  • For each digit 3, 5, 7, add “Foo”, “Bar”, “Qix” in the digits order.

And after that’s complete, there’s one final requirement:

  • We must keep a trace of 0 in numbers, each 0 must be replaced with char “*“

Attempting to do that without guidance gets quite tricky indeed. Try doing it without tests and you’re very likely to get things wrong, or to give up in the attempt, especially with the “*” requirement.

Getting started

We can start this off with a simple index.html page, that requests the code script and a test script.

<!DOCTYPE html>
<html>
<head>
    <title>FooBarQix Kata</title>
    <link rel="stylesheet" href="style.css">
</head>
<body>
<h1>FooBarQix Kata - 2018-01-02</h1>
<script src="foobarqix.js"></script>
<script src="foobarqix.test.js"></script>
<script>
</script>
</body>
</html>

We’ll add something into style.css later, and we’ll also add some code in the script area to loop through some values and show them on the page, but that can occur after we have some values coming from the foobarqix() function.

Test with an expect function

Without using any external libraries, we can create our own custom-made expect function.

These can be made to be nice and colorful in the browser console, but the code can get to be a bit complex and bulky, so this is a fairly minimal approach instead that gets us going nice and quickly,

foobarqix.test.js

function expect(actual, expected, msg) {
    console.log(msg);
     if (actual !== expected) {
        console.error(" Expected " + expected + " but got " + actual);
    }
}

That’s all you need to get started with testing. Other things can be added on from here, which is why there’s a wide range of testing libraries out there, but just checking that one value equals another, is the core of testing.

First test

According to the rules on the page, the foobarqix() is to accept a string and to return a string.

The three laws of test-driven development are of good value to know, but I like to simplify them to:

  • Write a failing test
  • with the least code to make it pass
  • then refactor

That forms a nice tight loop, that results in working code that works better and better each time.

We can start our first test by writing a simple test below the expect function in the test code.

foobarqix.test.js

expect(typeof foobarqix, "function", "The foobarqix function exists");

With the index.html page loaded and the browser console open, reloading the index page shows our first test in the console area.

The test fails, as it properly should do, and tells us that it expects a function. Let’s give it one.

foobarqix.js

function foobarqix() {
}

And on reloading the page, the test passes. This is a good first test as it helps to ensure that the filenames are all spelled correctly, and that everything is plumbed together properly.

I don’t want to have to reload the index page manually myself every time myself after every test and after writing code, so let’s have the page reload itself. That way when we save the file, the web page will automatically update and show us the result. I tried doing the update every 1 second but the flicker became too distracting, so 2 seconds seems about right.

function expect(actual, expected, msg) {
    ...
}
setTimeout(() => window.location = window.location, 2000);

Test for no input

The next test helps us to deal with no input to the function.

expect(foobarqix(), "", "No arguments should give us an empty string");

Then check the index.html page, to make sure that the console is correctly showing an error. If it doesn’t show an error then we are testing the wrong thing, and cannot know if the code that we write actually helped. Making the test pass helps us to know that our code has been beneficial.

In this case, we can return an empty string to make the test pass:

function foobarqix() {
    return "";
}

We could carry on testing for things that shouldn’t occur, which is the thorns around the gold idea, but that’s something that can be done on a future project when there’s a more useful benefit to be gained in protecting people from bad behaviour.

Next up though, we’ll begin testing what the foobarqix() function is supposed to achieve.

1 Like

Testing 1

The first actual result from our function happens here, with “1”. The Kata page says You should implements a function String compute(String) so we are going to use strings here instead of numbers.

These are slow steps that we are taking, but they are good practice for when we get in to more complicated situations.

The test is:

expect(foobarqix("1"), "1", "1 should give 1");

On the index page look for the failing test in the browser console. It expected 1 but it got nothing.
It’s tempting to add function parameters and return that from the function, but that’s a lot more complex than other solutions. Even though we have a good idea of where the code is going to go, we must design our tests to drive the code in the direction that we want it to go.

When it comes to the increasing complexity of code, there is a neat list called the TPP (transformation priority premise) where the higher up transformations are less complex and are easier to achieve than the ones lower down.


A function parameter is a variable assignment which is nearly at the bottom of the list, but it’s the only transformation that helps us to pass this test.

Even though it’s a number being given to the function, it’s as a string, so I’ll use value as the function parameter.

function foobarqix(value) {
    return "";
}

We can now use an if statement to change the function’s behaviour based on the function parameter, but before I return the value, notice what happens to the test when I add in the if statement.

function foobarqix(value) {
    if (!value) {
        return "";
    }
}

All tests pass except for the third test. Instead of getting an empty string it now gets undefined, which in the words of Corey Haines from his roman numerals kata with commentary(video) , “is a wonderfuly simple transformation according to TPP”.

We can now get the third test to pass by returning “1” from the function.

    if (!value) {
        return "";
    }
    return "1";

And all tests now pass.

We can now go back to our todo list and add a test to deal with numbers too.

expect(foobarqix(1), "", "Only strings should be accepted");

We can make this one pass by checking that typeof gives us a string.

function foobarqix(value) {
    if (!value || typeof value !== "string") {
        return "";
    }
    return "1";
}

Now that all of the tests pas we can consider if any refactoring needs to be done, which isn’t the case this time. We can move on to the next test.

Testing 2

expect(foobarqix("2"), "2", "2 should give 2");

Check that the index page shows a failing test, and on seeing it we can update the code.

While we could add another if statment to check for the other value, that’s at number 6 in the TPP and we have a simpler solution available to us, constant to a variable.

    // return "1";
    return value;

The tests pass, and we’re ready to move on. Now that we have some values coming back from the function, we can show these results on the index page.

1 Like

Showing the results

We can add a results section to the index page and update it with values from the foobarqix() function:

<h1>...</h1>
<div class="results"></div>
<script src="foobarqix.js"></script>
<script src="foobarqix.test.js"></script>
<script>
var results = document.querySelector(".results");
var numbers = Array.from({length: 1000}).map((_, i) => String(i + 1));
results.innerHTML =
    numbers.map(foobarqix)
        .join(", ");
</script>

While that works, I want something that looks a bit better than that, with the values neatly lined up across the page.
If we use the <ul> element for the results, that will make it easier to organise things.

<ul class="results"></ul>

Each result can be added as a new list item:

var results = document.querySelector(".results");
for (let i = 1; i <= 1000; i += 1) {
    const value = foobarqix(String(i));
    const li = document.createElement("li");
    const text = document.createTextNode(value);
    li.appendChild(text);
    results.appendChild(li);
}

That gives us an index page with a long list of results.

We can turn that list into a neat 10 row grid with some CSS:

style.css

ul {
    display: grid;
    grid-template-columns: repeat(10, 1fr);
}

And we now have a pretty full screen of results.

Testing 3

The requirements say that numbers divisible by 3, 5, and 7, must show certain names.

  • If the number is divisible by 3, add “Foo” instead of the number
  • If the number is divisible by 5, add “Bar”
  • If the number is divisible by 7, add “Qix”
  • For each digit 3, 5, 7, add “Foo”, “Bar”, “Qix” in the digits order.

The test for 3 involves both division and conversion of the digit.

expect(foobarqix("3"), "FooFoo", "3 is divisible by 3, and contains 3");

Check the failing test, and we can make this one pass as easily as returning “FooFoo” instead of the value. Using divisibility is not appropriate yet at this stage though, because we really should need to use the simplest code to make it pass. We can just check for “3” instead.

    if (value === "3") {
        return "FooFoo";
    }
    return value;

We will have to split up those values at some stage, but for now things work. The tests pass, and there’s nothing to refactor here yet.

Frequently it only a good idea to refactor when you have at least two examples, and preferably three to help guide the direction of your refactoring.

1 Like

Testing 5

There’s no need to test for 4, although we could if we want a sense of completion. On to 5.

The 5 test is similar to the 3 test.

expect(foobarqix("5"), "BarBar", "5 is divisible by 5 and contains 5");

The console on the index page says that it expects BarBar instead of 5, so let’s add that.

    if (value === "3") {
        return "Foo" + "Foo";
    }
    if (value === "5") {
        return "Bar" + "Bar";
    }
    return value;

The tests pass, and do we need to refactor this code? I can tell that we’re going to want to place the 3/5/7 numbers into an array, or an object. The 6 test should help us to resolve this issue.

It’s on to 6, which should help is when it comes to checking for multiples of 3.

Testing 6

expect(foobarqix("6"), "Foo", "6 is divisible by 3");

First we check that the test fails, and then make the code pass:

    if (value === "3") {
        return "FooFoo";
    }
    if (value === "5") {
        return "BarBar";
    }
    if (value === "6") {
        return "Foo";
    }

And now that the code is passing we can refactor the code.

Now that we have both 3 an 6, we can justify using the modulus operator to check if the value is divisible by 3.

    if (value % 3 === 0) {
        return "Foo";
    }
    if (value === "5") {
        return "BarBar";
    }

But now, either 3 or 6 fails because they need different results. This is now a good time to introduce a local variable.

I don’t know what to call the variable. It could be fbq as that’s short for FooBarQix, but even a generic variable called str seems better than that option, so we’ll use that.

    var str = "";
    if (value % 3 === 0) {
        str = "Foo";
    }
    if (value === "3") {
       str += "Foo";
    }
    if (value === "5") {
       str = "BarBar";
    }
    return str;

While that works for numbers 3-6, numbers 1-2 fail. We need to return the value only if str is empty.

    if (!str) {
        return value;
    }
    return str;

and all tests now pass.

1 Like

Testing 7

The next test is for “Qix” when divisible by 7.

expect(foobarqix("7"), "QixQix", "7 is divisible by 7 and contains 7");

The code to make this pass is going to result in quite some duplication, but the test will pass and we can do refactoring to remove some of that duplication.

    if (value % 3 === 0) {
        str = "Foo";
    }
    if (value === "3") {
       str += "Foo";
    }
    if (value === "5") {
       str = "BarBar";
    }
    if (value === "7") {
       str = "QixQix";
    }

Now that the test passes and there are no errors, we can refactor. The code here is clearly doing two different things, one related to division, and the other to do with digits in the number. Looking at the TPP I see that arrays and while loops are simpler than a function, so let’s let’s put those Foo/Bar/Qix names in an array:

var names = ["Foo", "Bar", "Qix"];
    if (value % 3 === 0) {
        str = names[0];
    }
    if (value === "3") {
       str += names[0];
    }
    if (value === "5") {
       str = names[1] + names[1];
    }
    if (value === "7") {
       str = names[2] + names[2];
    }

Testing 10

The tests for 8 and 9 aren’t needed, so we move on to 10.

expect(foobarqix("10"), "Bar", "10 is divisible by 5");

We can make this pass as we did with 3, by checking if the value is evenly divisible by 5, and updating the

    if (value % 5 === 0) {
        str = names[1];
    }
    if (value === "5") {
       str += names[1];
    }

The example at the FooBarQix page shows that 13 is next after 10. We can simplify the code when the 14 test occurs.

1 Like

Testing 13

This test helps us to improve recognition of digits within the value.

expect(foobarqix("13"), "Foo", "13 contains a 3");

We can make this one pass by … no. Hold on, the simplest thing first.

An if statement for 13 is the simplest thing.

    if (value === "3") {
       str += names[0];
    }
    if (value === "13") {
       str += names[0];
    }

Now that the tests all pass, we can refactor.

Both 3 and 13 contain the digit 3, so we can combine the two if statements into one, with:

    if (value.includes("3")) {
       str += names[0];
    }

All tests pass, and I can see the pattern that’s forming in the code now, using the modulus operator to tell if the value is a multiple, and using String.includes() to find out if the the value contains a specific number.

Testing 14

A test for 14 will give us the last of the divisor checks that we need.

expect(foobarqix("14"), "Qix", "14 is divisible by 7");

Which fails, but with this code:

    if (value % 7 === 0) {
       str = names[2];
    }
    if (value === "7") {
       str += names[2];
    }

the test passes.

We can now work on removing duplication from that divisor code.

    ["3", "5", "7"].forEach(function (factor, index) {
        if (value % factor === 0) {
            str = names[index];
        }
    });
    // if (value % 3 === 0) {
    //     str = names[0];
    // }
    if (value.includes("3")) {
       str += names[0];
    }
    // if (value % 5 === 0) {
    //     str = names[1];
    // }
    if (value === "5") {
       str += names[1];
    }
    // if (value % 7 === 0) {
    //    str = names[2];
    // }
    if (value === "7") {
       str += names[2];
    }

The 15 and the 17 test will help to drive out the requirement for String.includes on those other if statements,

Testing 15

expect(foobarqix("15"), "FooBarBar", "15 is divisible by 3 and 5, and contains 5");

Currently we get Bar, and need to get FooBarBar instead.

Changing = to += is a simple change that lets us get both the factor of three and of five.

        if (value % factor === 0) {
            str += names[index];
        }

And updating the if statement for 5 let’s us also match the 5 digit that’s in 15.

    if (value.includes("5")) {
       str += names[1];
    }

And all of the tests pass. The 17 test is where we’ll do more beneficial refactoring,

1 Like

Testing 17

This test helps to force the 7 condition to be more flexible, allowing us tor refactor the rest of the code.

expect(foobarqix("17"), "Qix", "17 contains 7");

Checking the index page it’s getting 17 instead of Qix.

Updating the if 7 condition is what we need to do here, so that any 7 in the value will add Qix:

    if (value === "7") {
       str += names[2];
    }
    if (value.includes("7")) {
       str += names[2];
    }

All of the tests pass, and we can now focus on refactoring the code.
This is where the suite of tests really becomes useful. Normally we are scared to make changes to complex code in fear of breaking something. Here instead though, we have tests that check everything that we care about remains working exactly as it should be, so we can easily catch any issues before they become a problem.

This is the code that I currently want to refactor:

    if (value.includes("3")) {
       str += names[0];
    }
    if (value.includes("5")) {
       str += names[1];
    }
    if (value.includes("7")) {
       str += names[2];
    }

I want to use a forEach loop for those, which means moving 3/5/7 out to a separate array.

    // if (value.includes("3")) {
    //    str += names[0];
    // }
    // if (value.includes("5")) {
    //    str += names[1];
    // }
    // if (value.includes("7")) {
    //    str += names[2];
    // }
    ["3", "5", "7"].forEach(function (digit, index) {
        if (value.includes(digit)) {
            str += names[index];
        }
    });

That 3/5/7 array is now used by both sets of code, so we should use a well named variable for that.

    var names = ["Foo", "Bar", "Qix"];
    var factors = ["3", "5", "7"];
...
    factors.forEach(function (factor, index) {
        ...
    });
    factors.forEach(function (digit, index) {
        ...
    });

I also don’t like how index is being used to access the names array. I want to use the factor itself to retrieve the information, which can be done an object, but a hash map seems to be the more correct system to use.

    var names = new Map([
        ["3", "Foo"],
        ["5", "Bar"],
        ["7", "Qix"]
    ]);
    var str = "";
    names.forEach(function (name, factor) {
        if (value % factor === 0) {
            str += name;
        }
    });
    names.forEach(function (name, digit) {
        if (value.includes(digit)) {
            str += name;
        }
    });

That code is now looking much improved, and the changes were easy to make too.

1 Like

Testing 21

The example page shows that 21, 33, 51, and 53 are next to test, so it seems reasonable that they are there to help improve the behaviour of the function. It’s certainly not possible to tell if it’s working properly by inspection of the index page.

expect(foobarqix("21"), "FooQix", "21 is divisible by 3 and 7");

This test works without any problems, so it’s on to 33

Testing 33

expect(foobarqix("33"), "FooFooFoo", "33 is divisible by 3 and contains two 3's");

We only got “FooFoo” this time. Why would that be? The first Foo is from the divisors, and the second foo is from the digits.

We’re checking the digits the wrong way. Currently we are going through each of the factors, but instead we need to go through each digit and check if that digit is in with the names.

    // names.forEach(function (name, digit) {
    //     if (value.includes(digit)) {
    //         str += name;
    //     }
    // });
    value.split("").forEach(function (digit) {
        if (names.get(digit)) {
            str += names.get(digit);
        }
    });

The tests all work now, which is a relief, and we can move on to the next test.

Testing 51

expect(foobarqix("51"), "FooBar", "51 is divisible by 3 and contains 5");

This test passes fine without adjustment to the code, and it’s on to the last test, before the final asterisk round.

Testing 53

expect(foobarqix("53"), "BarFoo", "53 contains 5 and 3");

This test works perfectly well too.

The main code that we currently have is:

function foobarqix(value) {
    if (!value || typeof value !== "string") {
        return "";
    }
    var names = new Map([
        ["3", "Foo"],
        ["5", "Bar"],
        ["7", "Qix"]
    ]);
    var str = "";
    names.forEach(function (name, factor) {
        if (value % factor === 0) {
            str += name;
        }
    });
    value.split("").forEach(function (digit) {
        if (names.get(digit)) {
            str += names.get(digit);
        }
    });
    if (!str) {
        return value;
    }
    return str;
}

and all of the tests are passing fine. It’s time now for the bonus round, of converting zero’s into asterisks.

1 Like

Testing asterisks

The last requirement of the FooBarQix kata, is:

  • We must keep a trace of 0 in numbers, each 0 must be replaced with char “*“

Testing 101

The first test we’re given for the asterisks is to convert 101 into 1*1

expect(foobarqix("101"), "1*1", "101 contains a 0 which converts to *");

We should be able to achieve that just by replacing the 0’s with * in the returned value:

    if (!str) {
        // return value;
        return value.replace("0", "*");
    }

And the test passes, good.

There are three more tests to go:

  • 303 => FooFoo*Foo
  • 105 => FooBarQix*Bar
  • 10101 => FooQix**

Testing 303

expect(foobarqix("303"), "FooFoo*Foo", "303 is divisible by 3, contains two 3's and the 0 converts to *");

I should be able to adjust the value variable before it gets checked for digits.

This test is harder to get working. If I add the asterisk inside of the digit check, it causes the 101 test to fail, and fails the 10 test.

    value.split("").forEach(function (digit) {
        if (names.get(digit)) {
            str += names.get(digit);
        }
        if (digit === "0") {
            str += "*";
        }
    });
// Expected Bar but got Bar*
// Expected 1*1 but got *

The 10 needs to be Bar* because the 0 gets converted. I’ll update that test so that it doesn’t become distracting:

expect(foobarqix("10"), "Bar*", "10 is divisible by 5 and the 0 converts to *");

There’s only one failing test now where 1*1 is currently being given as just as asterisk.

The if (!str) condition at the end of the code doesn’t seem to be good enough anymore. It’s not just when str is empty that we want the value, but also when none of the digits match the factors.

How can we check that? Adding the suffix on to str is helping to confuse things here because we have no factors of 101, but the digits and asterisk are being added on to it, so we need to separate them using a separate suffix variable.

We can also reduce the number of if statements inside of the map too:

    var suffix = value.split("").map(function (digit) {
        // if (names.get(digit)) {
        //     str += names.get(digit);
        // }
        // if (digit === "0") {
        //     str += "*";
        // }
        return names.get(digit) ||
            (digit === "0"
                ? "*"
                : "");
    }).join("");

After that improvement we still only have that 101 test failing, all the rest of the still pass.

That the suffix has an asterisk in it is confusing things, so if I remove the suffix when checking it it’s empty, that should help things.

    // if (!str && !suffix) {
    if (!str && !(suffix.replace("*", ""))) {

And all the tests pass. Good. We can now work at improving the code from here.

The asterisk was removed because I needed to know if there were any 3/5/7 digits that would get added to that suffix. Instead of removing the asterisk, I could instead check if any of the factors are in the value.

    var hasFactorAsDigit = value.split("").filter(function (digit) {
        return names.has(digit);
    }).length > 0;
...
    // if (!str && !(suffix.replace("*", ""))) {
    if (!str && !hasFactorAsDigit) {

Extracting functions

I want to move several pieces of code out to separate functions now. There are three of them that I can see as being very useful:

  • hasFactorAsDigit()
  • namesFactors()
  • namedDigits()

hasValueAsDigits() function

The hasValueAsDigits code is easy to move out to a separate function:

function hasFactorAsDigit(names, value) {
    return value.split("").filter(function (digit) {
        return names.has(digit);
    }).length > 0;
}
...
    // var hasFactorAsDigit = value.split("").filter(function (digit) {
    //     return names.has(digit);
    // }).length > 0;
...
    // if (!str && !hasFactorAsDigit) {
    if (!str && !hasFactorAsDigit(names, value)) {

namedFactors() function

The divisors code needs to be reworked so that it returns the names, instead of mutating the str variable.

    // var str = "";
    // names.forEach(function (name, factor) {
    //     if (value % factor === 0) {
    //         str += name;
    //     }
    // });
    var str = Array.from(names.keys())
        .filter(function (factor) {
            return (value % factor === 0);
        }).map(function (factor) {
            return names.get(factor);
        }).join("");

And the test all still pass. Cool.

We can tighten up that code too, using arrow notation:

javascript
    // var str = Array.from(names.keys())
    //     .filter(function (factor) {
    //         return (value % factor === 0);
    //     }).map(function (factor) {
    //         return names.get(factor);
    //     }).join("");
    var str = Array.from(names.keys())
        .filter((factor) => (value % factor === 0))
        .map((factor) => names.get(factor))
        .join("");

It’s even possible to use !(value % factor) but I find that to be too difficult to understand, as compared with (value % factor === 0)

It can now be extracted out to a function:

function namedFactors(names, value) {
    return Array.from(names.keys())
        .filter((factor) => (value % factor === 0))
        .map((factor) => names.get(factor))
        .join("");
}
...
    var str = namedFactors(names, value);
    // var str = Array.from(names.keys())
    //     .filter((factor) => (value % factor === 0))
    //     .map((factor) => names.get(factor))
    //     .join("");

namedDigits() function

The namedDigits code can also be moved out to a separate function in the same way:

function namedDigits(names, value) {
    return value.split("").map(function (digit) {
        return names.get(digit) ||
            (digit === "0"
                ? "*"
                : "");
    }).join("");
}
...
    var suffix = namedDigits(names, values);
    // var suffix = value.split("").map(function (digit) {
    //     return names.get(digit) ||
    //         (digit === "0"
    //             ? "*"
    //             : "");
    // }).join("");

That then leaves us with a wonderfully small foobarqix() function:

function foobarqix(value) {
    if (!value || typeof value !== "string") {
        return "";
    }
    var names = new Map([
        ["3", "Foo"],
        ["5", "Bar"],
        ["7", "Qix"]
    ]);
    var str = namedFactors(names, value);
    var suffix = namedDigits(names, value);
    if (!str && !hasFactorAsDigit(names, value)) {
        return value.replace("0", "*");
    }
    return str + suffix;
}

We only have the 303 and 10101 tests left to go and we’re all done.

1 Like

The last two tests as listed from the FooBarQix kata are for numbers 303 and 10101

Testing 303

expect(foobarqix("303"), "FooFoo*Foo", "303 is divisible by 3, contains two 3's and the 0 converts to *");

This one tests fine and passes, which has me wondering what 10101 has in store for us.

Testing 10101

expect(foobarqix("10101"), "FooQix**", "10101 is divisible by 3 and 7, and the 0's converts to *'s");

This test would have tripped up anyone that was adding only a single asterisk, but this test passes fine too with the code, so the kata is now complete.

Cleaning up

The setTimeout to refresh the page can be commented out from the test code, which lets us examine the index page more closely.

It can be tricky to make head or tail from the output of the index page, but it’s a damned fine mess that precisely meets all of the criteria, and you can’t ask for much better than that.

I’ve added some additional criteria to the index page, to help make it more clearly understood what’s going on there:

<h1><a href="http://codingdojo.org/kata/FooBarQix/">FooBarQix Kata</a> - 2018-01-02</h1>
<h2>Rules</h2>
<ul><li>If the number is divisible by 3, write “Foo” instead of the number</li>
    <li>If the number is divisible by 5, add “Bar”</li>
    <li>If the number is divisible by 7, add “Qix”</li>
    <li>For each digit 3, 5, 7, add “Foo”, “Bar”, “Qix” in the digits order.</li>
    <li>Keep track of every 0 in the numbers, replacing them with an asterisk “*“ character.</li>
</ul>

and made sure that the CSS only affects the results, not the other list that’s there:

.results {
    display: grid;
    grid-template-columns: repeat(10, 1fr);
}

and this kata can now be considered to be complete.

The full code for this vanilla kata can be found at https://github.com/pmw57/Kata/tree/foobarqix-2018-01-02

1 Like

###A review of the kata

While doing this kata and especially after completing it, more value came from it by thinking over what had been done, and pondering on lessons learned.

  • The readme file nearly went forgotten, and should be done much closer to the beginning of the project.
  • Should modulus division be done before tests demand it? Emotions say yes, but can be wrong.
  • Too much time was spent in the middle with lots of if statements. Would a different test order help?
  • There seemed to be three stages: initial if statements, reduced duplication, and extracted to functions.
  • Could I have realised sooner that factors.forEach wouldn’t work as well as names.forEach?
  • Changing existing variables was a mistake, because the code had to be rewritten before extraction.
  • The asterisks section was quite a struggle and I stopped caring about linting errors in my code.
  • The asterisks couldn’ve been done before namedDigits() function, it shouldn’t need to know about them.
  • The namedDigits() function can just filter out non-matching numbers instead.

I was going to do a different kata tomorrow, but I think that more value will be obtained by pondering how I might have approached things diffrerently, to more easily take care of the above issues.

2 Likes

Okay, so wipe the code and start over to re-explore these issues.

###The readme file nearly went forgotten, and should be done much closer to the beginning of the project.

I should just get organised and think more about making things clearly understandable.

Showing results

The index.html page has too much code. I wan to just instead issue one function call to generate the whole thing.
Keeping my JSLint need in mind, the index script is now only the following:

/*jslint browser*/ /*global foobarqix*/
var results = document.querySelector(".results");
foobarqix.showList(1000, results);

JSLint doesn’t like it when normal for loops are used, which has helped to inspire me to find other ways, such as:

Array.from({length: limit}).map((_, i) => i + 1);

which gives an array of numbers from 1 up to whatever limit you want.

As a result of this, I’ve place these functions in a separate results.js file

    function itemsReducer(items, value) {
        const text = document.createTextNode(value);
        const li = document.createElement("li");
        items.appendChild(li);
        li.appendChild(text);
        return items;
    }
    function show(callback, limit) {
        var values = Array.from({length: limit}).map((ignore, i) => i + 1);
        var items = values.map(callback)
            .reduce(itemsReducer, document.createDocumentFragment());
        document.querySelector(".results").appendChild(items);
    }

so that the index page now only needs to call:

results.show(foobarqix, 1000);

These updates form the basis of a good initial starting position from which to proceed with the kata.

If you wish you can download them too, from Base files prepared and a starting test of 1 so that you can easily make a start with your own kata too.

1 Like

Should modulus division be done before tests demand it?

When I know the direction that the code’s going to go in, two examples are enough of a pattern to wait for instead of waiting for three.

Too much time was spent in the middle with lots of if statements. Would a different test order help?

Definitely yes.

Testing first for 3 then 6, lets us refactor out the common divisor of 3, so after those two tests and refactoring, we already have:

    var str = "";
    if (value % 3 === 0) {
        str = "Foo";
    }
    if (value === "3") {
        str += "Foo";
    }
    if (!str) {
        return value;
    }
    return str;

When adding these files, a nice piece of kismet occurred from mistyping the command.
I intended to type git add *.js but instead typed git commit *.js which brought up a nice colorful vim editor where I can enter in my commit message. A cheatsheet of vim commands is handy.

Here’s someone else’s view of the vim editor.

Anyway, the benefit gained from deciding on the order of the tests, is that I’m not just blindly going through 1, 2, 3, etc.
Instead, I get to think about things first and decide on the direction that the code takes.

Because of that, after testing 3 and 6, the next test is 13 to help drive out the recognition of the 3 digit.

value.includes has a console error because non-string values are being given to it, so an initial check at the top of the function helps to fix all such future problems.

window.foobarqix = function foobarqix(value) {
    "use strict";
    if (typeof value !== "string") {
        return foobarqix(String(value));
    }
    var str = "";
    if (value % 3 === 0) {
        str = "Foo";
    }
    if (value.includes("3")) {
        str += "Foo";
    }
    if (!str) {
        return value;
    }
    return str;
};

We’re only three tests in, well four if you count the initial 1, and the refactoring process means we already have much better code than we did previously after 7 tests.

There seemed to be three stages: initial if statements, reduced duplication, and extracted to functions.

The poor choice of test order was responsible for those plateaus. When adding tests doesn’t seem to have much effect, it’s time to have a rethink about the tests that you are doing. You are the one in control of the tests, and don’t have to do them counting up in order from 1.

With this kata, testing for 3,6,13 results in immediate benefits in relation to 3.

Using pre-commit before committing code

I want to make sure that my code is clean before being allowed to commit code.
Git has hooks that can be used, such as pre-commit which runs before the commit, but windows has trouble with those.

I found from an articles on Git hooks, practical uses (yes, even on Windows) that it has a section called [Why won’t Git hooks work on windows?] that explains how to get them working on windows.

Give the .git\hooks\pre-commit file the correct path to the shell command at the top of the file, and it all works.

.git\hooks\pre-commit

#!C:/Program\ Files/Git/usr/bin/sh.exe
#
# Called by "git commit" with no arguments.  The hook should
# exit with non-zero status after issuing an appropriate message if
# it wants to stop the commit.

jslint --edition=es6 *.js

I can now be even lazier (more efficient?) when commit the code, with git commit * and it’s all checked before giving a pretty commit screen.

1 Like

Speeding the tests along

The requirements for 5 is taken care of with tests for 5,10,15.

    if (value % 3 === 0) {
        str += "Foo";
    }
    if (value % 5 === 0) {
        str += "Bar";
    }
    if (value.includes("3")) {
        str += "Foo";
    }
    if (value.includes("5")) {
        str += "Bar";
    }

Array? Object? Map?

Refactoring of that code is now easily achieved, to remove the duplication.
It’s easy to see that the Foo and Bar are directly tied to 3 and 5, which means that one of the array/object/map structures should be used:

var arr = [ ["3", "Foo"], ["5", "Bar"] ];
var obj = [ {"3": "Foo"}, {"5": "Bar"} ];
var map = new Map([ ["3", "Foo"], ["5", "Bar"] ]);

I’ll use an array this time around, and see how things go with that.

    var factorNames = [["3", "Foo"], ["5", "Bar"]];
    factorNames.forEach(function ([factor, name]) {
        if (value % factor === 0) {
            str += name;
        }
    });
    factorNames.forEach(function ([factor, name]) {
        if (value.includes(factor)) {
            str += name;
        }
    });

That’s looking pretty good.

Could I have realised sooner that factors.forEach wouldn’t work as well as names.forEach?

Yes I certainly could have. It was in front of me all of the time as one of the tests shown at the FooBarQix kata page. Looking through the tests for other ones that relate to Foo and Bar, there’s numbers 51 and 53, which can be done now.

    expect(foobarqix("51"), "FooBar", "51 is divisible by 3 and contains 5");
    expect(foobarqix("53"), "BarFoo", "53 contains 5 and 3");

51 passes right away, but 53 fails, forcing us to improve the code now instead of later.

The trouble with using an array

With an array we cannot easily find out if one of the factorNames arrays has a matching factor.
It can be done, but it’s messy, and has me already realizing that an object or map would’ve been better instead.

    value.split("").forEach(function (digit) {
        var matchingFactorNames = factorNames.filter(function ([factor, ignore]) {
            return factor === digit;
        });
        if (matchingFactorNames.length > 0) {
            str += matchingFactorNames[0][1];
        }
    });

We can replace .filter with .find

        var matchingFactorName = factorNames
            .find(function ([factor]) {
                return factor === digit;
            });
        if (matchingFactorName) {
            str += matchingFactorName[1];
        }

We can instead use .filter and .map to give us a matching filter name:

        var matchingFactorName = factorNames
            .filter(function ([factor]) {
                return factor === digit;
            }).map(function ([ignore, name]) {
                return name;
            });
        if (matchingFactorName) {
            str += matchingFactorName;
        }

Which has me wanting to move that code out to a separate function:

    function hasMatchingFactor(digit, factorNames) {
        return factorNames.filter(function ([factor]) {
            return factor === digit;
        }).map(function ([ignore, name]) {
            return name;
        });
    }
...
    value.split("").forEach(function (digit) {
        str += hasMatchingFactor(digit, factorNames);
    });

That does hide some of the troubling details away in the hasMatchingFactor() function, but with an array it’s about the best we can get. A map would have certainly made it easier with the Map.keys() and Map.has() methods, and an object can also be tried out the next time too.

1 Like

Changing existing variables was a mistake, because the code had to be rewritten before extraction.

As a part of an if statement there’s little other option but to mutate other variables, but as soon you start to use forEach loops, it’s a bad idea to reach out from inside of them to change things.

Instead of using .forEach, we can use the widely common filter, map, reduce methods instead.

Here I’ve used .join instead of reduce, and have used destructuring in the parameters to help simplify things.

    var str = factorNames.filter(function ([factor]) {
        return value % factor === 0;
    }).map(function ([ignore, name]) {
        return name;
    }).join("");

Doing the same with the digit matching code has me realise that I should be assigning it a variable as well.

    // function hasMatchingFactor(digit, factorNames) {
    function matchingFactor(digit, factorNames) {
    }
...
    var suffix = value.split("").map(function (digit) {
        return matchingFactor(digit, factorNames);
    }).join("");
    if (!str && !suffix) {
        return value;
    }
    return str + suffix;

Testing 7, 14, 17, and 21

I can now add the tests for Qix, which will be easy to make pass.

    expect(foobarqix("7"), "QixQix", "7 is divisible by 7 and contains 7");
    expect(foobarqix("14"), "Qix", "14 is divisible by 7");
    expect(foobarqix("17"), "Qix", "17 contains 7");
    expect(foobarqix("21"), "FooQix", "21 is divisible by 3 and 7");

Adding Qix to the array makes all of those tests pass.

    var factorNames = [["3", "Foo"], ["5", "Bar"], ["7", "Qix"]];

Which brings us to the final stage - asterisks.

1 Like

The asterisks section was quite a struggle and I stopped caring about linting errors in my code.

The main reason behind why I stopped caring though, is that I was working too hard at the problem.

What I should have done when I had trouble with the asterisks, was to do the simplest thing to make it pass.
That way I can make it all pass, and then refactor the code to better understand things, and improve its structure.

Instead, I wasted a lot of brain power trying many different things that all failed to pass the test. Get it passing first, and then from a working system you can increase your understanding of what’s going on.

Also now, that I’ve use a pre-commit script to check JSLint, I’m not allowed to commit any code until every linting problem is taken care of.

Here’s the 101 test:

   expect(foobarqix("101"), "1*1", "101 contains a 0 which converts to *");
}

and the simplest way to make it pass is with an if statement:

    if (value === "101") {
        return "1*1";
    }
    if (!str && !suffix) {
        return value;
    }

I can then convert any zeros in the value.

    if (value === "101") {
        return value.replace("0", "*");
    }

which is much easier than what I was trying to do before.

The next test for 303 helps to force us to consider what’s happening with digit matching factor section of code.

    expect(foobarqix("303"), "FooFoo*Foo", "303 is divisible by 3, contains two 3's and the 0 converts to *");

The simplest thing to do here is to add a check for the zero digit:

    var suffix = value.split("").map(function (digit) {
        if (digit === "0") {
            return "*";
        }
        return matchingFactor(digit, factorNames);
    }).join("");

and this code makes the 10 test appropriately fail, so we need to update that test to add in the asterisk that should be there now.

    expect(foobarqix("10"), "Bar*", "10 is divisible by 5 and the 0 converts to *");

And the test passes. I can now carry on with refactoring without having to try and figure out all of the details on how to make the test pass, because all of the tests are all ready passing.

The asterisks could’ve been done before namedDigits() function, it shouldn’t need to know about them.

Previously I was exhausted after trying ti use good coding techniques to make the tests pass, because I had to try many different types of techniques and attempt to understand how they affected the code and which tests failed.

Now that I’ve used a simple technique to get the tests passing, I have much more brain activity available to think about how to improve the code.

Both the loop and the if condition do asterisk conversion, so moving that conversion before both of them will help toi simplify things.

    value = value.replace("0", "*");
    var suffix = value.split("").map(function (digit) {
        if (digit === "*") {
            return "*";
        }
        return matchingFactor(digit, factorNames);
    }).join("");

    if (value === "1*1") {
        return value;
    }
    if (!str && !suffix) {
        return value;
    }

The map shouldn’t care if it’s an asterisk or not, so I can have it only change digits instead. They’re not really digits either.

    var suffix = value.split("").map(function (digit) {
        if (Number(digit)) {
            return matchingFactor(digit, factorNames);
        }
        return digit;
    }).join("");

With the 1*1 part of the code, I want to make the if condition as close as I can to the other if condition, which will help me to understand what the differences are between them.

    if (!str && suffix === "*") {
        return value;
    }
    if (!str && !suffix) {
        return value;
    }

I’ve now saved myself two hours of failed experimentation and wasting time. I can immediately tell that by removing asterisks from the suffix, I can use the one if condition.

    if (!str && !suffix.replace("*", "")) {
        return value;
    }

Now I am adding asterisks further up in the code, and removing them again here, so what can I do to prevent that from happening? I can check if the value has any of the factor digits instead. I already have a matchingFactor() function and it would be nice if I could use that again.

It would help if the matchingFactor() function only had one parameter, so I need a separate iterator function that takes the factorNames array and can be used by only giving values to it.

    function matchingFactorIterator(factorNames) {
        return function (value) {
            return matchingFactor(value, factorNames);
        };
    }

I can add the asterisk to the factor names, and remove the need for the map to check about digits

    var factorNames = [
        ["3", "Foo"], ["5", "Bar"], ["7", "Qix"], ["0", "*"]
    ];
...
    var suffix = value.split("").map(function (digit) {
        return matchingFactor(digit, factorNames);
    }).join("");
    value = value.replace("0", "*");

I can now use matchingFactorIterator with the map instead:

    var matchingFactorIter = matchingFactorIterator(factorNames);
    var suffix = value.split("")
        .map(matchingFactorIter)
        .join("");

And I can now use that matchingFactorIter() function. Well, not quite yet. When I try to use it I get poor results until I fix a bug with it. After the map I wasn’t joining the results together, which is done now.

    function matchingFactor(digit, factorNames) {
        return factorNames.filter(function ([factor]) {
            return factor === digit;
        }).map(function ([ignore, name]) {
            return name;
        }).join("");
    }

That bug was only found by using the debugger to step very slowly through the code, and would have been caught if I had remembered to keep in mind what the expected ins and outs were. String in, string out.

    var valueStar = value.replace("0", "*");
    if (!str && !valueStar.split("").find(matchingFactorIter)) {
        return valueStar;
    }

Using the matching matchingFactorIter() function we end up with this:

var valueStar = value.replace("0", "*");
if (!str && !valueStar.split("").find(matchingFactorIter)) {
    return valueStar;
}
return str + suffix;

Those exclamation marks can be inverted, from (!a && !b) to (a || b)

```javascript
    if (str || valueStar.split("").find(matchingFactorIter)) {
        return str + suffix;
    }
    return valueStar;

and we have much improved working code.

The 105 and 10101 tests complete things:

    expect(foobarqix("105"), "FooBarQix*Bar", "105 is divisible by 3 and 5, contains 5 and the 0 converts to *");
    expect(foobarqix("10101"), "FooQix**", "10101 is divisible by 3 and 7, and the 0's converts to *'s");

and we have now completed the kata, and confirmed the benefits of what we learned from the previous one.

The code for what has been done is found at https://github.com/pmw57/Kata/tree/a9b1e0f1fbd15bc60e954dcc278d12a73e396dd6

1 Like

This topic was automatically closed 91 days after the last reply. New replies are no longer allowed.