Merge lp:~julian-edwards/gwacl/add-role-endpoint into lp:gwacl

[Julian Edwards]

Add AddRoleEndpoints api call.

Mostly straightforward, although I didn't add a check that new endpoints clashes with existing ones, I'm not entirely sure what to check (ie what are primary keys for an endpoint on Azure) as I don't trust the documentation. Given that Azure should reject stuff it doesn't like, the same end product is achieved except with one more round trip.

Also, these tests are getting unwieldy. I'd like to sit down with someone to think about how best to refactor as the boilerplate is crazy.

[Jeroen T. Vermeulen (jtv)]

I like the "TestWhen" better than the more cryptic implicit inclusion of a scenario ("TestNoPreexistingItems" — does that prove that there aren't any, or does it just illustrate what happens when there aren't any? Is shampoo "for normal hair" for people who have normal hair, or people who would like to have normal hair?).

For the first time I notice that the narrower indentation really helps legibility. Fewer horizontal acrobatics for the eyeballs. Even so, my eyes glaze over when I try to review Go tests. So especial compliments for testing both error exits from the update method.

In AddRoleEndpoints, I think having separate "the list is nil" and "the list is non-nil" cases doesn't really pay. There's a siren's song in Go that draws us towards micro-optimizations. But now you have two blocks of code that both need their own test in order to be exercised at all. Why not say "if the list is nil, set it to an empty one" and then continue with the "append"? It shares more code between code paths.

Then there's the surrounding loop. It looks big, but actually this is just a linear search loop. It just happens to incorporate part of the action it undertakes once it finds what it's looking for, inside its body. But there's no reason why that code should be in the loop body: once you start down this road you might as well put the rest of the function in there, and do a "return" instead of a "break"!

I'm also slightly uncomfortable with it all because TestWhenNoPreexistingEndpoints and TestWhenPreexistingEndpoints mess with the original array pointers, and it's not actually specified that Go's append() will return a different array than you stuck in. It's probably OK but for my tastes it skirts a bit close to the details of how this all works — you *could* be creating a self-fulfilling prophecy in those tests. At least having the append() be in the shared code path will maximize our chances of spotting it if anything goes wrong with it.

Generally, it's more manageable to separate the search from what you do with the result (and avoid a level of nesting to boot):

    item := search(items)
    if item != nil {
        process(item)
    }

That has the added advantage that it makes the outcome explicit. What do you do if there is no NetworkConfiguration? Maybe that never happens, but it's at least worth a comment.

review: Approve

[Julian Edwards (julian-edwards)]

On 18/07/13 18:34, Jeroen T. Vermeulen wrote:
> Review: Approve
>
> I like the "TestWhen" better than the more cryptic implicit inclusion of a scenario ("TestNoPreexistingItems" — does that prove that there aren't any, or does it just illustrate what happens when there aren't any? Is shampoo "for normal hair" for people who have normal hair, or people who would like to have normal hair?).

I blatantly copied the style from Gavin's code. Glad you like it :)

> For the first time I notice that the narrower indentation really helps legibility. Fewer horizontal acrobatics for the eyeballs. Even so, my eyes glaze over when I try to review Go tests. So especial compliments for testing both error exits from the update method.

I figured I might as well not be responsible for more coverage gaps...

> In AddRoleEndpoints, I think having separate "the list is nil" and "the list is non-nil" cases doesn't really pay. There's a siren's song in Go that draws us towards micro-optimizations. But now you have two blocks of code that both need their own test in order to be exercised at all. Why not say "if the list is nil, set it to an empty one" and then continue with the "append"? It shares more code between code paths.

You know, I considered that, but I reckon it obfuscates what's really
happening.

I dunno, maybe you and I just look at it differently!

I changed it anyway, since I don't care so much either way but you do.

> Then there's the surrounding loop. It looks big, but actually this is just a linear search loop. It just happens to incorporate part of the action it undertakes once it finds what it's looking for, inside its body. But there's no reason why that code should be in the loop body: once you start down this road you might as well put the rest of the function in there, and do a "return" instead of a "break"!

OK.

> I'm also slightly uncomfortable with it all because TestWhenNoPreexistingEndpoints and TestWhenPreexistingEndpoints mess with the original array pointers, and it's not actually specified that Go's append() will return a different array than you stuck in. It's probably OK but for my tastes it skirts a bit close to the details of how this all works — you *could* be creating a self-fulfilling prophecy in those tests. At least having the append() be in the shared code path will maximize our chances of spotting it if anything goes wrong with it.

Doesn't this conflict with your other argument about optimisers being so
good that they detect all this?

> Generally, it's more manageable to separate the search from what you do with the result (and avoid a level of nesting to boot):
>
> item := search(items)
> if item != nil {
> process(item)
> }
>
> That has the added advantage that it makes the outcome explicit. What do you do if there is no NetworkConfiguration? Maybe that never happens, but it's at least worth a comment.
>

This is where my knowledge of Go (or lack of) forced my hand - are "i"
and "configSet" set after the loop exits?

Also, there is a comment about it. I added a TODO to make it explicit.

Cheers!

[lp:~julian-edwards/gwacl/add-role-endpoint]

200. By Julian Edwards on 2013-07-18

review comments