Working on a library: request for code review

[Rene Zwanenburg via Digitalmars-d-learn]

On Wednesday, 11 June 2014 at 18:29:27 UTC, Mike wrote:
> Here's the link to the repo: http://bit.ly/1mIuGhv

I usually don't trust shortened URL's. Can you please post full 
URL's when not constrained by a character limit?

> Any feedback would be great!

First of all, I like your coding style. It's nice and readable :)

https://github.com/emesx/TGA/blob/develop/source/tga/model/utils.d

All those functions can be marked pure. Pure is D is a bit 
different than in other languages. I can recommend the following 
article as a good explanation:

http://klickverbot.at/blog/2012/05/purity-in-d/

Also, depending on your preference, you may want to put those 
functions in a

pure nothrow
{

}

block, or put

pure nothrow:

at the top of the source file. I'm not sure if all those 
functions can be nothrow though.


https://github.com/emesx/TGA/blob/develop/source/tga/model/utils.d#L8

These array literals will be allocated every time the function is 
called. 'only' can be used instead:

only(16, 32).canFind // etc..

http://dlang.org/library/std/range/only.html

I remember a function which does something like only only + 
canFind on one go. It would look something like

header.colorMapDepth.among(16, 32);

but I can't find it right now.. Maybe it was only proposed but 
never added.


https://github.com/emesx/TGA/blob/develop/source/tga/model/utils.d#L101

This has a pretty bad time complexity. In the worst case this is 
O(n^2), I think. You could use a hashmap to build the table, then 
convert to an array. Or even only use hashmaps as color tables 
internally to improve performance across the board.


https://github.com/emesx/TGA/blob/develop/source/tga/model/types.d#L11

Depending on your taste, a union + anonymous struct could be used:

struct Pixel
{
	union
	{
		ubyte[4] bytes;
		
		struct
		{
			ubyte b, g, r, a;
		}
	}
}


https://github.com/emesx/TGA/blob/develop/source/tga/model/validation.d#L47

to!string is so common it has a special alias: text. It's placed 
in std.conv so you still need to import that.

"Invalid color map pixel depth: " ~ header.colorMapDepth.text


https://github.com/emesx/TGA/blob/develop/source/tga/io/readers.d#L24

Yeah, we need something to read an entire struct from a file and 
convert it to the correct endianness..


https://github.com/emesx/TGA/blob/develop/source/tga/io/readers.d#L24
https://github.com/emesx/TGA/blob/develop/source/tga/io/readers.d#L91

Unnecessary GC allocation in a hot loop == :(
Make a buffer outside the loop and call the normal rawRead:

auto buffer = new ubyte[colorMapByteDepth];
foreach(i; 0 .. (header.colorMapLength - header.colorMapOffset))
{
	file.rawRead(buffer);
	colorMap[i] = pixelReader(buffer);
}

Even better, use a static array with the max byte depth, then 
slice to the correct length:

ubyte[MaxByteDepth] buffer;
foreach(i; 0 .. (header.colorMapLength - header.colorMapOffset))
{
	colorMap[i] = file.rawRead(buffer[0 .. 
colorMapByteDepth]).pixelReader;
}

Do the same thing here:
https://github.com/emesx/TGA/blob/develop/source/tga/io/utils.d#L5

T read(T)(File file) if(isNumeric!T){
     ubyte[T.sizeof] bytes;
     file.rawRead(s[]);
     return littleEndianToNative!T(bytes);
}


https://github.com/emesx/TGA/blob/develop/source/tga/io/readers.d#L144

Depending on your taste. The first is built in, the other needs 
std.algorithm.

pixel.bytes[] = chunk[];
chunk.copy(pixel.bytes);


https://github.com/emesx/TGA/blob/develop/source/tga/io/utils.d#L40

There's a version in std.algorithm:
http://dlang.org/phobos/std_algorithm.html#min


I need to go. Please don't mind any typo's and untested code ;). 
Didn't take a look at writers yet, readers and util need some 
more scrutiny, but the main theme is: eliminate unnecessary 
temporary GC allocations.