Check-In Metrics and Code Reviews


Check-in metrics may include

  • test coverage,
  • duplicated code,
  • cyclomatic complexity,
  • afferent coupling (the number of other packages that depend upon classes within the package as an indicator of the package’s responsibility)
  • efferent coupling (the number of other packages that the classes in the package depend upon as an indicator of the package’s independence)
  • Instability (I): The ratio of efferent coupling (Ce) to total coupling (Ce + Ca) such that I = Ce / (Ce + Ca). This metric is an indicator of the package’s resilience to change. The range for this metric is 0 to 1, with I=0 indicating a completely stable package and I=1 indicating a completely unstable package.
  • warnings,
  • code style

When running tests, run each set in entirety so developer sees everything they expect to fix: first, all build errors.  If possible, next all unit tests; if possible, component testing, etc.

Production lines work on basis of assumption that previous step was correctly executed- this allows parallel work.  If the assumption is proven wrong, the work based on that assumption is discarded.

Whatever metrics are chosen to measure development or tester progress, beware that people will inevitably work to the measurement: if developers are rewarded based on the rate at which they produce lines of code, they will create unnaturally large solutions.

Code Reviews

There’s no substitute for having someone who really knows how to programme reviewing the code.  If no such person exists in the team, find someone. CUTTING CORNERS HERE WILL COST YOU CONSIDERABLY MORE WHEN THE PROBLEMS ARE DISCOVERED AGAIN LATER ON.  Code reviews are the single, cheapest thing that can be done to bring down the total project cost and timescale.  Treat the following as an aide memoire rather than a substitute for knowing what to do.
1) business logic is only in the middle tier
2) business logic is completely covered by unit tests for ALL edge cases
3) business logic is completely covered by unit tests for happy path
4) business logic is completely covered by unit tests for ALL sad paths
5) business logic is completely covered by unit tests for all permutations of flow
6) all XML documents are present, useful and correct
 – if it tells you less than or no more than the method or parameter name, the comment is worse than useless
 – all exceptions thrown by the method must be documented
7) no magic numbers
8) No stringly typing
9) All dependencies MUST be externalised
 – Pay special attention for things like DateTime.Now and DateTime.Today – these are untestable and MUST be avoided
10) Think about the conversation between tiers. Is this code needlessly chatty?
11) Think about possible malicious exploitation of code.  NEVER ASSUME IT IS SAFE BECAUSE OF CIRCUMSTANCE because you will always be wrong.
12) Check that any suppressions added to production code are reasonable and justified
13) Check for warnings
14) Check to see if any previous unit tests have been deleted. ignored or otherwise moved out of scope
15) Generic lists are not exposed.
16) Check IDisposable is used correctly
17) Watch for thread safety
18) Look for code duplication – is inheritance being used correctly?
19) Do any methods have lots of parameters? It’s probably a God method
20) Do any methods do more than one thing? Refactor
21) Are any methods more than a screen of code? Refactor
22) Is there more than one class per file?
23) Are all fields private?
24) Have for-loops been used where LINQ could be used instead?
25) Has LINQ been resolved earlier than necessary?
26) Is arithmetic and conditional operator precedence obvious?
27) Is there any zombie code?
28) Has anyone used anonymous types?
29) Has anyone used anonymous methods?
30) Is interfacing atomic?
31) Is inheritance and interface use in line with Liskov?
32) Are any methods more public than necessary?
33) Do all events correctly follow subscriber/publisher pattern and/or use event aggregator?
34) Have methods been used where really properties should?
35) Do XML comments only describe behaviour? They should NEVER describe implementation
36) Has Exception been thrown directly? I hope not.
37) TODOs and NotImplementedExceptions are mutually inclusive
38) No extension methods on system.object.
39) Are generics used where appropriate?
40) Generics must not be nested
41) Tuples are PROHIBITED except where there is absolutely no alternative
42) No default parameters
43) no named parameters
44) Exceptions must be explicitly caught, never just a basic Catch
45) Properties must be read only or read-write, never write only.
46) Watch for hiding, reintroducing methods
47) Run cyclomatic complexity indexing
48) Consider afferent coupling
49) Consider efferent coupling
50) Cast cautiously and only when necessary
51) Consider what should be on the stack vs heap
52) No unused private fields or methods. EVER.
53) Rethrow correctly, preserve stack trace
54) Consider checked/unchecked
55) Consider NAN
56) Use T? in preference to nullable<T>

Leave a Reply

Fill in your details below or click an icon to log in: Logo

You are commenting using your account. Log Out /  Change )

Google+ photo

You are commenting using your Google+ account. Log Out /  Change )

Twitter picture

You are commenting using your Twitter account. Log Out /  Change )

Facebook photo

You are commenting using your Facebook account. Log Out /  Change )


Connecting to %s