On 7/26/06, Allen Bierbaum <al...@vr...> wrote:
> I've just been running through the matcher code to try to track down an
> error in my bindings and I have a couple of questions. Note these
> questions have to do with why certain coding decisions were made so
> hopefully you are open to some friendly code critiquing. :)
I will try :-). It is almost midnight here.
> - Why is the file declarations/filters.py not called
> declarations/matchers.py?
Probably mistake, and this could be fixed.
> This file defines the matcher_base_t class and all matchers that derive
> from that class. It is the place in the code where all matchers are
> defined and every class in that file is called *_matcher_* but yet it is
> called filters.py. Seems confusing to me.
You are right.
> - declarations/matcher.py
>
> This file seems to just define 3 static methods for calling find with
> matchers and the 2 associated exceptions that can be thrown. Why define
> the class "matcher" when just having these methods and exception types
> in the file matcher.py already creates the namespace it seems like you
> want them to be within?
Yet another level of indirection. __init__.py file imports everything in it.
I consider location of classes to be implementation details.
> For that matter, why does this file even need to
> exist? The only place that calls these methods is scopedef_t so the
> methods could either move over to there or could move into the same file
> where all the matchers are defined (filters.py right now, but
> matchers.py may make more sense as described above).
May be because I program in C++ to much? Hint: std::algorithm header
> Anyway, those are my comments. Please let me know if this type of
> critique is helpful, if it is I will write a message when I see this
> type of thing in the code.
Yes it is. It will take some time until py++ will have good documentation.
The source code should be clear! If you are confused, than other users too.
And in this case the code should be fixed.
Thank you.
--
Roman Yakovenko
C++ Python language binding
http://www.language-binding.net/
|
