Ask HN: How do I get my team to write better code?
I'm leading a team of programmers - not all of whom I had a say in choosing. They're generally a smart lot when it comes to understanding the algorithmic & functional logic - but make beginner level mistakes like hardcoding thigns, naming functions like "findeventsfromthelast6hours" (when the 6 hours could be changed at some point - rendering the name of the function pointless). And so on...
I review commits now and then and find some of these issues - but due to the nature and timeline fo the project cannot do it for each and ever commit of course. So I'm wondering what I can do about this? Is there a short guide or book that I can recommend my team to read? I of course tell them these things myself too - but it gets frustrating because I keep feeling like a lot of this should really just be common sense.
86 comments
[ 2.2 ms ] story [ 158 ms ] threadSimply put: we use git and developers can't commit to the master branch. They commit to their own branch, the build system builds/tests that branch and the developer makes a pull request and gets the code reviewed.
1) You're more likely to catch bugs, security issues, and potential performance problems before they get into your main branch.
2) Your developers will get feedback on their code, which is hugely important to growth (you can't learn from your mistakes if you don't know you're making them).
3) Your developers will learn how to give feedback, and how to have constructive discussions about code by virtue of reviewing and commenting on pull requests.
4) I think there's some light, healthy social pressure when you know that other people are going to be doing code reviews. People seem to be a bit less likely to take the "easy" (read "hacky") way out of a problem when they know there will be eyes on their code.
My experience has been that most developers really like getting feedback and having these discussions. And I've definitely seen them pay off.
You need two other seniors to give you +1 to move on. The number "two" comes from the fact that we are still a small team.
People can will still review stuff that has already two +1's but the commiter could already move on if needed.
The advantage of pull requests is less "teaching how to do it properly" but more "learning from each other" and "continous exposure to all parts of the codebase".
Thus it's not the job of the Team Lead to do this reviews but everyone's.
Pull requests reviews are done (asynchronously) done each morning (whatever this means for each person/timezone). This leads to the fact that you usually can expect PRs to be done within 12-36hours.
I could not imagine working in a team with different experience levels (not speaking of countries/timezones/specialisation) without pull request reviews.
The other part is encouraging them to be engineers, not "coders". Coding implies write code and shoot it out. Engineering is a mindset of continuous improvement and complete examination of the problem and solution.
pull requests also essential but it's still admin overhead for you to review everything
You should also lead by example. Your post includes numerous spelling and grammatical errors. If you want them to be precise with such things, you yourself must do the same. Sorry, not picking on you, but your developers will follow your lead with any form of writing.
I do get your general point of leading by example though - and like the idea of "show them whats wrong instead of just telling them by fixing it in real time".
http://www.amazon.com/Code-Complete-Practical-Handbook-Const...
http://www.barnesandnoble.com/w/clean-code-robert-c-martin/1...
http://www.amazon.com/Writing-Solid-Code-Microsoft-Programmi...
http://www.amazon.com/Practice-Programming-Addison-Wesley-Pr...
http://www.amazon.com/Code-Complete-Practical-Handbook-Const...
I know these are old books and are C and C++ oriented, but it helped me a lot during my formative years and helped me transition from being a decent programmer to being a decent engineer. They are short books which are well written and not very dense.
Instead consider them to be a team of people. As you said, each of them are at different stages in their career in programming. You need to understand this and sympathize with them and their code. This takes time and effort on your part to help lead the way.
You say that you review the commits now and then and find some of these issues, but do you bring them up? Do you show them how the code can look and read better?
If you do, make this a process and act as the person approving pull requests for the first several weeks. Then formalize this process and delegate this job to the next person who you feel writes high quality code.
Again a SHORT guide or book will not turn around the quality of your codebase and the team working on it. If you're interested in better code, invest in your team. Send them to conferences, give them a budget to spend on books, have book clubs and lunch and learns. Invest in the process of improving the skills of your team.
It's really easy to write problematic code if you're being pushed to crank out work fast, all the time. The more stress and tighter the deadlines the more easily correctable problems you are going to see. Other than that you have to assess whether your team believes the errors you see are actual errors.
Plus half the time we did lunches it was as a team and we always ended up talking shop.
We also welcomed free food and had other people in the community come and speak. Now if you're in a culture where it's 100% coding 9 to 5 then I'd completely agree with you.
Pretty much everything in this comment thread describes approaches that only work with the buy-in and support of upper management. If OP (or anyone else) is truly in a position where code reviews are impossible, and lunch is everyone's "one break in the day", then there is very little you can do improve code quality in that environment.
In THAT kind of environment... you slog through, get what you can onto your resume in preparation for the next job search, and don't let yourself get too emotionally attached. Support for quality has to start from the top, you really can't "grassroots" it if upper management pays only lip service or doesn't really care at all.
It's easy to tell when a manager thinks of you as a "headcount", and it definitely doesn't inspire hard work or quality work.
In the process of connecting with your team as people it should become much more obvious what they need to improve. Sometimes it's encouragement and building confidence (especially for inexperienced devs). Sometimes it's asking questions about what they're trying to do. Also, if you have trust with your team, then offering suggestions for improvement and constructive criticism will be more well received.
The most important thing is to avoid them becoming defensive, and back off or move on when it does happen. It may sound like coddling, but there's a lot of psychological benefits for getting people to see their mistakes and possibly even come up with their own solutions. When people become defensive they come up with reasons to defend what they did, and are blind to the fact that there may be a better way.
I do bring the issues up - but usually end up being really frustrated because I assume that these things should be obvious. A lot of the comments in this thread are forcing me to reflect though, and realize that I didnt "know" those things out of the box either - but learned them by seeing others' good code and learning from that or because someone was patient enough to walk my through my mistakes (of which I certainly still make many).
While a pull-request based system would be ideal - it's the timeline and the scope of work that has us a little bit stuck with setting up such a new process.
My key takeaways from this thread would be though that I should certainly do more of the code-reviewing & maybe even get some of the team more involved in code-reviews.
Really appreciate the tremendous feedback here and it makes me realize again why I end up spending so much time on HN - there's just so much to be learned here from some truly helpful folks!
I'm glad that you've gotten so many takeaways but be very mindful of this. As you've said you didn't see a lot of these things when you just started out. It's through experience that things became obvious.
It'll take time but if you focus on teaching and improving your team you will get there.
You cannot sit and review each commit, the best you can do is to make them care about code.
[1] http://www.amazon.com/Clean-Code-Handbook-Software-Craftsman...
[2]http://amzn.com/0137081073
As for my specific suggestions, I would recommend "The Pragmatic Programmer: From Journeyman to Master", and actually apply the principles in the book yourself. Use unit tests, setup CI (such as Jenkins), a code review system (such as Gerrit), a bug tracker, code analysis (such as Sonar), and use tools (linters, etc). Also, cranking up warnings on your compilers/interpreters and not allowing in anything that doesn't pass muster is a good start. This might all seem like a lot, but you can do little parts of it at a time. It is worrisome that you say you can't review every commit. Perhaps you could assign members of your team to review each other's commits? This would also have the bonus of giving them a feel for what it's like to be on the receiving end of bad code, and to teach each other.
Just put them on mandatory level 2/3 support 20% of the time for the code that is written.
When you have to maintain code you fully grasp the concept of «well written code»: just code that is easy to maintain.
And every month try to have them talk about their top 3 reasons that makes them loose time when fixing code.
My fear with peer review is that it is very artificial, what I like with maintaining code is that you see your mistakes sometimes due to trying to write «clean» code.
Magic number deserves a bullet in the kneecap, but magic numbers disguised as factories are worse. A good code is a code where mistakes are obvious. Value these coders.
If the topic were math, you'd immediately see that a tutor and a lot of practice were essential parts of the equation.
If the topic were baseball a coach and a lot of practice would leap instantly to mind.
However, it is too often the case that employers and managers draw a line between the "educational" period of a persons career and the "work" period of their career. They also draw a line between what is their responsibility (in terms of improving an employees skill set) and the individuals. Combined these two lines means it is easy to get into a situation where, as a manager or employers, you are likely to think "You should have learned this 'before'" or "You should learn this 'on your own time.'" Neither of which thoughts actually help get you closer to a more skilled employee.
I'd also recommend choosing a strict language with useful conventions like Go (www.golang.org) if you have the possibility to switch. If will prevent many (not all) headaches in the long run, even if it's not immediately obvious.
For Clojure development: https://github.com/bbatsov/clojure-style-guide
For Scala: https://github.com/alexandru/scala-best-practices/ or http://docs.scala-lang.org/style/
Basically you get everyone in a room. The basic premise here is that everybody brings their "A game" and shares it with everybody else. One person types, everybody else decides what to type. Sounds chaotic, but as long as folks stay focused, what happens is that very quickly the entire group gets on the same page about IDE, coding standards, patterns, and whatnot.
I'd plan for a few days, but you might see results just in a solid morning of doing it. Don't know.
Of course, you probably don't have a few days. But -- how much time are you losing to this other stuff? Couldn't hurt. Beats giving people a book to read.
Looking forward to trying it!
You absolutely cannot control people. The only way to get people to do things, and i mean the ONLY way is to get them to WANT to do it. Therefore, you have to ignore the people who aren't cooperating with you and focus on the one's who do. Treat them like gold and be willing to make compromises with your quality standards because they will not be the same as yours. Then, as you make inroads on the project together, the rest of the team members either have a choice to get on board or get left behind. It is that simple.
Communicate.
Tell them what you are seeing, and just as importantly, why it can be a problem. Many times, beginner mistakes arise simply because they never have been exposed to the reasons for them being "mistakes" in the first place. So teach them. Nicely.
Think of them as students, and you are their teacher/mentor. Help them grow, support them and act as a resource so they can do their own jobs better. Treat the whole experience as an opportunity for growth, not a judgment on their skills.
2. Rotate who reviews so everyone gets a chance to review everyone and put yourself in the rotation.
3. Lead by example. Write some code into production and submit yourself to review. Everyone writes code, everyone reviews, everyone gets reviewed.
4. Each review find just one thing to refactor and let them know they should be looking for the same thing in other people's code.