Friday, September 11, 2009

What's Wrong with this Code?


I am working on a new presentation entitled "Software Craftsmanship for Working Class Developers" where I want to talk about this movement and the difficulties most of us have implementing its ideals in the average work place. The last exercise I want to do in the presentation is a group refactor of some actual code (with details and names changed to protect the guilty). In preparation I have shown it to several co-workers and asked them to tell me what is wrong with the code and what would they do to fix it. If I do this with no real preamble it is interesting how many different responses I get that even included "I see nothing wrong with it."

So I'd like to post this little demo project for people to download and then tell me what is wrong with it and refactor it to something more adherent to the principles we all know we should follow, but sometime don't always get the chance.

Here is a little background on the code. The Order class is used to hold data for an order (for what does not really matter to the task at hand) for a given Customer. We assign the Order to a specific Customer by assigning it a Customer number which is a 3 digit alpha-numeric identifier. We serialize these Orders and drop them into a directory for storage and processing. The Orders are given a sequential 3 digit order number based on the last Order in the drop directory.


This is some what based on a real situation and when you download the project you will see that it has a few unit tests (and I use that term loosely) that pass. The code works. It does exactly what it is supposed to do. So when you start refactoring remember that you would be having to justify every change to management who is currently perfectly happy with how it functions. I've put a subdirectory called Refactored.1 where you can put your refactored version and there is a text document that has a list I already started with what is wrong (IMHO) with the current code and feel free to add to it.


I plan to eventually post all the solutions that get sent back to me and use some of the examples in my presentation. I may even try to get a cool prize to giveaway to one of the people who submit via random selection. Happy coding!


You can download the demo project (C#) here: http://www.tommynorman.info/files/soliddemo.zip

5 comments:

Tom said...
This comment has been removed by the author.
Tom Hines said...

Here are some modifications I have made. Please delete the links if they do not belong here.

http://beltontroop111.org/Resources/SOLIDDemo_T.zip
http://tomhines.wordpress.com/files/2009/09/whatswrongwiththiscode-txt.pdf
http://tomhines.wordpress.com/files/2009/09/soliddemo-test-cs.pdf
http://tomhines.wordpress.com/files/2009/09/order-cs.pdf

Tommy Norman said...

Tom,
The links are fine and thanks for your submission. I was wondering why you removed all the MSTest attributes and went with a roll you own unit testing approach. Was there a reason? Do you use another testing framework normally?

Tommy

Tom Hines said...

I use VS2008 Standard (no internal test platform). I do have nUnit and could have "harnessed" the tests there.

As a sole-coder, I'm the only one who ever sees my code or tests (for good or bad).

Tom

chrismelinn said...

I have uploaded my modifications to:

http://cid-7f3aa0b5673d4b18.skydrive.live.com/self.aspx/.Public/SOLIDDemo%5E_Refactored.zip

Please let me know if you have any problems.