Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

Here is my situation. After almost 2 years I was able to convince my boss to use TDD, version control, coding standards for the entire team and a MVC framework for our web projects.

I was finally there. It should be heaven but it's ending up like hell.

I prepared some material about MVC and code samples showing common unit tests. Showed to the team, explained how things should be done and got their support. It seemed all fine, after all, team members were very experienced.

I also made sure that the members had time to do things. At first, writing tests can take a very long time. So I tried to make them comfortable with "losing" this time without any manager asking them to do things quickly and dirty.

After 15 days I was doing the first review of the code. And the result was not good. I found business logic on controllers and on views and the tests folder were almost empty

How do you deal with these kind of things? Do you recommend any tools that can help me on code review or this is something that I have to do manually?

I'm using PHP with Zend Framework.

share
    
It requires a Google Apps account, but you may want to check out Google Code Reviews google.com/enterprise/marketplace/… –  Michael Mior Jul 7 '11 at 15:08
1  
Ho god ! Best question ever. I'm in the same position here. –  deadalnix Jul 7 '11 at 15:26
23  
15 days is way to long to go without reviewing code, try 2 days instead! –  Jarrod Roberson Jul 7 '11 at 15:35
1  
I really appreciate all answers. Unfortunately i can't choose more than 1, but you certainly opened my eyes to calm down and give time for the team to adapt with the new changes. I will also increase the frequency of code reviews probably with a tool like phpUnderControl as suggested. Thank you! –  Fernando Jul 7 '11 at 19:36
    
It has been almost three years since this question was asked: did your adjustments work? Did the project eventually adopt your recommendations and succeed in the end? Did the answers help? –  John Gaughan Apr 11 at 14:14
show 1 more comment

migration rejected from stackoverflow.com Apr 14 at 7:26

This question came from our site for professional and enthusiast programmers. Votes, comments, and answers are locked due to the question being closed here, but it may be eligible for editing and reopening on the site where it originated.

put on hold as too broad by gnat, Giorgio, MichaelT, Bart van Ingen Schenau, Kilian Foth Apr 14 at 7:26

There are either too many possible answers, or good answers would be too long for this format. Please add details to narrow the answer set or to isolate an issue that can be answered in a few paragraphs.If this question can be reworded to fit the rules in the help center, please edit the question.

12 Answers

up vote 60 down vote accepted

You implemented a totally new process and you expected it to be perfect on the first try?

I would be shocked if everything was even marginal on the first pass.

Calm down and go over what you see with the other developers. They may have questions and you may have not explained things very well to begin with.

Also remember that concepts like "Business Logic" can be very hard to understand and categorize. Understanding it takes time and practice.

So take time and practice.

share
3  
Ditto. I went through the same thing - it will be months before your team has fully adapted to this new process. Wish I could +10 this answer. –  Jarrod Nettles Jul 7 '11 at 15:31
4  
Nice answer, you have my bow, and my axe. –  apacay Jul 7 '11 at 15:55
2  
Perhaps also increase the rate at which you review the code. Waiting half a month until you have a look at it is a bit long when implementing something new. –  Sverre Rabbelier Jul 7 '11 at 22:25
add comment

I've never come across any tool which can determine if you have business logic outside of the business layer so we always do it as part of the code review process. The good thing is, as developers become more familiar with the correct development methods (which will only happen if code reviews are strict) the issues will be less and less. I'd also suggest that you should do your code reviews more frequently (at first especially) as after 15 days if the code is fundamentally incorrect there will be a lot of work to correct it and the develop will be disheartened whereas if the code review was done after a couple of days (even if the functionality was incomplete) any problems can be easily rectified.

What source control system are you using? There are tools available for TFS to prevent code being checked in unless there is a certain %age of code coverage. I'm sure similar things exist for other products so it may be worth looking in to them.

share
add comment

I was in a somewhat similar situation to yours, and my solution was mentoring via pair programming sessions.

Take time to sit down with each developer for a couple of hours, to do some development together. Lead them through implementing some small feature the TDD way using MVC properly. Discuss any questions which come up. Ideally, repeat a couple of times (like once a week), until you ensure that they internalize the new approaches. At first you might be required to take control altogether, but gradually you can give the lead back to them.

It requires patience. Changing the way people think is one of the most difficult things in life.

share
    
I was going to suggest pairing up your programmers so ensure quality. Ideally, a more experienced developer and a less experienced developer would be paired together to benefit the most from knowledge transfer. –  John Cartwright Jul 7 '11 at 15:44
add comment

There are two looks you should consider to hybrid:

  • The tangible look:

Is the code under Version control?

If it's not, that's the first step, then you have a Code Delta. So you can take a prcise vision of the implementations.

  • The methodology look:

Review meetings as in Scrum Agile Methodology can be useful as well, this makes your own team to ask themselves what have they done wrong, what have they done ok and what could be improved. I would recommend to do the other Scrum meeteings as well.

Applying TDD would be another choice, generating the test before you even start to code could be hard to understand at first, but one of the best possibilities once your team has revolutioned their mindset to this.

You should use both!

share
    
Scrum reviews are for reviewing general time management, not low-level code details. –  Lightness Races in Orbit Jul 7 '11 at 15:35
    
I disagree with you on this. Agile is not focusing on only one aspect of tracking what happens. Review meetings are not to make a tracking of the project but to improve the quality of what is delivered, for example. –  apacay Jul 7 '11 at 15:51
1  
If it takes you 3 weeks (or 1, or however long your sprints are) to discuss poor code, something is very wrong. By all means reflect, saying "we had several code reviews this sprint that found lots of problems; how can we stop this from happening again?". But do not leave actual code review for sprint review meetings. –  Lightness Races in Orbit Jul 7 '11 at 16:18
add comment

The solution is both simple and hard: Set and Enforce High Standards - Do not accept code that has minor defects.

Define your development process and enforce it - It your process says that all tests will be created prior to actual code development, do not allow code development until all of the tests are complete and accepted.

That you had a bad first code reveiw should be expected. It will take time for your developers to adapt to the new process. There will be some passive resistance (Things like not writing tests) you will need to overcome. It will be painfull and probably hurt morale in the short term. But once they start producing good code and are in the process they will either grow to like it or move on. Your team is probably full of professionals that appreciate the results and in the end understand the means.

share
add comment

The issue with logic in the controller and views that you're describing is Anaemic Domain http://martinfowler.com/bliki/AnemicDomainModel.html.

One of the most common reasons for unit testing falling by the wayside is them becoming dirty hybrid tests see http://blog.stevensanderson.com/2009/08/24/writing-great-unit-tests-best-and-worst-practises/. These become more trouble than they're worth maintaining. Turning these back into unit tests proper will help with them become more productive.

As you say, they're all experienced, so they will have been through learning processes before and changing is hard, so it seems like a matter of identifying:

  • the strengths of the work so far
  • the issues that they have faced
  • a common solution

together and work towards the solution.

share
add comment

Firstly, are you sure your idea of heaven is the same as the teams? If they've not bought into it, maybe the problem is you not them.

Assuming they have bought into it then you do have to give them plenty of time to get used to the new ways of working. I would hold a post-project meeting (or friday afternoons) to get everyone together to talk through how they like the new ways, use the codebase as a discussion point, then you can raise a section of code and say that you would expect the business logic seen in a view to have been put in a model instead, etc etc. You get to ask them why it was done like that and the answers shoud be informative.

The lack of tests in a TDD environment are another matter though, but again - ask why they didn't write them and listen to the answers.

share
add comment

TDD, coding standards and MVC are all hard things to do if people are not used to them. Put them all together and you have a recipe for a very steep learning curve. This was never going to be a smooth ride.

You might have been better off introducing these concepts one by one. The easiest is coding standards, so that's where I would have started. The way to make sure they are being followed is with peer reviews. That way people get used to getting critized (and helped at the same time) by their colleagues.

The move on to version control, and after that TDD. TDD is a concept that needs to be thoroughly understood and embraced before it can work. And the introduction of TDD into an existing project is an uphill battle, since there is so much work in writing all the tests for existing code. This alone can take several months just to get to a decent level of acceptance and coverage.

And finally MVC. Now that can be a complete paradigm change, especially for experienced people who have never used it before. Most people who don't use MVC write their applications with control coming from the user, whereas in MVC the user action just triggers something and then the changes propagate through the application by way of events. I have seen good, indeed otherwise very good coders who have never fully grasped the concept and its consequences. A lot of teaching will be required.

There aren't really any tools I can recommend for this. I presume you are using either simpletest or PHPUnit for the unit testing (or something similar). The code reviews are an inherently human thing, and you should not underestimate the team dynamics that come from the review sessions. Once again, this needs mentoring, mentoring and more mentoring. And lots of time.

share
add comment

I think the discipline that you're looking for is called Agile Coaching, from what I heard at a Meetup I attended a few months ago. The head coach (at an agile bank!) recommended two books, Coaching Agile Teams and Agile Coaching. She also recommended the blog of one of the latter books' authors.

I think you'd also benefit from some of Johanna Rothman's work, see her site for blog and book links.

Apart from chasing up those resources, I think it is even more important to get some training from an experienced coach.

share
add comment

Going 15 days without giving your developers feedback on how well they're learning the concept is a long time. A better way to approach this is probably to sit down with each developer often during the early part of introducing these concepts, and go over his code with him, so that he doesn't spend 15 days going off in the wrong direction before you can get him back on track.

share
add comment

Search for "Continuous Integration" over the Web. I use Hudson/Jenkins to run a set of quality tools every time source code is commited. Findbugs, Checkstyle, Sonar are great tools for quality control. These tools are very common in Java projects but i guess you can find similar ones for PHP.

Another suggestion is to define goals for your team like have 10% of test coverage, no more than 10 warnings detected by Findbugs and so on.

It´s hard to keep your team motivated doing Code Reviews over time. It´s a bit boring! A checklist to guide your reviewers and a tool like Codestriker should help. Better if you use a tool integrated in your IDE. Eclipse has some and can be used with PHP. Pair programming is a great alternative for tradicional Code Review too.

And be calm! Quality is a continuous process which you are going to be always identifying problems and solving them. Quality is important, don´t give up! :)

share
add comment

How do you deal with these kind of things?

by Branching and Reviewed-Merges.

Do you recommend any tools that can help me on code review ...

We're doing it with Git, but you can do it with any other decent Version Control software.

Problem

The problem that you had -- well, in 2011, wasn't about TDD or misunderstanding of MVC, or unqualified developers, etc. The problem was that you didn't have any control over the master branch. That means your team members can potentially commit Low Quality code to your master branch. That's the reason of your frustration, I assume.

Solution

What you need to do is:

  • First, only Code Reviewers or Lead Developer should have push permission to the master branch. So, master is read-only for the rest of team.
  • Then you should require your team to start off with a new Branch for every single change they want to apply. It could be a bug fix, a new feature or improving a previously written code.
  • They can freely work on their own-created branches. Other team members can also clone that branch to work on it, if needed.
  • Then Code Reviewers or Lead Developer should merge those branches into the master branch or ask them to improve the quality and then request for the code review again.

If you do this properly, then:

  • you haven't slow down the team as they can freely create their own branches and work on it with the rest of team. Whenever they're finished a branch they will ask for review and merge.
  • At the same time, they still can't push unverified changes to the master. That means Quality Control.
  • And you will have an on-demand Code Review work-flow; So as long as the code reviewers do their job correctly, you will have a neat and high quality code on your master branch.
share
add comment

Not the answer you're looking for? Browse other questions tagged or ask your own question.