Compiz

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

compiz.fix_1063617.7
Merge into 0.9.9

Revision history for this message

Sam Spilsbury (smspillaz) wrote on 2012-12-04:

Thanks for the advice.

I'll have another look today to see where we can simplify some of the class
names (they are a bit lame).
On Dec 4, 2012 1:29 PM, "Daniel van Vugt" <email address hidden>
wrote:

> Review: Approve
>
> Identifiers too long:
> * "Initialize" should be "Init"
> * "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".
> * Many parameters are excessively descriptive and sometimes misleading.
> For example "copyToValue"/"copyFromValue" could be better represented as
> nouns: "dest"/"src".
> * 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.
> * "WithParamInterface" as a template name. How does the "With" prefix
> make sense?
> * "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.
> * "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.
>
> 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.
>
> 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.
>

Thanks for the advice.

I'll have another look today to see where we can simplify some of the class
names (they are a bit lame).
On Dec 4, 2012 1:29 PM, "Daniel van Vugt" <daniel.van.vugt@canonical.com>
wrote:

> Review: Approve
>
> Identifiers too long:
>   * "Initialize" should be "Init"
>   * "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".
>   * Many parameters are excessively descriptive and sometimes misleading.
> For example "copyToValue"/"copyFromValue" could be better represented as
> nouns: "dest"/"src".
>   * 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.
>   * "WithParamInterface" as a template name. How does the "With" prefix
> make sense?
>   * "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.
>   * "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.
>
> 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.
>
> 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.
>

« Back to merge proposal