I actively consider what I think ought to go into a controller

G'day:
This is one of these ones where I state the obvious (probably), and expose myself as a fraud in that it's taken me this long to reach this conclusion. I can only take solace in that I'm clearly in "good" company. Where "good" == "a lot".

Yesterday I was going a code review, and came across a controller method which made one of my eyebrows raise as it was about 40 lines long. Now this isn't intrinsically bad in context. I have seen (and still see!) controller methods that are hundreds and hundreds of lines long. In the past I've seen controllers tipping the 1000-line mark. Those examples are clearly just wrong. And the developers who wrote them and/or continue to maintain them should be shot. I'm not even sure I'm over-stating it with that sentence. Hmmm... I probably am I guess. But they should be squarely and actively admonished, anyhow.

So in the scheme of things, 40 lines ain't bad. Especially when we try to keep our code wrapped within 80-100chars, so a few of the statements were multi-line. There was about say a dozen statements in it altogether. Not so bad.

Before I go on - and not only because the dev concerned will be reading this article (once I send them the link... pretty sure they don't know this blog exists) - I hasten to add that this code is not by any means bad. I'm probably gonna go "close enough" on the code review, once one bit of the code is refactored at least into a private method. I am not having a go at the dev concerned here. They are just my muse for the day. And I happen to think they're pretty good at their job.

There was one bit of the controller that stood out to me and I went "nuh-uh, that's gotta go". There was a foreach loop just before the response was returned which performed a calculation. That's business logic and it has no place in a controller. Fortunately we have an appropriate service class which we can lift that out and re-home in, and slap some tests on it, and our code is a bit better and a bit more maintainable.

(NB: yeah... no tests... we have a policy of not testing controller methods, but that's with the caveat there's no logic in them. We followed the "don't test controller methods" bit, but not the "provided there's no logic in them" bit here).

However once I spot a problem, I immediately start looking for more, so I assessed the rest of the code there. The controller did seem too fat. I wanted to know if all the logic did actually belong in the controller, and my instinct was: probably not.

At the same time as writing this article I'm chatting on Slack about it, and me mate Mingo offered the perennial answer to the question "should this go in the controller?"

Isn't the answer always "No"?

~ mjhagen 2017

(yes, he cited the attribution too, so I'm including that ;-)

It's pleasingly pithy, and almost ubiquitously agreed with; but in a casually dismissive way that is seldom heeded, I think. The problem is it's alluding to a mindset one should adopt as a guideline, but clearly the answer isn't "no", so I think people dismiss it. If one says "one should never do x", and it's clear that "never" is slight hyperbole, people will acknowledge the hyperbole and stretch it back the other way as far as they see fit to accommodate their current whim.

With this in mind (retroactively... Mingo just mentioned that now, and the code review was yesterday, and this article was drafted in my head last night), I stopped to think about a rule of thumb as to when something ought to go in a controller.

Broadly speaking I feel there are only three types of logic that belong in a controller:

1. a call to validate / sanitise all incoming arguments so they're fit for purpose for fulfilling the request;
2. one (preferably) or more (and increasingly less preferably the more there are) calls to get data - using those incoming arguments - to build the response;
3. a call to assemble that data for the response;
4. returning the response.

Yeah that's 4. I was not counting the return statement. An example of this might be:

class UserController {
    function handleGet(rawArgs){
        args = validationService.validate(rawArgs)
        
        user = userService.get(args.id)
        publishedContent = contentService.getForId(args.id, args.startDate, args.endDate)
        twitterContent = twitterService.getForId(args.id, args.startDate, args.endDate)
        facebookContent = facebookService.getForId(args.id, args.startDate, args.endDate)
        
        response = {
            user = user,
            publishedContent = publishedContent,
            socialContent = {
                twitter = twitterContent,
                facebook = facebookContent
            }
        }
        return new Response(response)
    }
}

I think that's OK, even though there's four service calls. All the logic is busying itself with what it is to fulfil that request. The controller simply dictates what data needs returning, and gets that data. there is no business logic in there. It's all controller logic.

A pseudo-code example where we're adding business logic into this might be:

// ...
publishedContent = contentService.getForId(args.id, args.startDate, args.endDate)
topFiveArticles = publishedContent.slice(5)

// ...

response = {
    // ...
    articles = topFiveArticles,
    // ...
}

Here the slice call - even though it's a native array method - is still business logic. If the controller method was getTopFiveArticles or something, I'd go back to considering it controller logic. So it's a fine line.

Back to the code i was reviewing, it was doing 1&2, then it was going off-piste with that foreach loop, then doing 3&4. Ignoring the foreach, I was still itchy, but wasn't seeing why. It seemed like it was roughly analogous to my first example above.

So what's the problem? Why was the code still making me furrow my brow?

Well there was just too much code. That set-off a warning bell. I wanted to reduce the number of calls to various services (or repositories as is the case here: data from slightly different sources), but initially I couldn't really justify it. Then I looked closer.

The code in question was more like this (I've got rid of the date args to focus more):

class ContentController {
    function handleGet(rawArgs){
        args = validationService.validate(rawArgs)
        
        user = userService.get(args.id)
        publishedContent = contentService.getForId(args.id)
        
        twitterContent = twitterService.getForId(user.twitterId)
        facebookContent = facebookService.getForId(user.facebookId)
        
        response = {
            articles = publishedContent,
            socialContent = {
                twitter = twitterContent,
                facebook = facebookContent
            }
        }
        return new Response(response)
    }
}

I hasten to add the code I was reviewing is nothing to do with users and content and social media stuff, this is just an example which is analogous but would make more sense to people not in the context of our business.

The difference is subtle. The difference is that the decision that the IDs for the social content are stored in the user object is business logic. It's not controller logic to "know" that's how that data is stored. the user object here is not part of the data that composes the response; it's part of some business logic that dictates how to get the data.

And this does indeed break that second rule:

• a call to validate / sanitise all incoming arguments so they're fit for purpose for fulfilling the request;
• [...] calls to get data - using those incoming arguments - to build the response;

The call to get the social media content is not using those arguments. It's using a value from a separate call:

user = userService.get(args.id)
// ...
twitterContent = twitterService.getForId(user.twitterId)
facebookContent = facebookService.getForId(user.facebookId)

Another tell tale here is that user is not even part of the response. It's just used to get those "foreign keys".

My rule of thumb in those four points above is still correct. But one needs to pay special attention to the second point. Any time one's inclined to fetch data "indirectly", the indirection is business logic. It might not be a loop or some conditional or switch block or something more obvious that manipulates the data before it's returned, but that indirection is still business logic. And does not belong a the controller.

So how should that controller look? Move the content acquisition out into a service that has the explicit purpose of getting a user's content - from wherever - via some key:

class ContentController {
    function handleGet(rawArgs){
        args = validationService.validate(rawArgs)
        
        content = contentService.getById(args.id)
        
        response = {
            articles = content.published,
            socialContent = {
                twitter = content.twitter,
                facebook = content.facebook
            }
        }
        return new Response(response)
    }
}

This follows the second point to the letter, and there's now no business logic in the controller. Also the controller legitimately does not really need unit testing now, but the logic in the ContentService can be. Also the controller method is down to a size that would not raise any eyebrows.

This is not an earth-shattering conclusion by any measure, but I'm quite pleased to fine-tune my position here, and hopefully it can help drive discussion about this in code reviews in future.

It'd be great to hear anyone else's thoughts on this?

--
Adam

Help explain closure to the Adobe ColdFusion Team

G'day:
OK, this is a CFML-centric article. But it's also a call to provide code analogies in different languages to a CFML example, to show the ColdFusion Team they dunno what they're on about.

Here's some CFML code:

function doit() {
    var a = ["a", "b", "c"];
    var b = ["x", "y", "z"];
    var counter = 0;
    
    arrayEach(a, function(foo) {
        counter = 0;
        arrayEach(b, function(bar) {
            counter++;
            // in dump counter is always 0
            writeDump({
                counter: counter, 
                foo: foo, 
                bar: bar
            });
        });
    });
}
doit();

This leverages closure to reference the outer counter variable within the innermost function expression. Ergo, it's the same variable. So the expected output of this would be:

Note how the counter is declared in the main function, reset in the outer arrayEach handlers and incremented for each iteration of the inner arrayEach call. So it'll cycle through 1,2,3 three times when output.

That was run on Lucee. Running it on ColdFusion yields:

See how the counter is messed up: it's always zero. It should increment for each iteration of the inner function expression.

Adobe - being their typical selves - is claiming this is "by design":

https://tracker.adobe.com/#/view/CF-4197194:

That's nonsense.

Other languages behave predictably:

JavaScript:

function doit() {
    var a = ["a", "b", "c"];
    var b = ["x", "y", "z"];
    var counter = 0;
    
    a.forEach(function(foo) {
        counter = 0;
        b.forEach(function(bar) {
            counter++;
            // in dump counter is always 0
            console.log({
                counter: counter, 
                foo: foo, 
                bar: bar
            });
        });
    });
}
doit();
VM128:11 Object {counter: 1, foo: "a", bar: "x"}
VM128:11 Object {counter: 2, foo: "a", bar: "y"}
VM128:11 Object {counter: 3, foo: "a", bar: "z"}
VM128:11 Object {counter: 1, foo: "b", bar: "x"}
VM128:11 Object {counter: 2, foo: "b", bar: "y"}
VM128:11 Object {counter: 3, foo: "b", bar: "z"}
VM128:11 Object {counter: 1, foo: "c", bar: "x"}
VM128:11 Object {counter: 2, foo: "c", bar: "y"}
VM128:11 Object {counter: 3, foo: "c", bar: "z"}

And PHP (apologies for the rubbish way PHP does closure):

function doit() {
    $a = ["a", "b", "c"];
    $b = ["x", "y", "z"];
    $counter = 0;
    
    array_walk($a, function($foo) use (&$counter, $b) {
        $counter = 0;
        array_walk($b, function($bar) use (&$counter, $foo) {
            $counter++;
            // in dump counter is always 0
            var_dump([
                "counter" => $counter, 
                "foo" => $foo, 
                "bar" => $bar
            ]);
        });
    });
}
doit();

array(3) {
  ["counter"]=>
  int(1)
  ["foo"]=>
  string(1) "a"
  ["bar"]=>
  string(1) "x"
}
array(3) {
  ["counter"]=>
  int(2)
  ["foo"]=>
  string(1) "a"
  ["bar"]=>
  string(1) "y"
}
array(3) {
  ["counter"]=>
  int(3)
  ["foo"]=>
  string(1) "a"
  ["bar"]=>
  string(1) "z"
}
array(3) {
  ["counter"]=>
  int(1)
  ["foo"]=>
  string(1) "b"
  ["bar"]=>
  string(1) "x"
}
array(3) {
  ["counter"]=>
  int(2)
  ["foo"]=>
  string(1) "b"
  ["bar"]=>
  string(1) "y"
}
array(3) {
  ["counter"]=>
  int(3)
  ["foo"]=>
  string(1) "b"
  ["bar"]=>
  string(1) "z"
}
array(3) {
  ["counter"]=>
  int(1)
  ["foo"]=>
  string(1) "c"
  ["bar"]=>
  string(1) "x"
}
array(3) {
  ["counter"]=>
  int(2)
  ["foo"]=>
  string(1) "c"
  ["bar"]=>
  string(1) "y"
}
array(3) {
  ["counter"]=>
  int(3)
  ["foo"]=>
  string(1) "c"
  ["bar"]=>
  string(1) "z"
}

But I'm quite keen to know if there's any language that behaves as ColdFusion does in this example? If you've got a mo', could you knock out an equivalent of this code in [your language of choice], and share the results?

I'm 99.999% sure Adobe are just not quite getting closure, but wanna make sure I'm not missing anything.

Righto.

--
Adam

Code puzzle: find any match from an array of regex patterns

G'day:
I'm being slightly cheeky as I'm hijacking someone else's question / puzzle from the CFML Slack channel that we were discussing last night. Don't worry, that'll be the last mention of CFML in this article, and indeed the question itself was asking for a JS solution. But it's a generic code puzzle.

Let's say one has a string, and an array of regex patterns one wants to check for in said string. A number of solutions were offered, using a number of techniques. And it even branched away from JS into Clojure (thanks Sean).

How would you go about doing it?

The input data is:

patterns = ["Savvy", "Smart", "Sweet"]
testForPositive = "Super Savvy"
testForNegative = "Super Svelt"

And in these cases we want true and false to be returned, respectively.

Note that the patterns there need to be considered regular expressions, not simple strings. The example data is just using simple strings for the sake of clarity.

Initially I was gonna do something like this:

function matchesOne(str, patterns) {
    var onePattern = new RegExp(patterns.join("|"));
    return str.search(onePattern) > -1;
}

Here I'm joining all the patterns into one big pattern, and searching for that. I was slightly hesitant about this as it's loading the complexity into the regex operation, which I felt perhaps wasn't ideal. Also it's not very clever: if any of the individual patterns had a | in them: this'd break. Scratch that.

Ryan Guill seeded the idea of approaching it from the other direction. Using the array as an array, and using one of JS's array higher-order functions to do the trick. We saw examples using find and filter, but I reckoned some was the best option here:

function matchesOne(str, patterns) {
    return patterns.some(pattern => str.search(pattern) > -1);
}

I'm happy with that.

The ante was raised next: write a function which returned the position of the first match (not just a boolean as to whether there was a match or not).

To me this was a reduction operation: take an array and - given some algorithm applied to each element - return a final single value.

I tried, but I could not make it very clean. This is what I got:

function firstMatch(str, patterns) {
    return patterns.reduce(function(first, pattern){
        var thisMatch = str.search(pattern);
        return first == -1 ? thisMatch : thisMatch == -1 ? first : Math.min(first, thisMatch);
    }, -1);
}

I'm happy with the reduce approach, but I'm really not happy with the expression to identify the lowest match (or continue to return -1 if no match is found):

first == -1 ? thisMatch : thisMatch == -1 ? first : Math.min(first, thisMatch)

To be clear, if we expanded that out, the logic is:

if (first == -1) {
    return thisMatch;
}
if (thisMatch == -1) {
    return first;
}
return Math.min(first, thisMatch);

That's actually "cleaner", but I still think I'm missing a trick.

So the puzzle to solve here is: what's a clever (but also Clean - in the RC Martin sense) way returning the lowest non -1 value of first and thisMatch, or fall back to -1 if all else fails.

Using this code (or some approximation thereof):

function firstMatch(str, patterns) {
    return patterns.reduce(function(first, pattern){
        var thisMatch = str.search(pattern);
        // return result of your code here
    }, -1);
}

console.log(firstMatch("Super Savvy", ["Savvy", "Smart", "Sweet"]));
console.log(firstMatch("Super Svelt", ["Savvy", "Smart", "Sweet"]));

One should get 6 and -1 for the respective "tests".

Use any language you like.

Oh and hey... if you have any interesting solutions for the two functions themselves, feel free to post 'em too.


Righto.

--
Adam

CFML: that survey... sit. rep.

G'day:
A coupla people have asked me what's happening with my feedback from that survey you all did for me (Survey: CFML usage and migration strategies).

I have to simply apologise for the delay. I started to look at it, but could not work out how best to present the data. Then I buggered off to NZ to sit in the sun watching cricket and drinking beer with me mates; and "CFML survey data analysis" was not high on my list of priorities. I did start looking at the data some more though.

So not much progress yet, but I'm now back in London and trying to refocus on IT stuff, so I'll try to start feeding back soon. -ish.

Again: apologies for the delay.

Righto.

--
Adam

PHPUnit: setMethods and how I've been writing too much code

G'day:
Seems our technical architect is becoming my PHP muse. Good on ya, Pete.

Anyway, he just sent out what I thought was a controversial email in which he said (paraphrase) "we're calling setMethods way too often: in general we don't need to call it". To contextualise, I mean this:

$dependency = $this
    ->getMockBuilder(MyDependency::class)
    ->setMethods(['someMethod'])
    ->getMock();

$dependency
    ->method('someMethod')
    ->willReturn('MOCKED RESULT');

See here I'm explicitly saying "mock out the someMethod method". Pete's contention is that that's generally unnecessary, cos getMockBuilder mocks 'em all by default anyhow. This goes against my understanding of how it works, and I've written bloody thousands of unit tests with PHPUnit so it surprised me I was getting it wrong. I kinda believed him cos he said he tested it, but I didn't believe him so much as to not go test it myself.

So I knocked out a quick service and dependency for it:

namespace me\adamcameron\myApp;

class MyService
{

    private $myDecorator;

    public function __construct(MyDecorator $myDecorator)
    {
        $this->myDecorator = $myDecorator;
    }

    public function decorateMessage($message)
    {
        $message = $this->myDecorator->addPrefix($message);
        $message = $this->myDecorator->addSuffix($message);
        return $message;
    }

}

namespace me\adamcameron\myApp;

class MyDecorator
{

    public function addPrefix($message)
    {
        return "(ACTUAL PREFIX) $message";
    }

    public function addSuffix($message)
    {
        return "$message (ACTUAL SUFFIX)";
    }

}

Nothing surprising there: the service takes that decorator as a constructor arg, and then when calling decorateMessage the decorator's methods are called.

In our tests of decorateMessage, we don't want to use the real decorator methods, we want to use mocks.

First I have a test the way I'd normally mock things: explicitly calling setMethods with the method names:

public function testWithExplicitMock()
{
    $decorator = $this
        ->getMockBuilder(MyDecorator::class)
        ->setMethods(['addPrefix', 'addSuffix'])
        ->getMock();

 $decorator
  ->method('addPrefix')
  ->willReturn('(MOCKED PREFIX)');

 $decorator
  ->method('addSuffix')
  ->willReturn('(MOCKED SUFFIX)');

 $service = new MyService($decorator);

 $result = $service->decorateMessage('TEST MESSAGE');

 $this->assertSame(
        '(MOCKED SUFFIX)',
        $result
    );
}

And this mocks out the method correctly. This is my baseline.

Secondly, I still call setMethods, but I give it an empty array:

public function testWithImplicitMock()
{
    $decorator = $this
        ->getMockBuilder(MyDecorator::class)
        ->setMethods([])
        ->getMock();

 $decorator
  ->method('addPrefix')
  ->willReturn('(MOCKED PREFIX)');

 $decorator
  ->method('addSuffix')
  ->willReturn('(MOCKED SUFFIX)');

 $service = new MyService($decorator);

 $result = $service->decorateMessage('TEST MESSAGE');

 $this->assertSame(
        '(MOCKED SUFFIX)',
        $result
    );
}

This also mocks out the method correctly. Passing an empty array mocks out all the methods in the mocked class.

Next I try passing null to setMethods:

public function testWithNull()
{
    $decorator = $this
        ->getMockBuilder(MyDecorator::class)
        ->setMethods(null)
        ->getMock();

    $decorator
        ->method('addPrefix')
        ->willReturn('(MOCKED PREFIX)');

    $decorator
        ->method('addSuffix')
        ->willReturn('(MOCKED SUFFIX)');

    $service = new MyService($decorator);

    $result = $service->decorateMessage('TEST MESSAGE');

    $this->assertSame(
        '(ACTUAL PREFIX) TEST MESSAGE (ACTUAL SUFFIX)',
        $result
    );
}

This creates the mocked object, but the methods are not mocked. So the result here is using the actual methods.

In the last example I mock one of the methods (addPrefix), but not the other (addSuffix):

public function testWithOneExplicitMock()
{
    $decorator = $this
        ->getMockBuilder(MyDecorator::class)
        ->setMethods(['addPrefix'])
        ->getMock();

    $decorator
        ->method('addPrefix')
        ->willReturn('(MOCKED PREFIX)');

    $service = new MyService($decorator);

    $result = $service->decorateMessage('TEST MESSAGE');

    $this->assertSame(
        '(MOCKED PREFIX) (ACTUAL SUFFIX)',
        $result
    );
}

Here we see the important bit: when one explicitly mocks one or a subset of methods, then the other methods are not mocked. Which is kinda obvious now I say it, but I guess we had been thinking one needed to call setMethods to be able to then mock the behaviour of the method. And, TBH, I don't think we thought through what was happening to the methods we were not mocking. I think we started calling setMethods all the time because we were slavishly following the PHPUnit docs, which always call it too, which is a less than ideal precedent to "recommend".

Generally speaking, one should want to mock all methods of a dependency, and it should really be the exception when explicitly not wanting to mock a method: ie, to leave it actually callable.

The win here is that we can now de-clutter our tests a bit. We really seldom need to call setMethods, and we sure want to make it clear when we do want to only mock some of a dependency's methods. So this will make our code cleaner and clearer, and easier to code review and maintain. Win!

Cheers Pete!

Righto.

--
Adam

PHP: more array dereferencing weirdness

G'day:
(Firstly: I hasten to add that the "weirdness" is "weird to me". It might be completely predictable to everyone else in the PHP world).

This is a follow-on from my earlier article "PHP: accessing an undefined array element yields a notice. Or does it? WTF, PHP?". Now before I start: Kalle Sommer Nielsen has come back to me in a comment on that article and mentions it's expected behaviour, and explained it... but I don't quite follow yet. I'm hoping some ensuing experimentation will cast some light on the scene.

My next variation of this has me scratching my head more. Well: it's demonstrating the same thing, but perhaps more clearly.

<?php

echo PHP_EOL . "Test with array" . PHP_EOL;

$a = [1, 2, 3];
var_dump($a[0]);
var_dump($a[0]['id']);
var_dump($a[0][0]['id']);
var_dump($a[0]['id'][0]);

echo PHP_EOL . "Test with int" . PHP_EOL;
$i = 1;
var_dump($i['id']);
var_dump($i[0]['id']);
var_dump($i['id'][0]);

echo PHP_EOL;

echo PHP_EOL . "Test with string" . PHP_EOL;
$s = '1';
var_dump($s['id']);
var_dump($s[0]['id']);
var_dump($s['id'][0]);

This yields:

C:\temp>php test.php

Test with array
C:\temp\test.php:6:
int(1)
C:\temp\test.php:7:
NULL
C:\temp\test.php:8:
NULL
C:\temp\test.php:9:
NULL

Test with int
C:\temp\test.php:13:
NULL
C:\temp\test.php:14:
NULL
C:\temp\test.php:15:
NULL


Test with string
PHP Warning:  Illegal string offset 'id' in C:\temp\test.php on line 21
PHP Stack trace:
PHP   1. {main}() C:\temp\test.php:0

Warning: Illegal string offset 'id' in C:\temp\test.php on line 21

Call Stack:
    0.0002     356384   1. {main}() C:\temp\test.php:0

C:\temp\test.php:21:
string(1) "1"
PHP Warning:  Illegal string offset 'id' in C:\temp\test.php on line 22
PHP Stack trace:
PHP   1. {main}() C:\temp\test.php:0

Warning: Illegal string offset 'id' in C:\temp\test.php on line 22

Call Stack:
    0.0002     356384   1. {main}() C:\temp\test.php:0

C:\temp\test.php:22:
string(1) "1"
PHP Warning:  Illegal string offset 'id' in C:\temp\test.php on line 23
PHP Stack trace:
PHP   1. {main}() C:\temp\test.php:0

Warning: Illegal string offset 'id' in C:\temp\test.php on line 23

Call Stack:
    0.0002     356384   1. {main}() C:\temp\test.php:0

C:\temp\test.php:23:
string(1) "1"

C:\temp>

OK, so here I factor-out the top-level array from the mix. That test was always referring to the first element of the array, which is just the integer 1, so we might as well just test with that. Needless to say, there's the same results as with the first array element. But I still don't get it. What is using array notation to dereference an integer supposedly doing? It's a non-sensical operation, as far as I understand things. And if it is a nonsensical operation: it should error.

For completeness I also try a variation with a string "1". This does error like I'd expect. Well: it gives a warning and then returns an inappropriate (IMO) result anyhow. This is probably PHP doing it's combo of telling me I did it wrong but then going ahead and guessing what I meant anyhow. Grrr.

I'm gonna mess around with de-referencing integers some more when I get a moment, and try to work out WTF.

Righto.

--
Adam

TDD: still testing too much

G'day:
A while back I wrote this article - Maintaining untested code, and mocking too much when backfilling test (with JavaScript & Jasmine) - wherein my colleague pulled me up for testing too much when I'm doing my TDD. I'm working with PHP this time, and I've just been pulled up on it again. For much the same reason. It created an interesting thought exercise... well interetsing ish... so I'll write it up here.

Firstly, I cannot use the actual code I am working with due to the usual over-baked "commercial sensitivity" considerations, so this is simply an analogy, but it captures the gist of what I was doing.

We have an adapter that handles HTTP calls. It's basically a generic wrapper that masks the API differences between Guzzle 5 and Guzzle 6, both of which we are still using, but we want a unified adapter to work with both. It's very generic, and basically has methods for GET, POST, PUT etc, and doesn't encompass any of our business logic.

Business reality is that we use this mostly to transmit JSONified objects, so we're adding a layer in front of it to take the raw text responses and rehydrate them. I'm writing the GET method.

When I start designing my solution, I know that the happy path will return me a Response object that has JSON as its content. I just need to deserialise the JSON and return it. This is easy enough:

public function testGetAsObject() {
    $expectedResult = ['result' => 'valid JSON'];
    $expectedContent = json_encode($expectedResult);

    $adapter = $this->mockAdapterWithResponse(200, $expectedContent);

    $client = new HttpClient($adapter);
    $result = $client->getWithJson($this->uri);

    $this->assertEquals($expectedResult, $result);
}

mockAdapterWithResponse just does this:

private function mockAdapterWithResponse($statusCode, $content){
    $response = $this->getMockBuilder(Response::class)
        ->setMethods(['getStatusCode', 'getContent'])
        ->getMock();
        
    $response->expects($this->once())
        ->method('getStatusCode')
        ->willReturn(statusCode);

    $response->expects($this->once())
        ->method('getContent')
        ->willReturn(content);

    $adapter = $this->getMockBuilder(Adapter::class)
        ->setMethods(['get'])
        ->getMock();
        
    $adapter->expects($this->once())
        ->method('get')
        ->willReturn($this->response);
        
    return $adapter;
}

(there are other tests that already needed this). All it's doing is enabling us to set what the content for the call to the HttpClient will return, because we don't care how the client goes about doing this; we just care what we do with the content afterwards.

So back to my test code:

public function testGetAsObject() {
    $expectedResult = ['result' => 'valid JSON'];
    $expectedContent = json_encode($expectedResult);

    $adapter = $this->mockAdapterWithResponse(200, $expectedContent);

    $client = new HttpClient($adapter);
    $result = $client->getWithJson($this->uri);

    $this->assertEquals($expectedResult, $result);
}

All I'm doing here is mocking the adapter to return a predetermined result, and I then test that what comes back from my function is the deserialisation of that value. That's it. I then implement some code to make that test pass:

public function getAsObject($uri, $params = [], $headers = []) {
    $response = $this->get($uri, $params, $headers);

    $object = json_decode($response->getContent(), true);

    return $object;
}

And that's green: all good.

I've already done too much in my test, but have not noticed (and not put it in for code review, so no-one's pointed it out).

The next requirement for this method is that if the content coming back from the adapter isn't JSON, then just throw an exception instead of trying to deserialise it. Again, pretty easy to TDD that:

/** @expectedException \Exception */
public function testGetAsObjectThrowsExceptionOnNonJsonContent() {
    $content = 'NOT_VALID_JSON';

    $adapter = $this->mockAdapterWithResponse(200, $content);

    $client = new HttpClient($adapter);
    $client->getWithJson($this->uri);
}

Note there's not even any explicit assertions in there: the @expectedException annotation takes care of it.

This test fails at present: no exception is thrown thanks to PHP's stupid-arse behaviour if json_decode is given non-JSON: it just returns null. Bravo, PHP. But that's what we want... we've designed our code and have tests to prove the design, now we need to implement the code:

public function getAsObject($uri, $params = [], $headers = []) {
    $response = $this->get($uri, $params, $headers);

    $object = json_decode($response->getContent(), true);
    
    if (is_null($object)) {
        throw new \Exception('Unable to decode JSON result');
    }
    return $object;
}

The tests are green, so all good.

But what have I done wrong? Nothing major, but once again I'm testing too much. Actually my tests are fine. It's my helper method that's "wrong":

private function mockAdapterWithResponse($statusCode, $content){
    $response = $this->getMockBuilder(Response::class)
        ->setMethods(['getStatusCode', 'getContent'])
        ->getMock();
        
    $response->expects($this->once())
        ->method('getStatusCode')
        ->willReturn(statusCode);

    $response->expects($this->once())
        ->method('getContent')
        ->willReturn(content);

    $adapter = $this->getMockBuilder(Adapter::class)
        ->setMethods(['get'])
        ->getMock();
        
    $adapter->expects($this->once())
        ->method('get')
        ->willReturn($this->response);
        
    return $adapter;
}

Why am I testing that those methods are called once? It is completely irrelevant to what I'm testing how many times they're called.

What got into my head is I was being too dogmatic when I was writing my code, and I also cheated a bit with the TDD. Instead of designing the test variation first, and then implemented the code to pass the test, I was doing both at the same time. And it shows from my test implementation. Basically I didn't design my solution first, I designed it as I went, so - whilst tricking myself into thinking I was doing TDD - I was asking myself "right, what does my code need to do? It needs to call get on the adapter. OK, so I'll test I call it. What next... I need to get the content from the response. Right: I'll make sure that getContent gets called". All very stolid and admirable, but: wrong. All this is doing is testing my code (almost line by line!). TDD is not about testing my code; it's about testing my design. And it's the design and behaviour of the API of the method I'm testing. As I've said before, it'd be excellent it TDD was retconned to mean "Test Driven Design", not "Test Driven Development". The former is closer to the intent of the exercise.

I don't need to test those methods are called once: there's no logic in my function that will vary how many times they might be called, so they will be called once. We can trust PHP to do what we tell it to do by giving it source code.

The logic flaw here was demonstrated by my code reviewer observed: if you think you need to check how many times that functionality is called, why are you also not concerned with how many times json_decode is called too? It's an important part of your functionality which definitely needs to be called. Ha... yeah "duh", I thought. This drove home I was testing too much.

Another thing is that I was getting too distracted by what my implementation was whilst writing my test. The requirements of that function are:

• make a request, the response of which might be JSON:
• if it is: return the deserialised content;
• if it's not: raise an exception.

That first step is implementation detail from the POV of testing. What we're testing with TDD is the outcome of the function call. Which is: an object, or an exception. We're also only supposed to be testing the logic in the function the test is actually calling. I emphasise "logic" there because we're not testing the code. TDD is not about that. We're testing the logic. This is why we have two test variations: one wherein the logic to throw the exception is not true, so we get our object back; another when that if condition is true, so we get the exception.

We only need to test logic branching code (conditionals and loops, basically).

We do not need to prove every single statement is executed.

I was reminded a rule of thumb to apply when considering whether to use expects or not. Basically it boils down to if you have test variations which might result in a dependency being called a different amount of times, and that variation is germane to the result, then verify the call count.

Another good point made by my other colleague Brian was that when mocking stuff... only mock the bare minimum to get the test passing. Then stop. And that's probably the right place to leave it be.

An example of this might be if we had this logic in our method:

public function getAsObject($uri, $params = [], $headers = []) {
    $response = $this->get($uri, $params, $headers);

    $object = json_decode($response->getContent(), true);
    
    if (is_null($object)) {
        $this->logger("Bung content returned: $object");
        throw new \Exception('Unable to decode JSON result');
    }
    return $object;
}

Logging is an optional outcome of that method call, so we might want to test that the logger definitely gets called once in that second test. We could also be belt-and-braces about it and test it's not called at all in the happy-path test.

Or let's say the code was a caching proxy and one is testing the uncached and cached variations:

function get($id){
    if ($this->cache->exists($id)){
        return $this->cache->get($id);
    }
    return $this->dao->get($id);
}

In the uncached variation, the data is not found in the cache, so we want to make sure that the DAO is called to get the data from the DB. In the cached variation, we want to make sure the DAO isn't called. Even here, in the first example we're not really testing the DAO call was made specifically; we're testing the logic around the cache check didn't trigger the early return. We're testing the if statement, not the DAO call.

Bottom line, I think that if I find myself starting to do a call count on a mocked method, I need to stop to think about whether I will have cases where the expected call count will be different, and only then ought I even be bothering if the call-count variation is important to the outcome of the function call. If I'm only checking for one call count value... I'm doing it wrong.

And now rather than talking about my day job... I should actually be doing it...

Righto.

--
Adam