top | item 8977939

Yalz77: an LZ77 compression algorithm designed for code simplicity and clarity

19 points| nkurz | 11 years ago |bitbucket.org | reply

4 comments

order
[+] cessor|11 years ago|reply
Sorry, but no cigar from me. I like the code and it is well written, but I think clarity means something different to me.

"Clarity" does not come from putting comments and blank lines in the code. Properly naming your variables and functions will clear things up. For example fnv32a is a pretty poor name for a function, as it is not even pronouncable.

A good rule of thumb is to move the explanations in the comments to become the names of your functions.

For example "fnv32a" could be named "compression_quality_hash" I doubt that this is really the perfect name, but at least it is what the comment says; you get the idea.

Even if the authors of an original paper used names like this in their definition of an algorithm (so an argument for the poor names would be to stick to the reference impl), your code and your contribution could be to make things clearer, rather than sticking to their poor, mathematical naming schemas.

However I really like the interface of the algorithm. While your code looks quite complicated on the inside, it is nicely usable from the outside; much like a wristwatch.

Random Examples of variables that I would rename:

https://bitbucket.org/tkatchev/yalz77/src/a9a8fab4ba706b5633...

https://bitbucket.org/tkatchev/yalz77/src/a9a8fab4ba706b5633...

[+] wmu|11 years ago|reply
Fnv is well-know hashing function, like CRC. Names in the code are not the main problem.

1. Structure of the code is a mess: everything lives in the same namespace, while some stuff could be a private properties of classes/structs. 2. Why structs rather than classes? 3. Why user is forced to use std::string? (template parameter, please) 4. Why user can't plug own hashing function (maybe better or faster)? 5. Author assumed that "unsinged char" has 8 bits - std::uint8_t is the valid type. 6. There is no abstraction for integers encoding, just hard-wired varint encoder.