Re: [PATCH] code cleanup

Hi All,

I have cleaned this up after an email from Tor (see attachment) 
explaining what needs to be done to clean it up.
When something needs to be different then please let me know, as there 
are more a-like classes and I want to clean those up the same way, but I 
want to verify if this is ok before I change those.

--
Greetings,
Rob

Op 04-08-12 20:48, Gerrit schreef:
> >From Rob Snelders <libreoffice@ertai.nl>:
>
> Rob Snelders has uploaded a new change for review.
>
> Change subject: code cleanup
> ......................................................................
>
> code cleanup
>
> Change-Id: Ib154cd53253e4d802d13a024a20f6c34d499e672
> ---
> M svtools/inc/svtools/slidesorterbaropt.hxx
> M svtools/source/config/slidesorterbaropt.cxx
> 2 files changed, 132 insertions(+), 160 deletions(-)
>
>
>    git pull ssh://gerrit.libreoffice.org:29418/core refs/changes/68/368/1
> --
> To view, visit https://gerrit.libreoffice.org/368
> To unsubscribe, visit https://gerrit.libreoffice.org/settings
>
> Gerrit-MessageType: newchange
> Gerrit-Change-Id: Ib154cd53253e4d802d13a024a20f6c34d499e672
> Gerrit-PatchSet: 1
> Gerrit-Project: core
> Gerrit-Branch: master
> Gerrit-Owner: Rob Snelders <libreoffice@ertai.nl>
>
> _______________________________________________
> LibreOffice mailing list
> LibreOffice@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/libreoffice

I am sorry but I still have some critique of the patch... Please don't
take this too harshly.

Is this use of "IsDisposing" state variable really necessary? Or is
that band-aid protection against some bug (mismanagement of
dynamically allocated objects) elsewhere?

The use of the GetPropertyNames() static method seems a bit overly
complicated and duplicate to me; Is it really necessary to have this
method *and* local variables here and there initialised by calling it?
Could this be done in some simpler way? Can't this table just be a
static variable in the source file?

The SvtSlideSorterBarOptions::m_nRefCount thing seems a bit odd. The
use of the "reference count" term is misleading, it doesn't correspond
to what is normally thought of as reference counting, does it? (And if
it does, then we already have some mechanisms in LO to build reference
countig upon, no need to write your own.)

Is it really necessary to "hide" simple boolean fields behind public
setters and getters? Couldn't the fields just be made public instead?

The comments "We implement these class with a refcount mechanism!
Every instance of this class increase it at create and decrease it at
delete time - but all instances use the same data container! He is
implemented as a static member ..."  , "We use these class as internal
member to support small memory requirements. You can create the
container if it is neccessary. The class which use these mechanism is
faster and smaller then a complete implementation!" and "impl. data
container as dynamic pointer for smaller memory requirements!" make me
suspect you attempt some questionable micro-optimisation. I.e. make
the code a lot more complex for dubious gain in speed and size.

The use of naming like _impl suffix easily misleads the code reader
into thinking that it is the common Pimpl idiom that is used, while in
fact it is something else, a dynamically allocated block of static
variables? Or am I missing something? Anyway, what is the real gain
here? Does other similar code in LO use similar constructs?

The rest is just style issues,  if you can improve that too, that
would be great.

The capitalization of the description texts added to
officecfg/registry/schema/org/openoffice/Office/Impress.xcs is a bit
odd. Is the user and translators supposed to understand the difference
betwee "Slide Sorter" with a space and "SlideSorter" without space?

if (SvtSlideSorterBarOptions().GetVisibleImpressView()==sal_True)

should surely be written as

if (SvtSlideSorterBarOptions().GetVisibleImpressView()) , etc

The style of Doxygen (?) comments added is inconsistent; does /*@short
 really work, when elsewhere you seem to use /**   @short etc?
Sometimes you put the terminating */ right after the last word,
sometimes on a line by itself.

Why lots of spaces in front of semicolons in some places, to align
semicolons, but not consistently, and that is not something LO code
does elsewhere, is it?

OUString has a constructor that takes a string literal, doesn't it, no
need to use the RTL_CONSTASCII_USTRINGPARAM stuff I think.

In newly written code we shouldn't use sal_Bool and sal_Int32 unless
necessary; just bool and int is fine.

It is not clear who "we" and "you" in comments refer to. "we" = the
class in which the comment is located, "you" = code of subclasses?

--tml