How to transition from bad code to good code.
Moderator: General Moderators
- RobertGonzalez
- Site Administrator
- Posts: 14293
- Joined: Tue Sep 09, 2003 6:04 pm
- Location: Fremont, CA, USA
Not as a flat out rule, but often you'll find that the review will come down to "Did it pass the test? Yes? Code review complete." 
Code standards (indentation etc.) is somewhat null and void here, as we all have beautifiers that display code how we want to read them. Plus we aren't particularly fussy about it, providing it is readable we don't particularly care.
Code standards (indentation etc.) is somewhat null and void here, as we all have beautifiers that display code how we want to read them. Plus we aren't particularly fussy about it, providing it is readable we don't particularly care.
- CoderGoblin
- DevNet Resident
- Posts: 1425
- Joined: Tue Mar 16, 2004 10:03 am
- Location: Aachen, Germany
Test Driven Development (TDD) is good and useful, especially if everybody knows it already and it comfortable using it BUT when looking at a total rewrite anyway with a limited timescale it may not be possible to acclimatise everyone to TDD. It is after all a whole new way of thinking, not just coding. Coding style does have an important part to play even if it is as simple as everyone using the same development tools such as eclipse with the same options (tabs to spaces etc). Things should be annotated to a sufficient level etc. etc.
I have looked at TDD and realise its usefulness but still cannot get my head around it (especially mocking database requirements which seem to make up the bulk of things I do). When I started coding I seem to remember being taught that 80% of code is input/output with 20% actual processing. TDD is great for that 20% but I remain unconvinced of it's place for the rest.
I have looked at TDD and realise its usefulness but still cannot get my head around it (especially mocking database requirements which seem to make up the bulk of things I do). When I started coding I seem to remember being taught that 80% of code is input/output with 20% actual processing. TDD is great for that 20% but I remain unconvinced of it's place for the rest.
To be honest - and I realise the bitter Irony this post will have to those who remember my tantrums - coding style plays very little to anyone but nitpicks or pedants. If you can read the code, who cares if they used Dreamweaver, Eclipse or even notepad? Who cares if it's tabs or spaces? Does it stop your applications working? Does it restrict the effectiveness or efficiency of the code? Naming conventions, yes, whole heartedly agree, but why fuss to such granualr details like spaces vs tabs?CoderGoblin wrote:Coding style does have an important part to play even if it is as simple as everyone using the same development tools such as eclipse with the same options (tabs to spaces etc). Things should be annotated to a sufficient level etc. etc.
It really is, 100%, a mere semantic that should not get in the way of a developer being afforded the ability to work comfortably and be creative; if (good) code gets rejected just because a developer forgot to enable tabs-for-spaces, so he or she must go back to their workstation and find-replace them all, you are wasting time.
And regarding the rest, yes you are right, B/TDD is not something you can simply jump into, but in all scenarios I've been in, you cannot start learning soon enough. It is by far not an overnight change, and even if you had a team of developers who were all masters of B/TDD it would not be an overnight change. In fact it's probably better for your sanity if anything else to gradually introduce B/TDD. The approach we have taken here, is that for every bug/change that is introduced - a test case is created for the old code/functionality which is designed to pass with what the code does, then it is reviewed to see where the failure is, then it is changed, and finally the code refactored to pass the changed test. That test is then added to our repository and will remain as a tool for ensuring other changes do not break that particular module. Areas that are unlikely to be touched in this process are tested when a developer has free time.
There is always an excuse not to start - "I'm too busy." "It's too late for this project to begin using B/TDD" etc. Now you can swap B/TDD for anything that indicates change and you'll start to see a recurring pattern. Meanwhile the developers trudge along and continue with legacy code.
- CoderGoblin
- DevNet Resident
- Posts: 1425
- Joined: Tue Mar 16, 2004 10:03 am
- Location: Aachen, Germany
Main thing I think we agree on is that the code must be readable and understandable.Jenk wrote:To be honest - and I realise the bitter Irony this post will have to those who remember my tantrums - coding style plays very little to anyone but nitpicks or pedants. If you can read the code, who cares if they used Dreamweaver, Eclipse or even notepad? Who cares if it's tabs or spaces? Does it stop your applications working? Does it restrict the effectiveness or efficiency of the code? Naming conventions, yes, whole heartedly agree, but why fuss to such granualr details like spaces vs tabs?
Jenk wrote:There is always an excuse not to start - "I'm too busy." "It's too late for this project to begin using B/TDD" etc. Now you can swap B/TDD for anything that indicates change and you'll start to see a recurring pattern. Meanwhile the developers trudge along and continue with legacy code.
It's an unfortunate fact of life that "management" has a requirement to get things out the door and money in the bank. If you are lucky you have management that understands and allows you some freedom. If not you have to investigate things like TDD on your own. In any company (and the larger normally the worst) you have the issue of cultural change and this is where the main sticking point comes in. Your TDD development will be slower when you start and unfortunately the "Return Of Investment (ROI)" isn't something you can easily put a figure on but this is what most "non-tech" based managers (and indeed a lot of even techie ones) use to justify things. It's company life unfortunately.
The main thing I hope we can all agree on is that:
* Everybody should be upgrading from PHP 4 to 5 if they get a chance
* Everbody should at least look at Test Driven Development and PHP Design Patterns.
You've hit the nail on the head with regards to cultural change. Especially management wanting to put a figure on anything and everything, which in cases like B/TDD, Scrum, Agile it's very hard to do. Fortunately the company I work for are only small, so there is only a handful of people to convince and it was painful, but we were successful in the end.
Ken Scwaber's presentations made a lot of difference. 
- RobertGonzalez
- Site Administrator
- Posts: 14293
- Joined: Tue Sep 09, 2003 6:04 pm
- Location: Fremont, CA, USA
- John Cartwright
- Site Admin
- Posts: 11470
- Joined: Tue Dec 23, 2003 2:10 am
- Location: Toronto
- Contact:
- RobertGonzalez
- Site Administrator
- Posts: 14293
- Joined: Tue Sep 09, 2003 6:04 pm
- Location: Fremont, CA, USA
Ok, I am lost a little bit then. Don't we all want high efficiency, bullet proof code that not only works, but works well and securely? If the only review of the code is not even a code review but a test result review, how does that ensure the state of your code? Or am I looking at this the wrong way?Jcart wrote:Thats the whole point. You don't want to change the behavior of the code, you just want to change how you came about the end result.Everah wrote:I might just be too old school, but can't crappy code still "work" and pass tests?
Because your tests need to be solid, hence my emphasis on reviewing test cases.
B/TDD also stresses that you are not testing for passes, you are testing for failures. Tests/Specs can also include time to run etc. and should most definitely include security tests. Penetration testing involves a lot of automated testing, so why can't B/TDD?
Need something to escape mysql data? Write a Test that throws every dodgy character under the sun at it.
If you start coding for a test case, and it passes first time - that is your worst case! You must then go through the test case and ensure it is a sound test once again, else you risk the chance of having a crappy test.
B/TDD also stresses that you are not testing for passes, you are testing for failures. Tests/Specs can also include time to run etc. and should most definitely include security tests. Penetration testing involves a lot of automated testing, so why can't B/TDD?
Need something to escape mysql data? Write a Test that throws every dodgy character under the sun at it.
If you start coding for a test case, and it passes first time - that is your worst case! You must then go through the test case and ensure it is a sound test once again, else you risk the chance of having a crappy test.
- Kieran Huggins
- DevNet Master
- Posts: 3635
- Joined: Wed Dec 06, 2006 4:14 pm
- Location: Toronto, Canada
- Contact:
- RobertGonzalez
- Site Administrator
- Posts: 14293
- Joined: Tue Sep 09, 2003 6:04 pm
- Location: Fremont, CA, USA
- Kieran Huggins
- DevNet Master
- Posts: 3635
- Joined: Wed Dec 06, 2006 4:14 pm
- Location: Toronto, Canada
- Contact:
- Christopher
- Site Administrator
- Posts: 13596
- Joined: Wed Aug 25, 2004 7:54 pm
- Location: New York, NY, US
I think what Kieran said is just plain crazy!
Code review is the first step toward pair/team programming. A second set of eyes and a second brain is what has repeatedly been shown to produce better software. It is really one programmer in a million that produces great software alone.
Likewise I don't see how either way makes unit tests "do virtually nothing." This conversation has veered out of reality into conceptual-land! Unit tests are the thing that provides the support for refactoring. Shared code ownership, testing and refactoring are foundations of Agile. More eyes produce better designs. Unit tests give the courage to mercilessly refactor. Refactoring improves code. Toss in Continuous Integration and we have a winning methodology.
If the OP had said that the code base was crap, but it had pretty good unit test coverage -- we wouldn't be having this discussion. We would just tell jamesmm to start refactoring. See the difference.
Likewise I don't see how either way makes unit tests "do virtually nothing." This conversation has veered out of reality into conceptual-land! Unit tests are the thing that provides the support for refactoring. Shared code ownership, testing and refactoring are foundations of Agile. More eyes produce better designs. Unit tests give the courage to mercilessly refactor. Refactoring improves code. Toss in Continuous Integration and we have a winning methodology.
If the OP had said that the code base was crap, but it had pretty good unit test coverage -- we wouldn't be having this discussion. We would just tell jamesmm to start refactoring. See the difference.
(#10850)
- Kieran Huggins
- DevNet Master
- Posts: 3635
- Joined: Wed Dec 06, 2006 4:14 pm
- Location: Toronto, Canada
- Contact:
Maybe I should have been more verbose... I don't mean to imply that code review is useless and that unit tests are ineffectual. Not at all.
The original question was "why still do code review if unit tests ensure everything will work anyway". I was arguing for keeping code review on the grounds that it also helps developers learn skills and techniques from each other in a peer environment, hopefully making better developers in the process.
I won't do a job I can't learn something from.
The original question was "why still do code review if unit tests ensure everything will work anyway". I was arguing for keeping code review on the grounds that it also helps developers learn skills and techniques from each other in a peer environment, hopefully making better developers in the process.
I won't do a job I can't learn something from.