[Ctk-developers] Libs/DICOM/Core/ctkDICOMModel.cxx : variables names with same naming as method names

[Luis Ibanez]

Julien,

I like your suggestion better than mine.

The three approaches that you mentioned will
make code much readable and maintainable.

It will be nice to collect coding style guidelines
somewhere... at least in a Wiki,... or in a text
file to be included in the git repository.


     Luis


-------------------------------------------------------------------------------
On Thu, Apr 8, 2010 at 6:31 PM, Julien Finet <julien.finet at kitware.com> wrote:
> Agree, I'm not a big fan of the 2 solutions you gave though.
> When I remember to avoid variable shadowing, I have a couple of tricks to
> rename problematic variables:
> for example a variable named "row" could be renamed as:
>  - indexRow / itemRow / cellRow / childRow / parentRow  ( who owns the
> variable)
>  - rowToInsert/rowToDelete (action on the variable)
>  - newRow / oldRow / tmpRow ( lifetime of the variable)
>  - ....
> There is almost always one prefix or suffix that can be added and gives more
> information about the variable.
> Julien.
> On Thu, Apr 8, 2010 at 6:15 PM, Luis Ibanez <luis.ibanez at kitware.com> wrote:
>>
>> I see,
>>
>> It is too bad that Qt got satisfied with a lower quality bar.
>>
>>
>> We can still avoid having local variables with the same
>> names as member variables and member methods.
>>
>> For example,
>> many of the offending warnings were fixed by replacing
>>
>>     ambiguousvariablename
>>
>> with
>>
>>     ambiguousvariablenameValue
>>
>> we could have as well used the more arcane
>>
>>     ambiguousvariablename_
>>
>>
>> Making good choices on names of methods, and variables
>> is a an effective way of documenting the code.
>>
>>
>>    Luis
>>
>>
>>
>> -----------------------------------------------------------------------------------
>> On Thu, Apr 8, 2010 at 6:08 PM, Julien Finet <julien.finet at kitware.com>
>> wrote:
>> > Hi Luis,
>> > I understand your concern.
>> > We can't change the name of the method index() as it is a virtual method
>> > that has been declared in a Qt subclass.
>> > And the problem we face here is that the Qt library doesn't care about
>> > shadow variables. (and if I recall, they didn't plan on changing that).
>> > We unofficially decided during the CTK Pre Hackfest that a CTK class
>> > should
>> > follow the naming convention of its derived class, Qt in this example.
>> > Is that still OK for everybody ?
>> > Setting up KWStyle seems like a good idea. Can we configure it in a way
>> > to
>> > check a style depending on a filename (ideally the style of the derived
>> > class)
>> > Julien.
>> > On Thu, Apr 8, 2010 at 5:50 PM, Luis Ibanez <luis.ibanez at kitware.com>
>> > wrote:
>> >>
>> >> I recall that at some point we discussed a
>> >> naming convention for Qt-like classes, but
>> >> I'm not sure if we arrived to a consensus.
>> >>
>> >>
>> >> The file:
>> >>
>> >>  Libs/DICOM/Core/ctkDICOMModel.cxx
>> >>
>> >> had an abundant use of local variables with
>> >> names matching some member variables and
>> >> member methods.
>> >>
>> >> A notable example:
>> >>
>> >>                       "index"
>> >>
>> >> I just committed a fix for most of them,
>> >> but for the long run it will be great if we
>> >> agree on some naming convention.
>> >>
>> >> I would vote against having a method of a C++ class
>> >> to be called "index()" for example.       :-)
>> >>
>> >> a more useful name will be "getIndex()", for example,
>> >> or in this particular case, it looks like what the function
>> >> is actually doing is generating an index, so good names
>> >> could be
>> >>
>> >> ctkDICOMModel::generatgeIndex ( int row, int column, const QModelIndex
>> >> & parentValue ) const
>> >>
>> >> or
>> >>
>> >> ctkDICOMModel::computeIndex ( int row, int column, const QModelIndex &
>> >> parentValue ) const
>> >>
>> >> or
>> >>
>> >> ctkDICOMModel::produceIndex ( int row, int column, const QModelIndex &
>> >> parentValue ) const
>> >>
>> >>
>> >> There is already another
>> >>
>> >>                createIndex(int, int, Node *)
>> >>
>> >>
>> >> so it will be fine if we use
>> >>
>> >> ctkDICOMModel::createIndex ( int row, int column, const QModelIndex &
>> >> parentValue ) const
>> >>
>> >> since the signature will be different.
>> >>
>> >>
>> >> ---
>> >>
>> >>
>> >> Do we have a document on coding style ?
>> >>
>> >> if we do,
>> >> we probably should setup KWStyle testing
>> >> before things get out of control.
>> >>
>> >>
>> >>
>> >>      Luis
>> >
>> >
>
>