Code review comment for lp:~axwalk/juju-core/lp1186969-add-jujud-tools-help

Revision history for this message
Roger Peppe (rogpeppe) wrote :

On 24 July 2013 10:54, Andrew Wilkins <email address hidden> wrote:
> https://codereview.appspot.com/11758043/diff/1/cmd/jujud/helptool.go#newcode79
>> cmd/jujud/helptool.go:79: cmds := make([]cmd.Command, len(names))
>> How about:
>> cmds := make([]cmd.Command, 0, len(names))
>
>> then append to it inside the next loop rather than assigning
>> to a known index? Then you won't need the "if cmds[i] != nil"
>> condition inside the print loop.
>
> Actually that's just a brainfart; it should be make([]cmd.Command,
> len(names)).
> The condition will need to remain: it's there because NewCommand might
> fail
> in the first loop.

I'm suggesting that we only fill the slice with non-nil elements
(hence my suggestion to use append rather than "cmds[i] = c")
which would mean that even if NewCommand fails, we won't
need to test for a nil element in the second loop.

« Back to merge proposal