Shared review for August 21

[Andrew Wilkins]

I was planning to attend the standup today, but I've been summoned to
dinner at my mother in law's this evening. I'd appreciate if someone would
jot down the takeaways from the discussion. Here's a few thoughts from me:

In general, good, easy to understand code. Good ratio of code to comments
(and useful comments too).

Some of the utility function names are meaningful in the file context, but
not so much at the package level. e.g. ParentId (parent of what?),
NestingLevel (nesting level of what?), TopParentId (you get the picture).
To fill in the gap you need to be in the right file, or read the docstring.
I'm a big believer in self-describing names. Also, maybe these functions
should move to "names"?

machineContainers's field names are a little opaque. I'd prefer
Id->MachineId, Children->ContainerIds. Looks like the structure mirrors the
DB schema though.

createContainerRefOp's docstring is missing a word after the function name,
which confused me (as someone who's never been through this code before).
After reading the docstring for removeContainerRefOps, it's clear what it
should be.

Cheers,
Andrew


On Wed, Aug 21, 2013 at 2:59 PM, John Arbash Meinel
<john at arbash-meinel.com>wrote:

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Since I'm getting this out a bit late, I'm going to try and go with a
> smaller file. For those that get this in time, if you can take about 1
> hour to review the file:
>
>   state/container.go (and its associated tests, etc)
>
> Obviously some of the time will be spent looking up what various
> objects are in other files, etc.
>
> We'll be talking about it during the standup time today.
>
> John
> =:->
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.13 (Cygwin)
> Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
>
> iEYEARECAAYFAlIUZTQACgkQJdeBCYSNAANsMACgvY57WJ3Smq5d6NxFfA3X3vdR
> 7dUAn1ZW7dE+2WqOYz1wr+VRhDP4YehV
> =mZyj
> -----END PGP SIGNATURE-----
>
> --
> Juju-dev mailing list
> Juju-dev at lists.ubuntu.com
> Modify settings or unsubscribe at:
> https://lists.ubuntu.com/mailman/listinfo/juju-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.ubuntu.com/archives/juju-dev/attachments/20130821/a5c3b834/attachment.html>