Added Coding Conventions #342
Conversation
|
|
||
| Simple does not mean the fewest lines of code. Simple code is code: | ||
| * Easy to understand | ||
| * Self-documenting and not dependent on comments to explain what it does |
There was a problem hiding this comment.
We should not restrict people from writing comments. Self-Documenting is fine, but avoiding comments is a terrible idea.
There was a problem hiding this comment.
This doesn't say don't write comments. It says we prefer to write code that doesn't need them to explain it.
| To be able to cope with constant refactoring and evolving design, write code that: | ||
| * Is well tested. | ||
|
|
||
| Avoid test cases requiring detailed interactions with mocks because these are often brittle. |
There was a problem hiding this comment.
This line need more details or examples
CODING-CONVENTIONS.md
Outdated
| - Constructors should be simple, with dependencies passed in rather than built in the constructor. | ||
| - Pantheon does not use a dependency injection framework. | ||
|
|
||
| * Validates method parameters for public methods using the Guava `Preconditions` class. Avoid validating parameters in private methods. |
There was a problem hiding this comment.
We shouldn't restrict or discourage people from validating input. It's good defensive programming.
"private" is not a great indicator of whether you trust the caller. Especially when you consider the newbie who adds code 1 year from now and doesn't validate before calling your code.
It's my understanding that the only reason to avoid validation is to improve performance when validation gets called multiple times.
I would prefer to see something more like: "Be defensive. Validate all input with guava by default. If there is a specific reason to avoid it, add a comment instead explaining. eg a long, tight loop over known valid data is a waste of cpu." That way, future devs know why it was avoided, and know to look for pre-validation if they get a strange exception.
| ## 5.1 Log Messages | ||
|
|
||
| Make log messages: | ||
| * Not so frequent they are overwhelming in the log output |
There was a problem hiding this comment.
This should be removed as it is handled by the next line.
| * At the appropriate level | ||
| * Detailed enough to understand what actually happened. For example: | ||
|
|
||
| `Insufficient validators. Expected 10 but found 4` |
There was a problem hiding this comment.
Suggest more detail like: Insufficient validators. Expected 10 but found 4, and here they are: [list of validators]
There was a problem hiding this comment.
In practical experience actually including the full list of validators was completely overwhelming in log output.
|
|
||
| * As succinct as possible while still being clear. | ||
|
|
||
| * Bad: `Insufficient validators. Expected 10 but got: [address, address, address, address, address, address]` |
There was a problem hiding this comment.
No! This is an excellent. This should absolutely be there. It is far easier to track down what is missing when the data is printed in the log message.
PR description
Added Coding Conventions