Code review recommendations and Code Smells
Posted
by Michael Freidgeim
on Geeks with Blogs
See other posts from Geeks with Blogs
or by Michael Freidgeim
Published on Sat, 09 Jun 2012 07:19:52 GMT
Indexed on
2012/06/09
10:41 UTC
Read the original article
Hit count: 314
Filed under:
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.
I also found a link to a good book “Clean Code.A.Handbook.of.Agile.Software”
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.
I also found a link to a good book “Clean Code.A.Handbook.of.Agile.Software”
© Geeks with Blogs or respective owner
