Code review comment for lp:~mir-team/mir/attestable-timestamps-server

> +#include <nettle/hmac.h>
> ...
> + struct hmac_sha1_ctx ctx;
>
> Surely we don't want to introduce this dependency to user code? (See [1].)
>
> There is no point in this class heirarchy if you are putting the derived class
> definition in the same header.
>
> An option to consider here would the Named Constructor Idiom.

Done.

>
> ~~~~
>
> +// Using the NVI Idiom
>
> Nobody takes NVPI seriously - it is a solution in search of a problem.
>
> The principle options to consider are Interface and Cheshire Cat (a.k.a.
> Pimpl). For a comparison of the different options see[2].
>
> ~~~~~

Done.

>
> + CookieFactoryNettle(std::vector<uint8_t> const& secret);
>
> This will throw if the secret is too small, but the behaviour and minimum size
> is not documented in a doc comment on the function. (Having a non-doc comment
> in the private section of the class doesn't help the user.)
>
> (But as I don't think this class should be made public anyway the
> documentation should go on the corresponding factory function.)
>
> ~~~~

Done.

>
> > > The older logic (based on the executable path) seems less surprising.
> > >
> > > ~~~~
> >
> > Not sure what the correct answer is here. What would be the best solution?
>
> I think to first check relative to the executable path (as it did before this
> MP), then to check the install location.
>
> E.g. if running from a build directory it should pick up the one from e.g.
> <build>/bin/udev_recordings/ if not it picks up the installed version.
>
> ~~~~

Done.

>
> [1] http://wiki.hsr.ch/Prog3/files/overload72-FINAL_DesigningHeaderFiles.pdf
>
> [2] http://www.twonine.co.uk/articles/SeparatingInterfaceAndImplementation.pdf

Thanks! Those were good reads. If you've more articles, I would be more then happy to read :).