Working on a library: request for code review

[Rene Zwanenburg via Digitalmars-d-learn]

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.