iamOgunyinka/tinydircpp — pull request #1 by sehe (2014-02-02 03:56:40+00:00)
Title: refactorings after of review comments

So, since you nicely asked for review, and I had some time to spare (well, actually not this much, but I got hooked), please find here my ideas.

The findings
-----
So what did I find:

* no namespaces
* no const correctness
* no member initialization
* C style interface with pointers everywhere
* no exceptions
* no exception safety
* redundant counts for vector (and racy at that)
* broken relational operator on tinydir_file
* errno being assigned to
* gratuitously reading directory **twice** in tinydir_open_sorted
* assigning false to size_t
* function definition in header file, no header separation for tinydircpp.h

* bad interface design (unclear naming, conflicting goals/usage patterns)
* inefficient design (no iterator/callback interface, forcing allocation of all dir entries up front)
* ibid.: path duplicated across all tinydir_file objects

* lack of guards (assuming filenames without path separators, not checking for embedded NUL chars)
* buggy string handling: setting `path[path.size() - 2] = '\0';` instead of resizing
* bad logic (checking for file extensions matches in the middle of names as well)

* bad style: having local variables with the same name as member variables and with slightly different meaning
* bad style: many redundant pieces of code (clearing files, closing dirs, setting paths)
* bad style: implicit indirect includes (algorithm for stable_partition)

This is outside the API reservations I had noted with this commit: https://github.com/iamOgunyinka/tinydircpp/commit/a484a041f1f918d15fad42e22a27e878fb652e42#commitcomment-5248483

So, without further ado find my pull request that I tested on linux... It **should** still work on Windows.

Now, there is

0. no more C artefacts (bail, close, tinydir_file* etc.)
0. no more `vector`; no longer a requirement to store all files in a big container first
0. much more flexible interface (filter for heterogenous file types at once, filter for other characteristics than the extension).
0. vastly reduced public interface
0. 28% code reduction (145 LoC reduced), much cleaner headers

Remaining issues I can think of now would be

* FileHandler is now a class with too few responsibilities. Each member function could be a static function that just takes the `source` directory as well
* Error handling should be improved (right now, encountering (broken) symlinks will throw e_noent)