Difference between revisions of "Education ClassRoom/Previous Logs/OpenOffice.org Coding Guidelines"
Line 286: | Line 286: | ||
[17:33] <thorsten> that *also* include all those bSomething vars, which are, in this case, | [17:33] <thorsten> that *also* include all those bSomething vars, which are, in this case, | ||
+ | |||
+ | [17:34] <thorsten> initialized *later* in the ctor body, for no real reason. | ||
+ | |||
+ | [17:34] <thorsten> it all boils down to maintainability - | ||
+ | |||
+ | [17:34] <thorsten> if you have all members listed in the initializer list, you can simply count them, | ||
+ | |||
+ | [17:35] <thorsten> and ensure that everything is properly initialized, | ||
+ | |||
+ | [17:35] <thorsten> and ensure that everything is properly initialized, | ||
+ | |||
+ | [17:35] <thorsten> was that halfway understandable? ;) | ||
+ | |||
+ | [17:36] <ssaboum> yep i got it (almost :-) ) | ||
+ | |||
+ | [17:36] <thorsten> cool, | ||
+ | |||
+ | [17:36] <metrokid> got, it | ||
+ | |||
+ | [17:36] <thorsten> so, checking for the "init" item, we'd suggested the code owner to slightly refactor this ctor, | ||
+ | |||
+ | [17:37] <thorsten> and move the bSomething initialization to the initializer list. | ||
+ | |||
+ | [17:37] <thorsten> next item to check: | ||
+ | |||
+ | [17:37] <thorsten> " Clear Origin of Local Data (LocalData)" |
Revision as of 16:37, 13 November 2008
[16:59] <thorsten> --- classroom in ~1 minute ---
[16:59] <thorsten> there's a little intro presentation - http://wiki.services.openoffice.org/w/images/7/72/C%2B%2B- codingstandards.pdf
[17:00] * fredus (n=fredus@138.219.77-86.rev.gaoland.net) has joined #education.openoffice.org
[17:00] <metrokid> thank you for the file
[17:00] <thorsten> ok, hi everybody,
[17:01] <metrokid> hi
[17:01] <ericb2> thorsten: hello. Thanks for accepting to manage this ClassRoom.
[17:01] <thorsten> I'd like to start with the ooo coding standards classroom now,
[17:01] <Guillaume[ecn]> Hello
[17:01] <ericb2> @ all :
[17:02] <ericb2> we use to start with ~20 to 30 minutes of presentation, then questions
[17:02] <ericb2> but things can be adapted, of course
[17:02] <ericb2> thanks for your attention, and let's start :-)
[17:02] <thorsten> thanks ericb2 for the invitation,
[17:03] <thorsten> let's start with a quick motivational presentation about why coding standards & what for ;)
[17:03] <thorsten> #slide 2:
[17:03] <thorsten> has the link to the c++ coding standards for OOo,
[17:03] <thorsten> which I'll mostly focus on
[17:04] <thorsten> there are coding guidelines for perl & java as well,
[17:04] <thorsten> but ~90% of OOo is C++, so... ;)
[17:04] <thorsten> everybody able to see the media?
[17:04] <Guillaume[ecn]> yep
[17:04] <thorsten> good :)
[17:04] <thorsten> slide #3
[17:04] <IZBot> no issue with number 3
[17:04] <IZBot> no issue with number 3
[17:05] <thorsten> haha
[17:05] <thorsten> so, you can read the content for yourself -
[17:05] <thorsten> there's a bunch of reasons to have coding standards,
[17:05] * metrokid reading Origins of Standards
[17:05] <thorsten> for me, one of the most important is to have a common vocabulary,
[17:06] <thorsten> and, especially for c++, to limit the variations, i.e.
[17:06] <thorsten> have a somewhat bounded problem space, when trying to express a certain kind of functionality
[17:06] <thorsten> like - use c-like, all-macros kind of coding vs. using boost -
[17:07] <thorsten> if that's mixed, people have an even harder time reading other's code
[17:07] <thorsten> a second thing is, and that's prolly best shown by this page:
[17:08] * mmu_man has quit ("Vision[0.9.7-Z-101305]: i've been blurred!")
[17:08] <thorsten> http://wiki.services.openoffice.org/wiki/Writer_Code_Conventions
[17:09] <thorsten> to give best-practice examples of how other people solved generic problems
[17:09] <thorsten> @ all: questions so far?
[17:09] <thorsten> cool.
[17:10] <thorsten> so, we based this on some standard literature, and experience we had in OOo,
[17:10] <thorsten> but, like, this did not fall from the skies :)
17:10] <thorsten> slide #4, btw
[17:10] <IZBot> no issue with number 4
[17:11] <thorsten> slide 5:
[17:11] <thorsten> you speak up when I'm too fast or too slow, ok?
[17:11] <Guillaume[ecn]> k
[17:11] <ssaboum> ok
[17:11] <thorsten> cool
[17:11] <thorsten> so,
[17:11] <metrokid> ok
[17:11] <thorsten> having rules is all nice & sweet, but
[17:12] <thorsten> if you don't actually use them, e.g. by reviewing code for them,
[17:12] <thorsten> there's mostly theoretical value in them :)
[17:12] <thorsten> thus, coding standards & code reviews go more or less hand in hand.
[17:12] <thorsten> first, you learn the rules by checking for them,
[17:13] <thorsten> second, when doing a review, you need something to check for, right?
[17:13] * aude86_2008 (n=aude86_2@138.219.77-86.rev.gaoland.net) has joined #education.openoffice.org
[17:13] <thorsten> slide 6:
[17:13] <thorsten> that's how a code review could work -
[17:14] <ssaboum> http://wiki.services.openoffice.org/w/images/7/72/C%2B%2B-codingstandards.pdf
[17:14] <thorsten> thx ssaboum
[17:14] <aude86_2008> thank you ssaboum
[17:14] <ssaboum> lol sorry (someone didn't hav it )
[17:14] <thorsten> we're on slide 6 - how a code review against the standards could work
[17:15] <thorsten> don't spend too much time on it, one hour has proven manageable,
[17:15] <thorsten> and take turns in reviewing.
[17:15] <thorsten> take the review as suggestion, not correction -
[17:16] <thorsten> if somebody spots something in your code, actually, you might be right -
[17:16] <thorsten> OTOH, if you spot something in other people's code, assume, that *he* might be right, not you,
[17:16] <thorsten> so:
[17:16] <thorsten> it helps to present reviews in a non-insulting way :)
[17:17] <metrokid> ^_^
[17:17] <thorsten> slide 7:
[17:17] <thorsten> we would love feedback on the rules,
[17:18] <thorsten> if you miss important stuff, or if you thing something is nowadays a no-brainer, let us know -
[17:18] <thorsten> or, change it right away, it's a wiki :)
[17:18] * frouvier (n=frouvier@43.215.194-77.rev.gaoland.net) has joined #education.openoffice.org
[17:18] <thorsten> any questions so far, about the motivation & goals?
[17:19] <thorsten> cool,
[17:19] <thorsten> then, I'd like to move over to a concrete example:
[17:19] <ssaboum> :-)
[17:19] <thorsten> could everybody please view this page:
[17:19] <thorsten> http://wiki.services.openoffice.org/wiki/Cpp_Coding_Standards/GEN
[17:20] <Guillaume[ecn]> done
[17:20] <thorsten> ok, these are 8 easily checkable rules,
[17:20] <thorsten> in fact, each one of those topic pages is suitable for a code review,
[17:21] <thorsten> i.e. don't review for everything (at first), go for one topic.
[17:21] <thorsten> please, everybody quickly skim over it,
[17:21] <thorsten> and in like 3 minutes, we'll try and review concrete code with that
[17:22] <thorsten> ok: code is here:
[17:22] <thorsten> http://svn.services.openoffice.org/ooo/cws/canvas06/svx/source/svdraw/svdobj.cxx
[17:22] <thorsten> and
[17:22] <thorsten> http://svn.services.openoffice.org/ooo/cws/canvas06/svx/inc/svx/svdobj.hxx
[17:23] <ssaboum> ouch ...
[17:23] <thorsten> we go for the class SdrObject ctor
[17:23] <thorsten> yeah, it's kinda nasty code :)
[17:24] <ericb2> comments in German :)
[17:24] <metrokid> Warum ?
[17:24] <thorsten> yep, the example is hand-picked for the effect
[17:24] <thorsten> metrokid: why what?
[17:24] <ssaboum> for the french people out there ctor is "contructeur"
[17:25] <metrokid> Kommentare in Deutsch
[17:25] <thorsten> metrokid asks why the comments are in German -
[17:25] <Guillaume[ecn]> thx for the translation ^^
[17:25] <thorsten> and the answer is, this is code from StarDivision time,
[17:25] <thorsten> when dev was exclusively German
[17:25] <ssaboum> nice...
[17:26] <ericb2> thorsten: is the order of the 8 rules http://wiki.services.openoffice.org/wiki/Cpp_Coding_Standards/GEN the suggested order, or is it a random order ?
[17:26] <thorsten> more or less random,
[17:26] <ericb2> thorsten: ok
[17:26] <thorsten> there is no real precendence
[17:26] <ericb2> thorsten: optimize later, maybe ? :)
[17:26] <thorsten> ok, everybody looking at SdrObject::SdrObject() now?
[17:27] <ssaboum> nopeµ....
[17:27] <fredus> nope
[17:27] <thorsten> let's check for "Initialize Everything Immediately (Init)" in that method,
[17:27] <thorsten> tell me when you're ready
[17:28] <ssaboum> can you give a line please ?
[17:28] * ChanServ gives channel operator status to sm|CPU
[17:28] <Guillaume[ecn]> 441
[17:28] <metrokid> yep, need it too
[17:28] <Guillaume[ecn]> i think...
[17:28] <thorsten> yep
[17:28] <ssaboum> thanks guillaume ...
[17:28] <fredus> thx
[17:28] * metrokid at 441
[17:29] <Guillaume[ecn]> np
[17:29] <thorsten> cool, so, anybody spotting any violations of the Init rule?
[17:29] <Guillaume[ecn]> mnLayerID(0),?
[17:29] <ssaboum> ok
[17:29] <ssaboum> done
[17:29] <Guillaume[ecn]> mpSvxShape(0)?
[17:30] <thorsten> well, _those_ are initialized,
[17:31] <thorsten> but what about e.g. aOutRect ?
[17:31] <thorsten> (that's in the header, line 444
[17:31] <ssaboum> humm
[17:32] <thorsten> well - that member is a class, and has a default ctor.
[17:32] <thorsten> but that's not very readable, and one has to know a lot of context,
[17:33] <thorsten> so, the recommendation is to explicitely initialize *all* class members in the class initializer list,
[17:33] <thorsten> which is this list starting in the cxx, line 441
[17:33] <thorsten> that *also* include all those bSomething vars, which are, in this case,
[17:34] <thorsten> initialized *later* in the ctor body, for no real reason.
[17:34] <thorsten> it all boils down to maintainability -
[17:34] <thorsten> if you have all members listed in the initializer list, you can simply count them,
[17:35] <thorsten> and ensure that everything is properly initialized,
[17:35] <thorsten> and ensure that everything is properly initialized,
[17:35] <thorsten> was that halfway understandable? ;)
[17:36] <ssaboum> yep i got it (almost :-) )
[17:36] <thorsten> cool,
[17:36] <metrokid> got, it
[17:36] <thorsten> so, checking for the "init" item, we'd suggested the code owner to slightly refactor this ctor,
[17:37] <thorsten> and move the bSomething initialization to the initializer list.
[17:37] <thorsten> next item to check:
[17:37] <thorsten> " Clear Origin of Local Data (LocalData)"