Code review comment for lp:~julian-edwards/gwacl/fix-block-lengths

On Monday 05 Aug 2013 09:41:25 Raphaël Badin wrote:
> Review: Approve
>
> Looks good.

Thanks for reviewing Mr B.

>
> [0]
>
> 8 + // Block IDs must be a consistent length, so pad it out.
> 9 + blockID = fmt.Sprintf("%030s", blockID)
>
> I was just wondering about the choice of "30" here but I guess it does not
> really matter. Maybe worth turning it into a private variable though,
> especially if you do what I suggest in [1].

Entirely random but there's no point making it small. I used the value
suggested in the bug since it's likely to work.

>
> [1]
>
> 21 - assertBlockSent(c, context, data, b64("0"),
transport.Exchanges[0])
> 22 + assertBlockSent(c, context, data,
> b64("000000000000000000000000000000"), transport.Exchanges[0]) etc.
>
> Using strings.Repeat would be a bit more elegant here.

The problem with things like that is that you could modify the private
variable and no test would fail. Now some may consider that good, but
personally I start to feel suspicious and would end up poking around to work
out why. I'd much rather have tests that test outcomes entirely independently
of internal code parameters.