Code review recommendations and Code Smells

Some time ago Twitter told that I am similar to Boris Lipschitz . Indeed he is also .Net programmer from Russia living in Australia.

I‘ve read his list of Code Review points and found them quite comprehensive. A few points were not clear for me, and it forced me for a further reading.

In particular the statement “Exception should not be used to return a status or an error code.” wasn’t fully clear for me, because sometimes we store an exception as an object with all error details and I believe it’s a valid approach. However I agree that throwing exceptions should be avoided, if you expect to return error as a part of a normal flow.
Related link: http://codeutopia.net/blog/2010/03/11/should-a-failed-function-return-a-value-or-throw-an-exception/

Another point slightly puzzled me
“If Thread.Sleep() is used, can it be replaced with something else, ei Timer, AutoResetEvent, etc” . I believe, that there are very rare cases, when anyone using Thread.Sleep in any production code. Usually it is used in mocks and prototypes.

I had to look further to clarify “Dependency injection is used instead of Service Location pattern”.
Even most of articles has some preferences to Dependency injection, there are also advantages to use Service Location. E.g see http://geekswithblogs.net/KyleBurns/archive/2012/04/27/dependency-injection-vs.-service-locator.aspx.
http://www.cookcomputing.com/blog/archives/000587.html refers to Concluding Thoughts of Martin Fowler
The choice between Service Locator and Dependency Injection is less important than the principle of separating service configuration from the use of services within an application

The post had a link to excellent article Code Smells of Jeff Atwood, but the statement, that “code should not pass a review if it violates any of the code smells” sound too strict for my environment.

In particular, I disagree with “Dead Code” recommendation “Ruthlessly delete code that isn't being used. That's why we have source control systems!”. If there is a chance that not used code will be required in a future, it is convenient to keep it as commented or #if/#endif blocks with appropriate explanation, why it could be required in the future. TFS is a good source control system, but context search in source code of current solution is much easier than finding something in the previous versions of the code.

Related links:

http://stylecop.codeplex.com/

http://janvanderhaegen.wordpress.com/2011/11/04/coding-is-optional-a-random-walk-down-oo-road-less-is-morev

http://www.codinghorror.com/blog/2006/01/flattening-arrow-code.html

http://janvanderhaegen.wordpress.com/2011/11/04/coding-is-optional-a-random-walk-down-oo-road-less-is-morev

Posted on Saturday, June 9, 2012 6:19 PM .Net Framework , General Tips | Back to top

Comments on this post: Code review recommendations and Code Smells

# re: Code review recommendations and Code Smells

Mark Seemann articulated really well the issues with Service Locator pattern. Check this out: http://blog.ploeh.dk/2010/02/03/ServiceLocatorIsAnAntiPattern.aspx

Left by Boris on Jun 14, 2012 1:34 PM