Code review comment for lp:~compiz-team/compiz/compiz.fix_1063617.7

On Dec 4, 2012 1:29 PM, "Daniel van Vugt" <email address hidden>
wrote:
>
> Review: Approve
>
> Identifiers too long:
> * "Initialize" should be "Init"

I'll change that.

> * "Interface" and "Impl" are implied by the context and should be
obvious from the rest of the class name, if not the namespace. So you
should never need to say "Interface", "Impl" or "Base".

I agree with this, although the structural separation is necessary.

Either I can use the namespace trick, I'll give this a little more thought
and see what works.

> * Many parameters are excessively descriptive and sometimes misleading.
For example "copyToValue"/"copyFromValue" could be better represented as
nouns: "dest"/"src".

+1

> * ValueContainerPtr mDefaultValueContainer; ValueContainerPtr
 mNonDefaultValueContainer; ... I already know they are ValueContainers.
You rarely need to put the type name in the variable name. The variable
name is already half the description of a variable.

+1 . They can become mDefaultValue and mNonDefaultValue

> * "WithParamInterface" as a template name. How does the "With" prefix
make sense?

This is lame, but it comes from Google Test.

> * "MockInitializerFuncsWithDelegators" as a class name... The "With"
suffix is unusual and hinders readability because the primary noun is no
longer at the end (contrary to English grammar). Consider rewording
"WithWhatever" suffixes into a prefix of some sort.

This can become StubVerifiedSettingInitializers

> * "SemanticsParamFor" and "FailureSemanticsParamFor" are unreadable
because the types of the parameters dominate the function definition,
mostly due to templates. I don't have an understanding of the code to
suggest any alternatives right now, other than templates should only ever
be used scarcely.

Yeah, the only reason why templates were used was that there would be far
too much code repetition any other way.

>
> Finally, remember carefully chosen English is only half the problem in
readable code. The other half is realizing code is mostly symbolic. You
should see the structure and flow without having to read the words
themselves. And remember we recognize words or identifiers as symbols. So
if an identifier is made up of too many words it becomes unrecognisable as
a symbol and easily mistaken for some other identifier with similar
spelling.

Yeah, I agree with this. Sadly english is not too good at dealing with this
problem.

Have another look at this tomorrow, I'll fix up some of the names tonight.

>
> But enough said. I've said it all before.
>
> --
>
https://code.launchpad.net/~compiz-team/compiz/compiz.fix_1063617.7/+merge/136917
> Your team Compiz Maintainers is subscribed to branch lp:compiz.