Transcript Coding Standard & Code Review Milan Vukoje www.vukoje.net [email protected] Soprex      SkfOffice2 SkfOffice3 Big5 Quality oriented We are hiring… Themes      Is code important? Code Refactoring Coding Standard Code Review Tools.

Coding Standard & Code
Review
Milan Vukoje
www.vukoje.net
[email protected]
Soprex





SkfOffice2
SkfOffice3
Big5
Quality oriented
We are hiring…
Themes





Is code important?
Code Refactoring
Coding Standard
Code Review
Tools
Importance of code




mechanical process?
guaranteed to be done
50-65% of overall effort
50-75% of overall errors
Technical Debt




When software organization chooses a design
or construction approach that's expedient in
the short term but that increases complexity
and is more costly in the long term.
--Ward Cunningham
intentional debt (make up issues)
unintentional debt
Educate your managers
Coding Horror




Fear
Stress
Cargo cult programming/Just in case coding
Strange bugs




Heisenbug - bug that disappears or alters its characteristics when
an attempt is made to study it.
Mandelbug - bug whose causes are so complex that its behavior
appears chaotic.
Schroedinbug - bug that manifests only after someone reading
source code or using the program in an unusual way notices that it
never should have worked in the first place.
…
Refactoring

Refactoring (noun): a change made to the internal structure of
software to make it easier to understand and cheaper to modify
without changing its observable behavior.

Refactoring: Improving the Design of Existing Code
--Martin Fowler
Coding Standard
In a complex program, architectural guidelines give the
program structural balance and construction guidelines
provide low-level harmony, articulating each class as
faithful part of a comprehensive design… Without a unifying
discipline, your creation will be jumble of sloppy variations
in style. Such variations are essentially arbitrary. One key to
successful programming is avoiding arbitrary variations so
that your brain can be free to focus on the variations that
are really needed.
-- Steve McConell
The Book

Framework Design Guidelines: Conventions,
Idioms, and Patterns for Reusable .NET Libraries
-- Krzysztof Cwalina and Brad Adams
Soprex Coding Standard








Code Commenting
Naming
Code Layout
Type Design Guidelines
Member Design
Exception Management
Stored Procedures
.NET types usage
CS – Code Commenting



DO comment API.
DO NOT comment whole methods or method bodies. If some code is not
used, it should be deleted.
DO comment complex methods, especially if/else statements.
CS - Naming




If you are not able to come up with a right noun phrase you should rethink of
a general design of the type.
Most easily recognized names should be used for most commonly used
types.
Method names should describe what the method does and not how it is
implemented.
Names of derived types should contain, if appropriate, name of its base
type.
public interface ISessionChannel<TSession> where TSession: ISession
{
TSession Session {get;}
}

System.Delegate – Add suffix “EventHandler” to names of delegates that
are used in events. Add suffix “Callback” to names of delegates other than
those used as event handlers. Do not add suffix “Delegate” to a delegate.
CS - Type Design Guidelines

Choosing between class and structs

DO favour defining classes over interfaces.
AVOID using marker interfaces (interfaces
with no members).
DO NOT use public nested types as a logical
grouping construct; use namespaces for this.
DO favour using an enum over static
constants.



CS - Property Design






DO NOT provide set-only properties.
DO provide sensible default values for all properties.
DO allow properties to be set in any order even if this
results in a temporary invalid state of the object.
Remember there is a strong expectation that setting a
property is not much more expensive than setting a
field.
AVOID throwing exceptions from property getters.
Property getters should be simple operations and
should not have any preconditions. If a getter can
throw an exception, it should probably be redesigned
to be a method.
CS - Event Design




DO use System.EventHandler<T> instead of manually creating new delegates to be
used as event handlers.
DO use a protected virtual method to raise each event.
public class AlarmClock {
public event EventHandler<AlarmRaisedEventArgs> AlarmRaised;
protected virtual void OnAlarmRaised (AlarmRaisedEventArgs e) {
EventHandler<AlarmRaisedEventArgs> handler = AlarmRaised;
if (handler ! = null ) {
handler (this, e);
}
}
DO NOT pass null as the event data parameter when raising an event. You should
pass EventArgs.Empty.
DO use a return type of void for event handlers
CS - Exception Throwing










DO NOT return error codes.
DO report execution failures by throwing exceptions.
DO NOT use exceptions for the normal flow of control, if possible. (Tester-Doer
pattern)
DO create and throw custom exceptions if you have an error condition that can be
programmatically handled in a different way than any other existing exception.
DO document all exceptions thrown by publicly callable members because of a violation of the
member contract (rather than a system failure) and treat them as part of your contract.
DO NOT create and throw new exceptions just to have your team’s exception.
DO NOT throw or catch System.Exception.
DO provide a rich and meaningful message text targeted at the developer when throwing an
exception.
DO throw an InvalidOperationException if the object is in an inappropriate state.
DO throw ArgumentException or one of its subtypes if bad arguments are passed to a member.
CS – Exception Handling






DO NOT catch exceptions that you can not handle.
DO NOT cache exceptions just to rethrow them (without handling)
CONSIDER wrapping specific exceptions thrown from a lower layer
in a more appropriate exception, if the lower layer exception does
not make sense in the context of the higher layer operation.
AVOID catching and wrapping nonspecific exceptions.
This is often undesired and is just another form of swallowing errors.
DO specify the inner exception when wrapping exceptions.
CS - .NET types usage






DO NOT use weakly typed collections in public APIs.
DO use ReadOnlyCollection<T> or a subclass or ReadOnly-Collection<T>
for properties or return values representing read-only collections.
DO NOT return null values from collection properties or from methods
returning collections. If there is no items for collections return empty
collection. This rule implies to DataTable type also.
DO NOT return snapshots collections from properties. Properties should
return live collections.
For string manipulation StringBuilder class should be used because it is far
more efficient than calling methods on String class (e.g. string
concatenation).
DO use System.Uri to represent URI and URL data instead of string.
CS Advantages







Unified code
Faster code understanding/modifying
Easier “code smell” identification
Cleaner API
Avoiding “genius” solutions (KISS)
Reducing chances for bugs
real working framework
Adopting the standard




Real challenge
Needs support in upper management and
team leader determination
Should everybody comply?
Redefine what does “done” mean
Getting clean… code






Start small
Add rules incrementally
Divide and conquer
Use tools
Intensive code reviews
Fight the Klingons!
Code Review


Formal CR and troubles with it
Informal CR






Coding time reviewing
Pair programming
Code testing with code inspections
Two heads are smarter
collective code ownership
Enhances communication and learning
Code Reveiw
What to review?






Newbie's code
Challenging tasks
Spikes
Buggy code
Architecture significant tasks
Widely reused code
Code Review Checklist








Clarity
Maintainability
Accuracy
Security
Scalability
Reusability
Efficiency
OOP principles, encapsulation
CR challenges




Keep it integrated
Don’t overdo it – Perfect is the enemy of
good, and good is what we want.
Encourage positive critics
Effective CR meetings
CR meetings





Focus on enhancing code rather than
criticism
Integrate with refactoring
Create refactoring issue or add TODO
comments
Avoid arguments about coding in general and
theoretical discussion
Discover bugs and issues early
Available Tools








VS refactoring features
VS compiler warnings
Code Analysis
StyleCop
TFS check in policy
Great way to learn .NET
Performance issues
Suppressions
Pitanja?
Milan Vukoje
www.vukoje.net
[email protected]