Code review comment for lp:~natefinch/juju-core/005-azure-address

the code and tests for Addresses looks sane to me, though it might be
good to have someone on Red squad confirm the actual APIs you are
calling.

I have some questions about the Checker you implemented. The only bit
I'm genuinely worried about is mutating slices passed in by the caller.
If you add a test to reassure me we aren't changing them, the rest is
all just suggestions.

https://codereview.appspot.com/13082044/diff/1/testing/checkers/checker.go
File testing/checkers/checker.go (right):

https://codereview.appspot.com/13082044/diff/1/testing/checkers/checker.go#newcode157
testing/checkers/checker.go:157: // use it on very large slices
Is it hard to create a smart implementation that notices if the types
are hashable and can then just insert them into a map with counts and
then compare it?

counts := make(map[TYPE]int)
for i, v := range expected:
   counts[v] += 1
countsObtained := make(map[TYPE]int)
for i, v := range obtained:
   countsObtained[v] += 1

Obviously you need a bit more reflect magic than I've typed here, but
you also end up with nice summaries of the contents and what is
different. (You could even do it as a delta map and count up with
expected, and down with obtained.)

If it is "cheap" to determine if something is hashable, I think the
algorithm to count them is nice and short and reasonably obvious. And
probably most things we're doing are strings, structs, and ints anyway.

https://codereview.appspot.com/13082044/diff/1/testing/checkers/checker.go#newcode198
testing/checkers/checker.go:198: // that way we make sure the count of
duplicate items is the same
I'm a little concerned if removing them would mutate the caller's
slices. (You might compare to make sure they match, and then do other
things with the slice.)

Can you confirm that it doesn't?

Even better, add it as a test case so we ensure that we don't grow that
behavior accidentally in the future.

Another option that avoids rewriting the array N times would be to just
allocate a 'bool' array of the same length, and flag each entry with
True once you've seen it. When you iterate later, you can just check and
'continue' if that entry has already been seen.

You still end up with N^2 comparisons, but you have a lot fewer
reallocations.

My theoretical mind wonders if optimizing for the case of "lists are
identical" is worthwhile and starting the nested search at the offset of
the outer search. Or just doing one pass against all items until we have
something that doesn't match, and only then switch to the N^2 form.
Certainly not terribly important here.

https://codereview.appspot.com/13082044/diff/1/testing/checkers/checker_test.go
File testing/checkers/checker_test.go (right):

https://codereview.appspot.com/13082044/diff/1/testing/checkers/checker_test.go#newcode37
testing/checkers/checker_test.go:37: func (s *CheckerSuite)
TestSameContents(c *gc.C) {
I personally would rather see these split out as separate "Test*" cases,
rather than just commented sections. So a new suite for SameContents and
then test cases in that.

func (s *SameContentsSuite) TestSame(c *gc.C) {
   c.Check([]int(1, 2, 3), jc.SameContents, []int(1,2,3))
}

func (s *SameContentsSuite) TestEmpty(c *gc.C) {
   c.Check([]int{}, jc.SameContents, []int{})
}

etc.

That is a bit of a style thing, but I like having lots of very fast,
very focused test cases. (I do realize that you are using Check, so we
will get multiple errors in a single test case.)

As a taste thing, though, you're allowed to have a different opinion.

https://codereview.appspot.com/13082044/