On Monday, 16 June 2014 at 19:42:14 UTC, Mike wrote:
I have refactored the code as recommended.
I have also modified the not-yet-reviewed writers part to take
advantage of the same approach (preallocated static-sized
buffer) rather than allocate slices in loops.
Hoping to hear something from you guys!
Best,
Mike
https://github.com/emesx/TGA/blob/develop/source/tga/model/utils.d#L80
Fully qualified names are only necessary when there's a name
conflict. The std.algorithm can be removed.
https://github.com/emesx/TGA/blob/develop/source/tga/model/utils.d#L8
It's a shame the result of among can't be returned directly but
casts are terribly heavy handed. They're best reserved for when
you're poking holes in the type system ;)
For safe conversions use to!something:
return to!bool(isColorMapped(header)
? header.colorMapDepth.among(16, 32)
: header.pixelDepth.among(16, 32));
https://github.com/emesx/TGA/blob/develop/source/tga/model/utils.d#L29
This one depends on taste, but these helpers can be eliminated by
changing the Header definition a little. It's the same union /
anonymous struct trick from the previous post, only this time
with bitfields:
http://dpaste.dzfl.pl/13258b0ce0c4
https://github.com/emesx/TGA/blob/develop/source/tga/model/utils.d#L92
Hashtable lookup would indeed be nice here, but if you ever need
a linear lookup it's included in std.algorithm:
http://dlang.org/library/std/algorithm/countUntil.html
const index = colorMap.countUntil(pixel);
enforce(index >= 0, "Pixel color not found in color map: " ~
pixel.text);
return index.to!ushort;
Note the use of to!ushort to avoid the cast. to!ushort also
performs overflow checking.
https://github.com/emesx/TGA/blob/develop/source/tga/model/validation.d#L52
This is one of the many places where the allowed bitdepths are
enumerated in an array / among(). Putting the allowed bitdepths
in an enum has some advantages: use of '.max' for buffer sizes,
EnumMembers in conjunction with among and only, to! checking for
correctness... Some examples here:
http://dpaste.dzfl.pl/30d2a593468f
But it depends on your taste. Personally I prefer to use strong
typing like this whenever possible. Helps to find bugs earlier,
and makes maintenance easier when your software and data format
are still in flux.
https://github.com/emesx/TGA/blob/develop/source/tga/model/validation.d#L39
This should be checked both ways. The current method will not
detect an error when the image type is not mapped but a color map
is present. At least I assume that is not a valid TGA file?
enforce(header.colorMapDepth.among(EnumMembers!ColorMapDepth));
enforce(header.isColorMapped == (header.colorMapType ==
ColorMapType.PRESENT));
enforce(header.isColorMapped == (header.colorMapDepth !=
ColorMapDepth.BPP0));
https://github.com/emesx/TGA/blob/develop/source/tga/io/readers.d#L95
You could change this to
foreach(ref pixel; pixels)
{
pixel = ...
}
https://github.com/emesx/TGA/blob/develop/source/tga/io/readers.d#L165
Readability can be improved using the union + bitfields thingy.
It's a bit longer but much easier on the eyes IMO:
http://dpaste.dzfl.pl/77ecbafa1e0e
https://github.com/emesx/TGA/blob/develop/source/tga/io/writers.d#L23
The with statement can be used here:
with(image.header)
{
write(file, idLength);
...
}
I think this is everything I can come up with. The
writeCompressed function can probably be simplified using
std.algorithm but it's not straightforward. I'll need to ponder
it for a while.
