More on Request-Response. Maybe some little objects?

I’d really like to get to providing more actual game capability here, for two good reasons. First, it’ll be more fun. Second, I think it’s of utmost importance in real product development to always be showing visible progress that actual humans can understand. Kent Beck once said that all software methods are based on fear, and my fear is that my sponsors, seeing too little progress, will cancel the development—or cancel me. I hate when that happens.

However, the current focus on the robot-world request-response linkage is also important, and my sponsors want to be sure that we will be able to accomplish that necessary aspect in good order. And, more important to me, the current code is limited, weak, and unattractive, and I think we’ll find some really nice little lessons in the request-response area.

Exceptions

We’ve already seen something that I’ve never done before in Codea Lua—to my best recollection. We have the main operation of the server protected from exceptions. If the server hurls during a request, the system recovers and sends a SYSTEM ERROR response packet to the robot who was involved.

The code to do this was nearly trivial:

function World:request(rq)
    ok, result = pcall(function() return self:requestProtected(rq) end)
    if ok then
        return result
    else
        return self:error("SERVER ERROR "..tostring(result))
    end
end

The Lua pcall calls a provided function in “protected mode”, returning either true, followed by the returns from the call, or false, followed by the error, typically a string. This is all roughly equivalent to the try/catch with which you are probably familiar.

So that’s nice.

The Spec is … Less Than Ideal

Both the request and response are specified for us, as well as all the commands we have to handle. The request and response are simple objects that can be translated to and from JSON, and amount to nothing more than some named values, named objects, and named arrays of values. Objects like that are quite error-prone. For example, what’s wrong with this response object?

{
  ...
  state: {
    positon: [20,30],
    direction: "N",
    hits: 5,
    shots: 6,
    status: "Normal"
  }
}

That object would compile just fine, turn into JSON just fine, turn back into a dictionary just fine … and it’s seriously wrong.

The word “position” is misspelled; the correct field name is “shields”, not “hits”; the correct status is “NORMAL”, not “Normal”. And those are the errors I know of.

Well, no matter what the spec says, we need to be as sure as we can be that the request and response are valid. Based on some attempts to validate these things, I’m distressed by how tedious and error-prone it is:

function World:isValidResponse(r)
    if not r then return false end
    if r.result ~= "OK" and r.result ~="ERROR" then return false end
    if not r.data then return false end
    return true
end

function State:validatePosition()
    local position = self._dict.position
    return position and type(position)=="table" and #position==2
end

Neither of these is complete, and they won’t get better. If I had to bet, I would bet that due to the press of time, many teams would wind up with very weak validation of their input and output, with the result that there would be defects in the game, some of them likely being released to the unsuspecting robot users.

We’ll see what can be done about this issue as we go forward.

Request-Response Has Some Value

I conclude that there is some value to working a bit more on the request-response cycle, but I remain quite anxious about adding features as soon as possible. Perhaps we can find a balance and do a bit of both.

Let’s get started.

Review

It was almost 24 hours ago that I last worked on this program, so I have no idea where we stand. Let’s see what commits we had. Most recent first, we have these:

  1. Request uses pcall to handle hurls. Refactoring to ensure valid response.
  2. unknown command returns ERROR response.
  3. Robot launch is done in WorldProxy as a request-response.
  4. Initial World:request function supports launch.
  5. WorldProxy:launchRobot forms a request, does not use it.

OK, this gives me two tasks to help me decide what to do. First, I want to review the launch sequence to see whether it’s done enough for now. Second, I then want to look at the other requests that we can handle, forward and scan, to see which of those we might want to move into the request-response style.

Into the Code

Here’s the Proxy side of launching:

function WorldProxy:launchRobot(name,robot)
    local kind = "RonRobot1"
    local shields = 99
    local shots = 99
    local request = {
        robot=name,
        command="launch",
        arguments={kind,shields,shots}
    }
    local response = self.world:request(request)
    return response.state
end

Already we see things we should be concerned about. In particular, this:

        arguments={kind,shields,shots}

Is the correct order kind, shields, shots? In a real situation, we have only the spec to tell us. Here we can also check the code, because unlike most people building robot clients, we are also writing the server. In a real situation, we’d probably have the spec and that’s it.

In addition, our request is just a dictionary of key-value pairs. It has no constructor, no helper methods.

However, what we see here does form a request, and does expect a response back, and then returns the part of the response that the robot currently wants … to be handled here:

function Robot:init(name,aWorld)
    assert(aWorld:is_a(WorldProxy), "expected WorldProxy")
    self._world = aWorld
    self._state = State(aWorld:launchRobot(name, self))
    self._x, self._y = self._state:position()
    self._name = name
    self.knowledge = Knowledge()    
end

This is pretty flimsy, and pulling the x and y out of the state is particularly risky. If the state ever changes, and it will, we’ll be using the wrong x and y.

We’ve done one decent thing, which is that we’re using a class State to contain the state. Whether we should create it here, on the return, or in the Proxy … I’m not certain yet. Either way, we should refer to our state to get our x and y. So let’s search in Robot for use of _x and _y and fix them.

A quick check tells us that we do have the accessors in Robot:

function Robot:x()
    return self._state:x()
end

function Robot:y()
    return self._state:y()
end

We just need to be sure that we use them everywhere. Search. I find one test referring to robot._y, and the saving of the values in the init as shown above. Fix the test, remove the save. Test. Good. Commit: remove member vars _x,_y from Robot, refer to _state.

Cruising

We’re still surveying the code, refreshing ourselves on where things are. I think that’s worth thinking about:

If request and response were all nicely tied up, there’d be no need to be digging around in the code to see how far along various commands are: they’ll all work, or not work. As things stand, we’re in the process of moving from an open-coded style to a more object-oriented style, and because we don’t have a good design yet, we have to look all around to see what’s up. This is time-consuming and error prone. This is why it pays off to keep the code clear, clean, and well-factored.

Who else accesses _state? Well lookee here:

function Robot:forward(steps)
    self._state = self._world:forward(self._name, steps)
    self.knowledge = self.knowledge:newLensAt(self:x(),self:y())
end

Two things of note. First, there’s code that clearly is supposed to change x and y, and does it correctly, calling the methods. We did that yesterday. Good on us. I wish we had mentioned it in a commit.

Second, the proxy (world) is clearly returning a State instance here, whereas in the code we just looked at, the robot did the conversion. We need to decide on a way.

Here’s a tentative decision:

The WorldProxy will return an instance of Response, yet to be created, and that object will include an instance of State, or will return one via an accessor, or … will offer methods so that the Robot doesn’t need to know anything about the Response’s structure.

Well, when you put it that way … it’s pretty obvious that we should wrap that dictionary stuff as a Response and the Response object should provide all the necessary accessors to keep the Robot happy. I’m glad we had this little design session.

And that gives us something to do: start building the Response object. We can start with a very simple one, but we’ll certainly want a test framework for it

Response Object

-- TestResponse
-- RJ 20220701

function test_Response()
    
    _:describe("Response", function()
        CodeaUnit_Detailed = false
        
        _:before(function()
        end)
        
        _:after(function()
        end)
        
        _:test("Response", function()
            _:expect(2).is(2)
        end)
        
    end)
end

Test should run, 2 being a very good approximation for 2. And it does. Let’s write a real test:

        _:test("Response Exists", function()
            local resp = Response()
        end)

This will drive out the class:

1: Response -- TestResponse:16: attempt to call a nil value (global 'Response')

To which we respond:

Response = class()

Green. Commit: initial response test and class. Swell. Not very exciting, though. Let’s create a somewhat more real test.

        _:test("Response has key elements", function()
            local dict = {
                result="OK",
                data={},
                state={
                    position={3,2},
                }
            }
            local resp = Response(dict)
            _:expect(resp:result()).is("OK")
            _:expect(resp:x()).is(3)
            _:expect(resp:y()).is(2)
        end)

I chose to check result just for fun and x and y because we’ll need them in just a moment. The test will drive out some methods:

2: Response has key elements -- TestResponse:28: attempt to call a nil value (method 'result')

To which:

function Response:init(responseDictionary)
    self._r = responseDictionary
end

function Response:result()
    return self._r.result
end

Leads us by the nose to:

2: Response has key elements -- TestResponse:29: attempt to call a nil value (method 'x')

I would normally do y also but this:

function Response:x()
    return self._r.state.position[1]
end

Leads to this:

2: Response has key elements -- TestResponse:30: attempt to call a nil value (method 'y')

Leads to:

function Response:y()
    return self._r.state.position[2]
end

So that’s nice. We’re green. Commit: Response has result(), x() and y().

Installing Response

I’d like to install the response now. I expect a little trouble but with any luck not much.

Since State also understands x() and y(), I can probably do this incrementally. If not, a few tests will break and we’ll just fix them.

I’m going to change the member varialble Robot._state to _response:

function Robot:init(name,aWorld)
    assert(aWorld:is_a(WorldProxy), "expected WorldProxy")
    self._world = aWorld
    self._response = aWorld:launchRobot(name, self)
    self._name = name
    self.knowledge = Knowledge()    
end

function Robot:x()
    return self._response:x()
end

function Robot:y()
    return self._response:y()
end

function Robot:forward(steps)
    self._response = self._world:forward(self._name, steps)
    self.knowledge = self.knowledge:newLensAt(self:x(),self:y())
end

Those are fine. But I also find this:

function Robot:setPosition(x,y)
    self._state:setPosition(x,y)
end

Is that used? I find no uses of it. Delete it, see what happens. I expect some errors because no one is returning a Response or State in one Case, and a State in the other. Test.

5: Robot moves forward on 1 key -- TestRobot:151: attempt to call a nil value (method 'y')

That test is this:

        _:test("Robot moves forward on 1 key", function()
            local world = WorldProxy()
            world:addFactAt("O",3,1)
            local robot = Robot("Louie", world)
            _:expect(robot:y()).is(0)
            robot:scan()
            _:expect(robot:factAt(3,0)).is(nil)
            robot:keyboard("1")
            _:expect(robot:y()).is(1)
            robot:scan()
            _:expect(robot:factAt(3,0)).is("O")
        end)

We’ll see what forward does. Probably not returning a Response.

function Robot:forward(steps)
    self._response = self._world:forward(self._name, steps)
    self.knowledge = self.knowledge:newLensAt(self:x(),self:y())
end

That leads here:

function WorldProxy:forward(name,steps)
    local result
    local rq = {
        robot=name,
        command="forward",
        arguments={steps}
    }
    local jsonRq = json.encode(rq)
    local decoded = json.decode(jsonRq)
    local dName = decoded.robot
    local dCommand = decoded.command
    if dCommand == "forward" then
        local dSteps = decoded.arguments[1]
        local responseDict = self.world:forward(dName,dSteps)
        return State(responseDict.state)
    end
    assert(false,"impossible command "..dCommand)
end

I kind of expected that to work, even wrong, but we’ll make it right:

    if dCommand == "forward" then
        local dSteps = decoded.arguments[1]
        local responseDict = self.world:forward(dName,dSteps)
        return Response(responseDict.state)
    end

Test again.

1: Robot updates knowledge on move -- TestResponse:47: attempt to index a nil value (field 'state')
5: Robot moves forward on 1 key -- TestRobot:151: attempt to call a nil value (method 'y')
7: Robot ignores bad packet direction -- TestResponse:47: attempt to index a nil value (field 'state')

We’d best have a glance at the World.

function World:forward(robotName,steps)
    local robot = self._robots[robotName]
    robot._y = robot._y + steps
    local response = {
        result = "OK",
        data = {},
        state = self:robotState(robot)
    }
    return response
end

That actually looks OK. I could revert, or figure this out. Reverting is probably right, but the big fool decides to press on a bit.

I wasn’t creating the Response correctly. Should be:

    if dCommand == "forward" then
        local dSteps = decoded.arguments[1]
        local responseDict = self.world:forward(dName,dSteps)
        local resp = Response(responseDict)
        return resp
    end

Test.

5: Robot moves forward on 1 key -- TestRobot:151: attempt to call a nil value (method 'y')

I want to know which of the calls to y this was. Improve the test.

        _:test("Robot moves forward on 1 key", function()
            local world = WorldProxy()
            world:addFactAt("O",3,1)
            local robot = Robot("Louie", world)
            _:expect(robot:y(),"initial").is(0)
            robot:scan()
            _:expect(robot:factAt(3,0)).is(nil)
            robot:keyboard("1")
            _:expect(robot:y(), "post").is(1)
            robot:scan()
            _:expect(robot:factAt(3,0)).is("O")
        end)

This may not be enough to help me out. Test. Right, Can’t tell which one it is. Where’s 151?

function Robot:y()
    return self._response:y()
end

A quick print tells me that I have a table in _response instead of a Response instance. Best make the other returns do the right thing, Who’s setting it?

function Robot:init(name,aWorld)
    assert(aWorld:is_a(WorldProxy), "expected WorldProxy")
    self._world = aWorld
    self._response = aWorld:launchRobot(name, self)
    self._name = name
    self.knowledge = Knowledge()    
end

function Robot:forward(steps)
    self._response = self._world:forward(self._name, steps)
    self.knowledge = self.knowledge:newLensAt(self:x(),self:y())
end

OK, I guess it has to be the launch?

function WorldProxy:launchRobot(name,robot)
    local kind = "RonRobot1"
    local shields = 99
    local shots = 99
    local request = {
        robot=name,
        command="launch",
        arguments={kind,shields,shots}
    }
    local response = self.world:request(request)
    return response.state
end

I really thought returning a state would pass the tests. But since we’re here we’ll fix it:

function WorldProxy:launchRobot(name,robot)
    local kind = "RonRobot1"
    local shields = 99
    local shots = 99
    local request = {
        robot=name,
        command="launch",
        arguments={kind,shields,shots}
    }
    local responseDict = self.world:request(request)
    return Response(responseDict)
end

Is this going to make the tests run? I am hopeful but not certain. It does. Whew.

A bit of reflection, please.

Reflect

The changes here seemed trivial and yet my understanding was somehow wrong. Oh … I think it’s because no one was casting the state in response to a type. OK, I feel like I know what happened.

However, I could probably have found smaller steps to take and saved myself some confusion and some time. Reverting was almost certainly better than debugging.

But we’re here now. Have we learned anything?

Yes, I think we have learned something good and something bad.

Something good: we now have a Response object and it is already serving to normalize access rather than having code all over going self._response._this_or_that.

Something bad: it is possible that the Proxy will forget to cast the response dictionary from the server as a Response. We’ll want to deal with that by ensuring that there’s just one place where the response comes back, and casting it there. But we might also want to go belt and suspenders by verifying in Robot.

Let’s do that.

Ensuring Response

We refactor this code:

function Robot:init(name,aWorld)
    assert(aWorld:is_a(WorldProxy), "expected WorldProxy")
    self._world = aWorld
    self._response = aWorld:launchRobot(name, self)
    self._name = name
    self.knowledge = Knowledge()    
end

To this:

function Robot:init(name,aWorld)
    assert(aWorld:is_a(WorldProxy), "expected WorldProxy")
    self._world = aWorld
    self:setResponse(aWorld:launchRobot(name, self))
    self._name = name
    self.knowledge = Knowledge()    
end

function Robot:setResponse(resp)
    self._response = resp
end

Test. Green. Commit? OK: implement setResponse method.

Find all setters of _response and change to call setResponse:

function Robot:forward(steps)
    self:setResponse(self._world:forward(self._name, steps))
    self.knowledge = self.knowledge:newLensAt(self:x(),self:y())
end

Test. Green. Commit: all response setters use setResponse method.

Now enhance the method to check:

function Robot:setResponse(resp)
    assert(resp:is_a(Response), "invalid response")
    self._response = resp
end

Green. Commit: setResponse asserts that we have a Response instance.

Let’s reflect again.

Reflection

We’re now doing a better job of packaging up the World’s responses, but we haven’t handled all the cases in hand. In particular, the scan function (which should be called look, by the way), doesn’t use the response object at all.

We’re still not really going through the protocol in a standardized fashion, with no duplication. In World we have this:

function World:request(rq)
    ok, result = pcall(function() return self:requestProtected(rq) end)
    if ok then
        return result
    else
        return self:error("SERVER ERROR "..tostring(result))
    end
end

function World:requestProtected(rq)
    local c = rq.command
    local args = rq.arguments
    local response
    if c == "noResponse" then
        -- nothing
    elseif c == "launch" then
        response = self:launchRobot(rq.robot, args[1], args[2], args[3])
    else
        response = self:error("No such command "..tostring(c))
    end
    if self:isValidResponse(response) then
        return response
    else
        return self:error("SERVER RETURNED NO RESPONSE TO "..tostring(c))
    end
end

Clearly what we intend here is that callers (WorldProxy) are supposed to create a request and call (via all kinds of rigmarole) the World:request function.

We do that here:

function WorldProxy:launchRobot(name,robot)
    local kind = "RonRobot1"
    local shields = 99
    local shots = 99
    local request = {
        robot=name,
        command="launch",
        arguments={kind,shields,shots}
    }
    local responseDict = self.world:request(request)
    return Response(responseDict)
end

We also have this sketched flow in the proxy:

function WorldProxy:forward(name,steps)
    local result
    local rq = {
        robot=name,
        command="forward",
        arguments={steps}
    }
    local jsonRq = json.encode(rq)
    local decoded = json.decode(jsonRq)
    local dName = decoded.robot
    local dCommand = decoded.command
    if dCommand == "forward" then
        local dSteps = decoded.arguments[1]
        local responseDict = self.world:forward(dName,dSteps)
        local resp = Response(responseDict)
        return resp
    end
    assert(false,"impossible command "..dCommand)
end

We can, I think, change that to use request. Let’s try it and see if we get some nice duplication to think about.

Change forward to use request

function WorldProxy:forward(name,steps)
    local result
    local rq = {
        robot=name,
        command="forward",
        arguments={steps}
    }
    local jsonRq = json.encode(rq)
    local decoded = json.decode(jsonRq)
    local dName = decoded.robot
    local dCommand = decoded.command
    if dCommand == "forward" then
        local dSteps = decoded.arguments[1]
        local responseDict = self.world:request(decoded)
        local resp = Response(responseDict)
        return resp
    end
    assert(false,"impossible command "..dCommand)
end

This breaks things, but I’m not surprised. Failures are:

1: Robot updates knowledge on move -- TestResponse:51: attempt to index a nil value (field 'state')
5: Robot moves forward on 1 key -- TestResponse:51: attempt to index a nil value (field 'state')
7: Robot ignores bad packet direction -- TestResponse:51: attempt to index a nil value (field 'state')

I think we don’t cope with the command forward in the World. Let’s check:

function World:requestProtected(rq)
    local c = rq.command
    local args = rq.arguments
    local response
    if c == "noResponse" then
        -- nothing
    elseif c == "launch" then
        response = self:launchRobot(rq.robot, args[1], args[2], args[3])
    else
        response = self:error("No such command "..tostring(c))
    end
    if self:isValidResponse(response) then
        return response
    else
        return self:error("SERVER RETURNED NO RESPONSE TO "..tostring(c))
    end
end

I’m pretty sure we returned an error. Let’s put a print in our setResponse, just for fun.

function Robot:setResponse(resp)
    assert(resp:is_a(Response), "invalid response")
    if resp:result() ~= "OK" then
        print("Bad Result :", resp:result(), resp:message())
    end
    self._response = resp
end

And we need this:

function Response:message()
    if not self._r.data then
        return "no data element"
    elseif not self._r.data.message then
            return "no data.message"
    else
        return self._r.data.message
    end
end

Slightly messy but we have to cope with what might happen. Test, expecting better messages. And sure enough”

Bad Result :	ERROR	No such command forward

That repeats for each test that fails. Yummy.

Implement:

function World:requestProtected(rq)
    local c = rq.command
    local args = rq.arguments
    local response
    if c == "noResponse" then
        -- nothing
    elseif c == "launch" then
        response = self:launchRobot(rq.robot, args[1], args[2], args[3])
    elseif c == "forward" then
        response = self:forward(rq.robot, args[1])
    else
        response = self:error("No such command "..tostring(c))
    end
    if self:isValidResponse(response) then
        return response
    else
        return self:error("SERVER RETURNED NO RESPONSE TO "..tostring(c))
    end
end

We are green. Works a treat. Hate the args[123] stuff, but that’s OK for now.

Commit: Command forward uses World:request.

It’s 1158, let’s sum up.

Summary

We’ve made really decent progress today. We have a commit about every 15 minutes (and missed out one that we should have done, did you notice?). We moved local Robot references to refer to the State package, and then to the Response package, without changing the way that Robot thinks of things. It’s still written, for better or worse, the way I wanted it to work, and its references to values it needs are cleanly forwarded to its Response member variable.

We now have launch and forward issuing request dictionaries (which should become objects, I think), and receiving Responses. Those methods are separate in the proxy, and are beginning to show some duplication.

I think we may now have no references to the State class. A quick test agrees. Remove that tab. Test. Green. Commit: Remove unused State class.

I think we’ve made good progress toward normalizing request-response, with work yet to be done, but things closer to what we need. Perhaps tomorrow we might implement “backward” or something easy like that, to give us a new feature, and to help us focus on getting the request-response to a more cohesive implementation.

There is a larger lesson here, and it’s about incremental development. We often worry that if we don’t get the initial design just right, we’ll be doomed forever. This is not usually the case. As we’ve seen here, over a few days, we can move from one scheme, robot and world in direct call-return communication, to another very different one where they are use request-response communication, in a series of small steps that don’t break anything.

I believe that it’s always possible, for very large values of always, to adjust a design incrementally. It’s part of what gives me confidence to begin working on features early, which I very much want to do, because “working software is the primary measure of progress”, as someone once said, and I really want to show progress. It keeps the customer satisfied.

See you next time!