Ignatz and Jmv38 on the Codea forums commented on the previous article. I had hoped to do more anyway so here's the next one.

Note that this article is different from what it might have been, because I've had conversations with other programmers looking at (working with) the code. This isn't bad, it's good. It's like pair programming without all the travel. Be aware, though, that many of the "discoveries" in this article have already been discovered. There's a long note on the forum replying to their comments. This article will follow that note's line of reasoning, but I plan to rewrite it rather than copy and edit.

Because that's what I do.

GUI

Ignatz and Jmv38 talked about why I hadn't built the GUI and whether I should. I plan to do so, but this isn't that article.

It's worth mentioning here that I find GUIs hard to test, so I like to write really solid "model" objects, test the heck out of them, and then write thin GUIs where "nothing can go wrong". Or at least very little.

Even so, some testing is probably needed, and I think we'll discover that in the GUI article when I get to it.

Duplication

There is significant duplication in the calculator as it stands. Take a look:

    function Calculator:press(key)
        if key == "*" then
            self:doPending(function(d,t) return d*t end)
        elseif key == "/" then
            self:doPending(function(d,t) return t/d end)
        elseif key == "-" then
            self:doPending(function(d,t) return t-d end)
        elseif key == "+" then
            self:doPending(function(d,t) return t+d end)
        elseif key == "=" then
            self:doPending(nil)
        else
            if self.first then
                self.temp = self.display
                self.display = ""
                self.first = false
            end
            self.display = self.display..key
        end
    end

There are four or five if statements, checking a key and then doing a do-Pending. And inside each doPending there is a function definition that looks exactly the same except for the operation done inside. (I note that they all are in t then d order except the first one. That's because I did a non-commutative operator second and t has to be first. I should change the first one to match, and I will. (I just did, and the tests all run.))

We'd like to remove this duplication. Perhaps I should stop and say why.

Why we remove duplication

I've mentioned some of the concerns already. If there is duplication in the code, that tells us that there is some idea, either in our mind or emerging in the code, that is not well-represented in the code. Here, the idea is "hey, all these operators are alike".

If there is duplication and changes are needed, we have to make the changes in many places. This is tedious and error-prone.

Here's a thing I haven't mentioned in this series yet. Long ago, Kent Beck (creator of Extreme Programming) listed the elements of "simple code". His list has taken a few forms but the one I prefer is this one:

Simple code:

  1. Runs all the tests.
  2. Contains no duplication.
  3. Expresses all the programmer's ideas.
  4. Minimizes the number of programming entities: lines, classes, files, etc.

These items are in priority order.

Now if you think about these, you might think "But wait, expressing ideas might require duplication. Numbers 2 and 3 should be reversed." Well, you might be right. If there were ever a conflict. But I prefer them in this order, because more often than not, duplication to "express an idea" really means that the code contains an idea that I have not as yet recognized. So I like to feel the conflict.

The list does come in the other order and in other forms. Find one you like and use it. I like this one.

Anyway, we've got this code: what shall we do about it?

Issues

I see two issues with those ifs. First of all, they are clearly very redundant. The duplication should be removed. But they are also a bit unclear: those embedded functions inside the doPending inside the then make the code tricky to read, especially since we don't expect to see functions written in line like that. At least I don't.

Both the following ideas come from Jmv38. We could break the functions out like this:

function Calculator.mult(d,t) return t*d end
function Calculator.divide(d,t) return t/d end
function Calculator.minus(d,t) return t-d end
function Calculator.add(d,t) return d+t end

Then we'd say, for example:

if key == "*" then
    self:doPending(self.mult)

and so on. That might be a bit more clear but still leaves us with duplication.

Maybe we would look at this and realize that there is a one-to-one relationship between the character pressed and the function to be executed. (And the function has that character in the middle of it, but I can't figure a way in Codea to just do that. Unless I compile on the fly. No. Just no.)

So Jmv38 suggests that we build a local table, indexed by the key pressed:

local operations = {}
operations["*"] = function(d,t) return t*d end
operations["+"] = function(d,t) return t+d end
operations["/"] = function(d,t) return t/d end
operations["-"] = function(d,t) return t-d end

Now as it happens, I didn't realize that one could define a local table that way. It's "obvious" when you think about how Codea works but it wasn't obvious to me. This is why pair programming is so powerful. The other person knows stuff you don't know and thinks of stuff you don't think of.

We're left with the question of how to access the table. I think I prefer this:

function Calculator:press(key)
    if key=="*" or key=="+" or key=="/" or key=="-" then
        self:doPending(operations[key])
    elseif key == "=" then
        self:doPending(nil)
    else
        if self.first then
            self.temp = self.display
            self.display = ""
            self.first = false
        end
        self.display = self.display..key
    end
end

Are you starting to wish these things were in order plus minus times divide? I am. Something in my brain wants them to be that way. I'll fix that because what good is having a touch of OCD if you don't give in to it once in a while?

Done, and the tests still run. I feel better now.

Now Jmv38 suggests going a step further with

local f = operations[key]
if f then
    doPending(f)
elseif key == "="
    ...

Nothing wrong with that, probably. But it does require us to look back and say "When could f be nil? Oh, if key isn't one of those operators. Hmm ..."

I don't like things that make me go "Hmmm". So I prefer the or. However, there are a lot of binary operators in the world, and that if could get pretty long. Jmv38's approach extends to new operators smoothly. So it could be better. I prefer it more explicit. YMMV.

Summing up ...

First of all many props to Ignatz and Jmv38 for their ideas and questions. We wouldn't be here without them.

I'll show the relevant code one more time below. First, the discussion. We saw more duplication, and we thought of ways to remove it. We also saw some code that didn't express itself well. We considered ways of fixing it but (with help) we saw an approach that seemed to promise help on both dimensions. We tried it, we liked it.

But those tests!! Without those tests, I'd have been scared as hell about making a major change like a table of functions and removing all my if statements. It would be too easy to screw it up.

The tests give us confidence that when we screw up, we'll find out before anyone else does. That's a very good thing.

Next time, the GUI. Might be soon, might not. I've got a gig next week.

Current Code

--# Calculator
Calculator = class()

function Calculator:init()
    self.display = ""
    self.op = nil
    self.first = false
end

local operations = {}
operations["+"] = function(d,t) return t+d end
operations["-"] = function(d,t) return t-d end
operations["*"] = function(d,t) return t*d end
operations["/"] = function(d,t) return t/d end

function Calculator:press(key)
    if key=="+" or key=="-" or key=="*" or key=="/" then
        self:doPending(operations[key])
    elseif key == "=" then
        self:doPending(nil)
    else
        if self.first then
            self.temp = self.display
            self.display = ""
            self.first = false
        end
        self.display = self.display..key
    end
end

function Calculator:check(key, result)
    self:press(key)
    self:displayIs(result)
end

function Calculator:doPending(func)
    if self.op ~= nil then
        self.display = "" .. (self.op(self.display, self.temp))
    end
    self.first = true
    self.op = func
end

function Calculator:displayIs(string)
    if self.display ~= string then
        local diag = "expected /"..string.."/ but got /"..self.display.."/!"
        print(diag)
    end
end

function Calculator:draw()
end

function Calculator:touched(touch)
end

--# Main
-- Article Calc

-- Use this function to perform your initial setup
function setup()
    print("tests begin")
    local c = Calculator()
    c:check("1","1")
    c:check("2","12")
    c:check("*","12")
    c:check("3","3")
    c:check("=","36")

    c = Calculator()
    c:check("4","4")
    c:check("5","45")
    c:check("0","450")
    c:check("/","450")
    c:check("1","1")
    c:check("5","15")
    c:check("-","30")
    c:check("1","1")
    c:check("9","19")
    c:check("=","11")
    c:check("+","11")
    c:check("5","5")
    c:check("=","16")
    print("tests end")
end

-- This function gets called once every frame
function draw()
    background(40, 40, 50)
end