Abdelrazak Younes <[EMAIL PROTECTED]> writes:
> I would be very happy if you could review my code.
One thing I like about your code is that it makes the code of clients of
ParagraphList a lot more readable. One place where this isn't the case is in
undo.C where code like this suggests that you haven't yet hit on the perfect
interface.
--- undo.C 24 Nov 2005 16:22:39 -0000 1.69
+++ undo.C 8 Feb 2006 21:39:52 -0000
<at> <at> -106,7 +106,8 <at> <at>
advance(first, first_pit);
ParagraphList::const_iterator last = plist.begin();
advance(last, last_pit + 1);
- undo.pars = ParagraphList(first, last);
+ /// \todo verify this
+ undo.pars = ParagraphList(plist, first_pit, last_pit+1);
I'm in total agreement with Lars that you should proceed in small steps.
LyX is developer-driven, so you'll really wow the team by making the code
of users of ParagraphList more beautiful. Users care about performance,
so you'll wow them by using something other than vector<paragraph> as
the data store, but that's definitely a second step. Get the interface
right first.
Incidentally, there are two main differences between the way Lars and
I review code:
1. I'm less grumpy and more encouraging
2. My opinion counts less than Lars'
You've already shown me that you're a competent programmer with bundles
of energy, so I'd like to welcome you aboard and state that, despite
(maybe because of ;-)) the grumpiness, these guys have been a pleasure
to spend time with. I'm sure that you'll have fun here and you'll learn
as much as you contribute. The emphasis here is on writing quality code
and there's a strong belief in peer review. So, stick with it ;-)
One thing I'm less keen on in your code is your use of multiple typedefs
in the ParagraphList to mean the same thing
class ParagraphList {
public:
// Good
typedef std::vector<paragraph>::iterator iterator;
// Horrible
typedef std::vector<paragraph>::iterator PLI;
};
Ducking out again,
Angus
