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

Revision history for this message
Daniel van Vugt (vanvugt) wrote :

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.

review: Approve

« Back to merge proposal