Review: std.uuid
Jonathan M Davis
jmdavisProg at gmx.com
Wed Jun 13 18:57:51 PDT 2012
On Saturday, June 09, 2012 21:30:57 Dmitry Olshansky wrote:
> Code: https://github.com/jpf91/phobos/blob/std.uuid/std/uuid.d
> API-Docs: http://dl.dropbox.com/u/24218791/d/src/uuid.html
* I'm not terribly fond of having names like Variant and Version, but they
_are_ local to UUID, so I guess that they're clear enough. But the fact that
you have to put a note about it _not_ being std.variant.Variant definitely
indicates that another name might be better. I know that the RFC uses the word
variant, but it also indicates that type would probably be a better name, and
if Variant is unclear enough that a note is needed, then perhaps it really
isn't the right name.
* nameBasedMd5 should probably be nameBasedMD5, and nameBasedSha1 should
probably be nameBasedSHA1. The correct names are MD5 and SHA-1, not md5 and
sha1, so it would be more correct to use nameBasedMD5 and nameBasedSHA1. With
acronyms, the thing that most of Phobos does is to keep all of the letters the
same case, so if the first letter is lowercase, then the whole thing is
lowercase, and if the first letter is uppercase, then the whole thing is
uppercase. I'm not sure if MD5 and SHA-1 are acronyms or not, but they're
similar enough, that I'd say that the same naming scheme should be used, which
you do in most cases, but you didn't with the enums for some reason.
* For clarity's sake, please always put const after the function signatures
for all const functions. In same cases you do, and in others you don't.
* If all that's preventing toString from being nothrow is idup, then just do
this:
string toString() @safe const pure nothrow
{
try
return _toString().idup;
catch(Exception e)
assert(0, "It should be impossible for idup to throw.");
}
and if there's a bug report for idup, then list it in a comment so that we
know to get rid of the try-catch block once that's been fixed (and if there
isn't a bug report, one should be created).
* Get rid of nilUUID. Just use UUID.init. It just seems like an extra,
unnecessary complication when the two are identical.
* Please use SHA-1 in the documentation rather than SHA1, since SHA-1 is the
correct name. Also, make sure that it's SHA-1 and not just SHA. You're not
always consistent (e.g. take a look at sha1UUID).
* parseUUID should be clear on whether it takes both lowercase and uppercase
hex digits, and I'd argue that it should definitely accept both. [0-9a-f]
implies that it only accepts lowercase hex digits.
* Use std.exception's assertThrown. It'll make your tests cleaner. And if you
can't do that, because you need to look at the exception itself, then use
collectException. So,
thrown = false;
try
//make sure 36 characters in total or we'll get a 'tooMuch' reason
id = UUID("{8ab3060e-2cba-4f23-b74c-b52db3bdf6}");
catch(UUIDParserException e)
{
assert(e.reason == UUIDParserException.Reason.invalidChar);
thrown = true;
}
assert(thrown);
becomes
//make sure 36 characters in total or we'll get a 'tooMuch' reason
auto e = collectException!UUIDParserException(UUID("{8ab3060e-2cba-4f23-b74c-
b52db3bdf6}"));
assert(e !is null);
assert(e.reason == UUIDParserException.Reason.invalidChar);
It's much shorter that way and less error-prone.
* You should probably change empty's body to
enum ubyte[16] emptyUUID = [0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0];
return data == emptyUUID[];
I'm not sure whether it currently allocates or not if you do that (I _think_
that it'll do the heap allocation at compile time and then only have the value
type at runtime), but regardless of whether it does or not, I'd expect that
once it's fixed so that assigning an array literal to a static array, it
definitely _won't_ allocate on the heap, whereas your current implementation
will regardless of that fix.
And if that doesn't seem like a good change, then you can just make a module-
level or struct variable which is immutable ubyte[16] and have empty use that.
As it stands, it's always going to allocate the array literal on the heap.
It's probably not a big deal, since empty probably won't be called all that
often, but if we can easily avoid the extra heap allocation, we should.
* I know that
void toString(scope void delegate(const(char)[]) sink)
is what has been previously discussed for a toString which doesn't allocate,
but I have to wonder why we don't just use an output range. I suppose that
that's a whole other discussion, but that's what I would have suggested rather
than a delegate - though in some circumstances it probably _is_ easier to just
create a nested function rather than a whole output range. Maybe we should be
moving to having 3 overloads of toString (normal, accepts delegate, and
accepts output range).
* rndGen returns a Random (which is already aliased to Mt19937), and already
initializes it with an unpredictable seed. So, why not just us rndGen in
randomUUID? It becomes
return randomUUID(rndGen());
It's much shorter and just as good as far as I can tell.
* Change randomValue in randomUUID to auto. The default random number
generator _does_ use uint, but others could use ubyte, ushort, or ulong
instead. I'd also be tempted to argue that you should make it immutable and
use a different variable for each random value (you won't ever accidentally
fail to set it to a new value that way - that I don't think that reusing
variables is a good idea in general), but the benefits of that are debatable.
Actually, just make it a loop. Something like
foreach(size_t i; iota(0, 16, 4))
{
randomGen.popFront();
immutable randomValue = randomGen.front;
u.data[i .. i + 4] = *cast(ubyte[4]*)&randomValue;
}
That's probably slightly less efficient (though I wouldn't think by much), but
it's much cleaner. And actually, since the randomValue isn't guarantee to be a
uint, but your code needs it to be 4 bytes, you should probably change that
line to
immutable uint randomValue = cast(uint)randomGen.front;
* I'd advise giving more descriptive names to parseError's parameters. It's
true that it's a nested function and not in the actual API, but it would make
the function easier to understand.
* You should probably use R or Range for template parameters which are ranges.
That's the typical thing to do. T is usually used for generic types rather
than for ranges. The code will be somewhat easier to read if you use R or
Range given that that's how the rest of Phobos is.
* In parseUUID, element should be a size_t, since it's used specifically for
indexing. As it stands, the compiler is going to have to do extra conversions
in some of the places that it's used (e.g. converting it to size_t when
indexing).
* In the unit tests, I'd argue that it would be cleaner to just use the result
directly in an assertion rather than saving it if it's not going to be used
outside of the assertion. For instance,
id = parseUUID("5668122d-9df0-49a4-ad0b-b9b0a57f886a"d);
assert(id.data == [86, 104, 18, 45, 157, 240, 73, 164, 173,
11, 185, 176, 165, 127, 136, 106]);
should probably be
assert(parseUUID("5668122d-9df0-49a4-ad0b-b9b0a57f886a"d).data ==
[86, 104, 18, 45, 157, 240, 73, 164, 173, 11, 185, 176, 165, 127,
136, 106]);
It's generally shorter, cleaner, and avoids the risk of reusing the variable
without setting it again.
* For your functions which can take range types, test with filter!"true" so
that you get a range which isn't an array or string.
* When testing various string types, you should take advantage of TypeTuple
and std.conv.to to be thorough. For instance, instead of
id = parseUUID("5668122d-9df0-49a4-ad0b-b9b0a57f886a"d);
assert(id.data == [86, 104, 18, 45, 157, 240, 73, 164, 173,
11, 185, 176, 165, 127, 136, 106]);
id = parseUUID("5668122d-9df0-49a4-ad0b-b9b0a57f886a"w);
assert(id.data == [86, 104, 18, 45, 157, 240, 73, 164, 173,
11, 185, 176, 165, 127, 136, 106]);
do something like
foreach(S; TypeTuple!(char, const char, immutable char,
wchar, const wchar, immutable wchar,
dchar, const dchar, immutable dchar))
{
assert(parseUUID(to!S("5668122d-9df0-49a4-ad0b-b9b0a57f886a")).data ==
[86, 104, 18, 45, 157, 240, 73, 164, 173, 11, 185, 176, 165, 127,
136, 106]);
}
And while you're at it, you could throw _all_ of your string tests in that
loop save for those which wouldn't really make sense to put in there (and you
probably don't want to put very many exception tests in the loop, since those
get to be expensive). You get much better testing this way.
However, do make sure that some of the exception tests use multiple string
types even if they aren't in the TypeTuple loop, since some of your exceptions
(in parseUUID in particular) depend on the type of range passed in.
* I'm not sure if you do or not, but if you have a spot where you have to
create a UUIDParserException from another exception but don't want to have to
specify the file and line number, then you should create an overload of
UUIDParserException which takes the Throwable first (and probably forwards to
the other constructor to avoid code duplication). Exception does this, and it
can be quite useful. But UUIDParserException can't be publicly constructed, so
only do it if it's actually useful within std.uuid.
* Personally, I'd prefer UUIDParsingException to UUIDParserException, since
you're not writing a full-on parser but rather just doing parsing in some of
your functions. But that's fairly subjective.
- Jonathan M Davis
More information about the Digitalmars-d
mailing list