Ask HN: How do I get my team to write better code?

https://news.ycombinator.com/item?id=8567134

by kamakazizuru • 12 years ago

63 81 12 years ago

news.ycombinator.com

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.

jgrahamc• 12 years ago

I wrote a short blog post for work about this http://blog.cloudflare.com/making-code-better-with-reviews/

Simply 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.

kevincennis• 12 years ago

I enthusiastically second the suggestion to use pull requests. They make a huge difference in my experience, for a few reasons:

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.

andreasklinger• 12 years ago

We do team based pull request reviews. (in github)

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.

cscharenberg• 12 years ago

This is a good process and works well for my company as well. Everyone does code reviews but you need 2 senior people to call it complete and check it in.

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.

Bahamut• 12 years ago

If your timelines are such that you cannot institute a process for code reviewing pull requests and you have a team of developers, then I have to question the planning by the managers.

dharma1• 12 years ago

do a half day workshop with your team going through typical mistakes, how to avoid them, and how to write good quality code - after that there should be no excuses.

pull requests also essential but it's still admin overhead for you to review everything

blueatlas• 12 years ago

We do 1-1 peer reviews and make changes in real-time so that the developer can see the preferences for code style, naming, indenting, etc.

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.

kamakazizuru5• 12 years ago

Right, well it's one thing when posting on HN - I'm not really running a spelling and grammar check. That said - my complaints are not around their writing style. Many of them are non-native speakers and I can live with bad grammar & spelling in comments. The pain-point is really the coding style that leads to bugs where they could have easily been avoided.

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".

joezydeco• 12 years ago

Steve McConnell's Code Complete might be a good resource. Could you buy a copy for each team member and take an hour each week to read and discuss a chapter? That might get you on the road to better coding practices.

http://www.amazon.com/Code-Complete-Practical-Handbook-Const...

rglullis• 12 years ago

Each week, you take one of them to be your pair. Do pair programming for 2-3 hours each day of the week. Make a wiki page with code style guide and recommendations. Each common mistake you find your pair doing, add to the wiki page. At the end of the week, you and your pair review the commits you have done and from a couple of others from the team.

blahbap• 12 years ago

As for books, "Clean code" by Robert Martin is an obvious choice. It covers the fundamentals of good, clean code.

http://www.barnesandnoble.com/w/clean-code-robert-c-martin/1...

smanatstpete• 12 years ago

Try having your team read these books:

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.

edgo• 12 years ago

Code Complete has 960 pages, not in the short book category

llamataboot• 12 years ago

Clean Code and The Pragmatic Programmer are both great books, but really your team needs to be doing code review on every line of code that is going to make it to production. It doesn't have to be /you/ reviewing all of it. As a bonus, having your team review each other's code builds more familiarity with the app(s) you are working on and allows all your team to learn from each other.

rmcastil3• 12 years ago

You need to stop thinking of them as 'coders'. When you think of them that way, you're putting them in a box and limiting them.

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.

ritchiea• 12 years ago

This 100%, but also OP says due to the nature of the timeline of the project he cannot review all of the commits, this leads me to ask: what are your developers' timelines for delivering work like? Equally important, what do they feel like their timelines are like?

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.

BillyMaize• 12 years ago

I agree with this other than the lunch and learns. Leave their one break in the day out of it.

rmcastil3• 12 years ago

This all depends on the environment you're working in. When we used to do lunch and learns they were on Fridays which were blocked off for side projects.

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.

StevePerkins2• 12 years ago

Heh... have a bad experience with lunch-n-learns at a previous gig? :)

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.

joshdance• 12 years ago

This is the first things I thought when reading this question. The are not 'your coders'. They are part of your team. They are your team. You need to let them know what you need, and why you hold them to a higher standard. Teach, don't demand.

tex0gen• 12 years ago

Agree 100%. My boss has invested in Treehouse to improve code practice. We also started pair programming to help each other identify potential bugs and teach each other new ways of doing things. It reduces tech debt as well.

zo1• 12 years ago

Treehouse? All the searches for that keyword with this topic led me to some sort of beginner's course on programming... Link, please?

jobu• 12 years ago

Instead consider them to be a team of people.

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.

kamakazizuru5• 12 years ago

You're very right about the coders bit - I guess it was more an issue of framing it. I've corrected it now.

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!

rmcastil3• 12 years ago

I assume that these things should be obvious.

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.

shreeshga• 12 years ago

Give them a free copy of Clean Code[1], I started to care about my code more after I read that book.

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...

avelis• 12 years ago

Any chance I get I make an effort to suggest this book. There is also another book by Robert C. Martin which I suggest called The Clean Coder[2].

[2]http://amzn.com/0137081073

lukesan• 12 years ago

I would start with Refactoring by Martin Fowler. For me this is the quintessential book when it comes to basic software design principles. It introduces code smells and tells you step by step how and why to improve your code design

npsimons• 12 years ago

There are many different things you can do, but it all comes down to: do your programmers care? If they don't at all, it will be impossible to make them improve. If they care a lot, they will improve themselves. If they care a little, that's where you can try some of the strategies others have given here.

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.

julie1• 12 years ago

Forget all the craps about new silver bullet mantra (peer review, agile and peer coding and shit).

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.

ainiriand• 12 years ago

If you can spend some money I recommend hiring someone good to give some lessons, some hours per week. Then there is also the option to give those lessons yourself.

iandanforth• 12 years ago

This. The general form of this question is, "How do I help someone increase their skill level?"

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.

tamersalama• 12 years ago

They are not coders. They are partners in success. Make sure first you treat them as such. Each person might have much to give above turning specs to code (ideas, processes, improvements, automations, design, architecture, etc...). If they feel they have a stake and their efforts are accommodated (and praised), there will naturally be an improvement.

thegenius2• 12 years ago

Yes. This. The OP is NOT leading a team of programmers. He is asking HN to lead his team of programmers. "lead" is not a noun. It is a verb!

lazyjones• 12 years ago

You absolutely need coding standards (the stricter, the better, as long as they make sense), especially with languages like C or Perl where you can shoot yourself in the foot with what looks like particularly "smart" coding.

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.

eccp• 12 years ago

Point them to some coding guidelines for the language/platform which you're developing.

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/

DanielBMarkham2• 12 years ago

Saw something interesting at a conference a while back. If you have a few days, it might be worth trying. It's called "mob programming"

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.

brianmcc• 12 years ago

I find this both crazy and quite appealing! Maybe doing so after also dishing out some of the good books recommended elsewhere in the thread could work well. First read about good practice, then participate in it?

DanielBMarkham2• 12 years ago

Yes. It struck me as one of those "crazy enough it might work" things.

Looking forward to trying it!

thegenius2• 12 years ago

the best way you can improve the quality of code is to LEAD BY EXAMPLE. you're not alone in that you've been given a leadership position in a circumstance outside your control, and it's naive to think that even if you were given your pick of the litter, that they'd all be A players who respect you and don't want your job, have issues with your quality, etc.

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.

a3voices• 12 years ago

Good coders should start new projects. Mediocre coders should maintain projects.

codingdave• 12 years ago

It is your team, with your own culture, so I would not presume to tell you exactly how to do it, but I can give one general bit of advice:

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.

jwitchel• 12 years ago

1. Smaller commits. Break the project down into smaller tickets and have them check-in more frequently.

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.

euphemize• 12 years ago

Most tips here are very good, suggesting books ("Clean Code", etc.), team huddles and showing them the way. But you seem to be implying that they could be great coders - if they wanted to.

I can't help but feel like these programmers aren't very passionate about their code, and that's what you need to help them with. If they're naming variables wrong and hardcoding strings, but understand the asymptotic running times of what they're doing, I'd go on a hunch and say they're bored or unmotivated.

I say this because I recognize myself in some of these behaviors - when I want to get rid of something and don't care much about the project. "What's the fastest shortcut I could take to get me out of here?".

vladimirralev• 12 years ago

I honestly think this complaint is poorly argumented. It depends on the nature of the project. When you say "findeventsfromthelast6hours" that's perfectly reasonable name for disposable code or private implementation. You should only be concerned about such name if it made it to the API somewhere. Hardcoding things is also not a bad thing by itself. If I had to guess, your team is under the impression the code they write is prototype and will be rewritten anyway, not API to be consumed by other parties or doesn't have a direction that allows them to make the proper abstractions. Sure you can demand nothing to be hardcoded, but you must remember eventually those params will have to come from somewhere. I see so many people move hardcoded values Java -> IOC/DB config or Erlang -> DB not realizing it's more difficult to reinitialize the component than to do a hot reload and speed up your development process at the very least. Whatever you do it must be for a well-defined reason. It's also possible your team developed their own process where it's easier or faster to fix these issues on the go rather than planning in advance.

cellularmitosis• 12 years ago

Indeed, I think Robert Martin's "Clean Code" would agree that if you don't need to vary the "6 hours" part at run-time, then "findeventsfromthelast6hours" is actually better than passing in an "hours" argument ("Clean Code" advocates minimizing function arguments). Refactoring the name in the face of a one-time change in behavior (e.g. "4hours") should be trivial with any modern IDE.

jtwebman• 12 years ago

Below are a few things that could help but you said it yourself. If you don't have time to review then my guess is you are also pressuring the devs for time. Sometimes to make your deadlines they might have to cut corners. Maybe you can switch to a more agile process and instead of crazy deadlines you ask them what they can get done the next 2 weeks (correctly).

- Setup process for them to review each others commits (Code Reviews)

- Add a tool like FxCop, JsLint or JsHint to the build and commit process

- Hire more senior programmers that they will look up too.

- Paired programming

- Be a team and do stuff as the team (We are not factory workers)

nXqd• 12 years ago

Short term solution : Lead by example, a good chance that they can learn from those and maybe copy them afterward. But the code quality will improve. This can take your time but as long as you have several good examples, you can reuse those. Long term solution: Encourage them to watch conference and good books like Clean Code, or TDD books. Those are incredibly good. Maybe they are not interested in at first, and never, but those who want to be better will become ones.

StevePerkins2• 12 years ago

What I've seen work is a combination of code reviews and ongoing continuing education.

----- CODE REVIEW SIDE -----

This can include automated tools, human interaction, and computer-assisted human interaction. To start with, pick a set of command code standards and style guidelines. Google has nice ones for many languages that you can use as a starting point (https://google-styleguide.googlecode.com/svn/trunk/).

Secondly, enforce them through automated tools (e.g. in a Java shop, these might include PMD, FindBugs, Checkstyle, etc). Have everyone install the IDE plugins for those tools (along with your common config). Incorporate them into your continuous integration build process. Perhaps put in place a static analysis tool like Sonar to send regular reports on code issues.

The human element would start by having someone review committed code before it's allowed to be merged downstream. Git's pull requests, along with diff view systems like GitHub's, make this easy... but you can use workable practices with Subversion or whatever also.

There are limits to "pull request"-based code reviews, though. When change sets grow too large, it's difficult for the reviewer to understand the high-level vision for the changes. You start reviewing the trees, rather than the forest. So I think you still need periodic in-person code reviews, to walk through the state of a codebase at certain milestones and discuss higher-level issues.

----- CONTINUING EDUCATION SIDE -----

The thing that I've seen work best here is a regular, ongoing lunch-and-learn type series. Take any of the standard code quality treatises (e.g. "Pragmatic Programmer", "Code Complete", "Clean Code", etc), and tackle a chapter per week. Better yet, split up the chapters and have the team members themselves present a chapter. It's great experience for them (if they're not TOO freaked out by public speaking), and you'll never learn something as deeply as you do through the process of teaching it to others.

The problem with lunch-n-learns is that you MUST have buy-in from management to make them a serious and ongoing part of your team culture. When deadlines are tight and there are fires to put out (and when are there not?), a lot of companies are quick to cancel that week's lunch-n-learn. When that happens, they lose traction and fall apart.

rovolutionary• 12 years ago

You can set up a weekly code review where you take turns letting each developer present a piece of code they wrote to the team, and then discuss and give feedback about that code.

Also direct them towards resources where they can learn clean code practices. Perhaps institute a monthly "book club" where you select books about good coding practices. I would suggest starting with Clean Code by Robert Martin

projectileboy• 12 years ago

I spent 10 years "coaching" (that word... blech...) teams, and from my experience there's no easy answers. What works for one team at one point in time might be horrible for other groups. Having said that, here are some things that have worked for me:

* If some of the people on the team have more skills than others, you could try asking people to pair program for part of the day. You could try setting up a lab space for this if people are interested.

* Weekly brown bag sessions where you talk about a technical topic of interest. Let people on the team nominate ideas. Do it over lunch and have the company have food delivered. The better the food, the more people will want to attend.

* You should get an automated build in place, and encourage people to do some level of automated testing if they aren't already. Some people take this a step further and use the automated tests to help drive out the design (Test-Driven Development by Kent Beck is a good book on this topic).

* You could try starting a technical book club where people read a chapter a week, and then get together and discuss it over lunch. Given your situation, I'd strongly recommend starting with "The Practice of Programming" by Kernighan and Pike. Again, it helps a lot to have the company get good food delivered.

Don't make a million changes at once. I might start with the book club if I were in your position. None of this stuff will magically transform your team, but these kinds of things can start creating a culture where people start to care more about the quality of their work. You'll encounter people who have very strong religious feelings for or against any one of the things I listed above, usually based on some past experience. I've seen each of the above succeed big and fail hard with different groups. Just try what seems to make the most sense for your team.

One last note: when you try things, don't do stuff in fits and starts - talk with the team about what you're going to do, see if there's buy in, and if there is, commit to trying it for, say, four weeks. After the four week period is done, talk with the team about whether or not it's working for them, and if they'd like to stop or continue. Over time, you can add/remove/change more things in the same manner.

Good luck!

waps• 12 years ago

I work at a place that requires code review and has automated checkers for "code quality". Let me tell you the obvious : the best code I see has these problems, because the best code is the best code because it uses the language to it's maximum effect, which often precludes having perfect "style". The best code is tested in functional ways, which means that it doesn't have "good coverage", nor does it really use unit tests more than a little bit.

And the worst code I see, by people who learned to code a month ago or worse and don't know any algorithms (but feel like they know better than people who've studied algorithms and languages for years) almost without exception perfectly styled. It contains moronic errors like swapping variables around (because these programmers do not know how to use the type system), it contains 5 unit tests for every single little function, because that increases the lines of code metric that is so universally used. And the reason for half the lines of good is "good practices".

Here's how you recognize good code : firstly it is not possible to shorten it without causing a MAJOR disaster in the readability area. Every concern is properly separated out into it's own pieces of code, and aside from the (short) main function there is very little single-purpose code. The typing system is used well. Prices and amounts are NOT the same data type, for example, and cannot be obviously switched. Unit tests exist for core algorithmic pieces only and other than that there are system tests that confront the code with real-world situations while running almost the entire program, ideally under heavy load with half the backend unresponsive, and has a statement that says the test fails if it takes longer than 1/10th of a second. It implicitly follows and beautifully implements a design document that is not written in word, but in a 10 to 15 line comment on top of the file/class.

Note the issues : 1) hardcoding things 2) naming mistakes

Both of the issues you complain about can be fixed mechanically or with absolutely minimal supervision through refactoring. Yet next to all the comments below suggest DOUBLING the manual effort needed to get code into the repository. While automated fixing might be problemating, writing linters that detect these problems is trivial.

Unless of course the problem is that you feel that you need to fix how others program because you "know better", but can't actually write code checkers. In this case, why are you leading them ?

So well in that case I'd advise a slice of humble pie, and a compilers course.

I review commits now and then and find some of these issues - but due to the nature and timeline of the project cannot do it for each and ever commit of course.

You sound like someone with an MBA. A programmer would recognize this for what is is : a problem screaming to be automated. Code commits can be made dependant on code checkers succeeding - just write the ones you want/need. Can't do that ? You're not a programmer - or at least not a good one - and stop whining about how difficult programming is - learn it first.

arvanhalleorg• 12 years ago

Technical books. Code videos from stuff like Google I/O and friends. Coding conventions. Peer programming. Use pull requests and assign them to another member of the team. It won't fix it overnight, but you'll notice the changes as they come in. Also, you might want to let them learn to write better code. People adapt best when you lead them, instead of getting them to do it. Everyone was a beginner and not everyone was exposed to the same amount of code or scenarios.

edw519• 12 years ago

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.

Why not? I do.

I have 8 direct reports and I review every single line of code that they write. Some days this is all I do.

Others think I'm crazy but our group outperforms every other group by any measure. I'm convinced it's because of peer review.

A few thoughts:

1. Regarding resources and timelines: It's not when you start, it's when you finish. Every hour of peer review saves me at least 10 hours on the back end (systems testing, regression testing, user acceptance testing) and 100 hours on the back/back end (maintenance).

2. I never have to tell anyone anything more than once. I make sure I explain why. (When you do <x>, you cause <y>.) Then they learn and remember. This is not a religious debate; it's a way of conducting business. This won't work nearly as well if people are more interested in showing what they know than learning how to do it properly.

3. As I get to know different people, I know where I can skim and where I have to focus. I rarely return much to senior people. I often return a lot to junior people.

4. Everyone makes mistakes, even senior people, and even me. Peer review, like regression testing, is a pain in the ass, but it more than pays for itself on the back end.

5. As soon as you feel you're ready, you're senior people can take some of the peer review workload. I have done this before, but it hasn't lasted because of turnover.

6. Once people know everything will be peer reviewed, they become more careful about what they're doing.

7. Technical debt is a huge timebomb. I know of no better way of slowing it down than 100% peer review.

8. Everything is documented in the ticket. This provides some feedback on how it's working and what should be changed (programming standards, peer review methods, etc.)

9. Once you put your phone away, get off the internet, and stop going to useless meetings, it's amazing how much more time you have to do things that really make a difference, like peer review.

10. I hate doing this. I'd rather just code. But the only thing I hate more than peer review is fixing bugs. That's why I do this.

circlefavshape• 12 years ago

100% peer review is the only process change that has been implemented anyplace I've worked in the last 18 years that seriously and lastingly improved code quality. I don't like reviewing, but it's very very effective, especially once you get good at it (and, like anything, it takes practice) - also, fwiw, it really improves you own coding

HeyLaughingBoy• 12 years ago

I agree completely and I would add that you need to keep the review size reasonable for this to work. I've been asked to review thousands of lines of code at once and it doesn't take long before your eyes glaze over and you start missing things.

kamakazizuru5• 12 years ago

The logic of "spend time now because otherwise you'll spend 10x later" - really resonates with me. I guess I need to see it as an investment rather than a waste of time - that really turns the entire thing around quite a bit! Thanks :) - I also completely agree with number 9 - it is easier said than done though in some companies :/

ayers• 12 years ago

My current company does this also. The team is slightly smaller with 6 but we are in the process of taking on a couple more developers and the peer review process will still remain. The CTO like yourself reviews all code commits as a final sign off once the change has been through a more thorough peer review. So it is possible to do 100% peer reviews.

Your point 10 highlights exactly how I feel about it.

feyn• 12 years ago

I’ve been in a technical lead position across 5 companies over the last 10 years, and after trying just about everything written or spoken about influencing team code quality, I finally found a formula that works:

1) Take on the most brutal coding tasks/features yourself. You will have to cherry-pick the upcoming work to look for things that just about any other engineer would freak out about if they got it assigned to them.

2) Knock it out of the park in terms of time-to-market and code quality. You need to do both, to eliminate the concern of, “We don’t have time to write quality code.” The code should be so clean, so well factored, so well unit tested, that you would be comfortable getting a code review from Martin Fowler, Kent Beck, and Bob Martin all at the same time. This also implies that you’re read their books, and would know how to pass their code review.

3) Call a code review with the entire team, and aggressively code review your own code. There is always room for improvement in any code, depending on hour you interpreted current requirements and future needs. This has to include phrases like, “I didn’t know what to do here so I…”, “I really which I had more time to change this to…”, “I went back-and-forth on how best to abstract this, but ultimately I chose this abstraction because.” Basically, you’re looking to show them the worst possible code review on the best possible code.

4) Depending on the personalities involved (including your own), decide how you are going to coach each engineer 1-on-1. No two people are alike, so you will need to dial your approach to each person. Some people will thing you are a show-off, some will think you intimidating, as well any another other human emotion and concern that causes someone to not seek out or not take advice. This is where your emotional IQ and communication skills will be tested.

5) When you start coaching them, don’t go after every single flaw. Try to find the underlying root cause of the problem in the code, and address that. A common problem I run into is the people just don’t understand OOP but think they do. I usually have to coach them on basics like encapsulation and cohesion, working them up to design patterns, and further up to domain modeling, all the while instructing on proper implementation technique (such as unit testing.) Once they have the fundamentals in place, you can begin fine-tuning certain implementation patterns.

6) Over time, at least one member of your team will get good. With their permission, publicly review their code. In this, you want to be very positive: “I like what they did here…”, “That’s a good way to encapsulate that problem”, “That’s a powerful abstraction, especially the way it’s used…” This Rite of Passage should then become what other engineers are striving for, as only the best engineers are getting public praise fests.

This basic formula has served me very well with my current team, in that they are one-upping themselves on code-quality. Still, however, I take on the most grueling coding tasks to set the example and lead from the front. This acts to continue to demonstrate my commitment to code quality, as well and lets the team know I would not ask them to do something that I would not do myself.

yeezul• 12 years ago

I highly recommend (to everyone here for that matter) the book Clean Code: A Handbook of Agile Software Craftsmanship by Robert C. Martin. It deals specifically with the mistakes you've mentioned in your post, such as hardcoding and naming functions.

Great easy read. Cheers

avigesaa• 12 years ago

Depending on the language and context, an explicit method name like "findEventsFromLast6Hours" isn't that bad. If it's just a private helper method, I'd let it pass.

Empirically, I've found that methods with ambiguous parameters (e.g. "findEvents(6)") are a much bigger problem (event amongst senior programmers).

Thetawaves2• 12 years ago

I also had a problem with this example from OP. It is generally a good idea to implement the minimum amount of functionality to satisfy the requirements. I have seen many complex systems fail because it was over-engineered with the idea that 'we might need this down the road'.

When requirements change, you almost always have to rewrite the code. If you don't have to rewrite the code, you still have to go back and read it to make sure it satisfies the requirements. Less code to deal with will make this process easier. There is no reason to over-engineer today for what might happen tomorrow.

Code for the here and now, and aggressively delete code whenever possible.

Thetawaves2• 12 years ago

If your 'coders' are writing functions like findEventsFromLast6Hours, you are not giving them clear requirements.

I assume you gave this engineer the requirement 'find all events from the last 6 hours.'

You should have given them the requirement to find the latest events for a specified interval.

tgreenhaw• 12 years ago

Start by watching "The Downfall of Agile Hitler" https://www.youtube.com/watch?v=l1wKO3rID9g

jasallen• 12 years ago

Honestly, the things you describe are not that bad. If they are writing code that passes the test cases, move on and be happy. If you'd described things like circular dependencies and passing brittle "magic strings" through multiple layers -- things that will GENUINELY cause maintainability issues I'd sympathize more.

I doubt anyone named it the "...last6hours" but then parameterized the number of hours -- so you're changing that code anyway if the number of hours changes. If you have a requirement to parameterize that, do so, otherwise YAGNI, move on.

Hardcoded strings is another one. Frankly, unless you are using the string in more than two places or it's part of a "set" of values, where referencing the full set is useful, you may just be making more work to do it 'correctly'.

You reference timeline pressure. "Good, Fast, Cheap, pick two". If you don't have enough programmers for the timeline alotted, the powers-that-be have already selected Cheap and Fast. Deliver a product that does the job asked, a few maintenance iterations (if the product is one of the 30% that make it that far) will teach you want areas of code are touched a lot and need to be "made maintainable"

kamakazizuru5• 12 years ago

Thanks - while a lot of the other advice on this thread is helpful in terms of being a better team lead in general - I think the stuff you mentioned is really what I can apply almost immediately - especially the last bit about makign things maintainable.

thothamon• 12 years ago

Lots of good advice, but I suggest having the whole team review each others' code 100% of the time. In other words, each commit gets a random or rotating selection of two people who review the code.

Reviewing other people's code is almost as valuable as being reviewed. Even if it's not perfect all the time, it builds a good culture of coding and gives your developers valuable experience.

The culture of the code review should be a careful examination with Q&A. You need to take as long as you need to get it right. This is a great investment in your product and your team that will pay off starting right away and extending into the long term.

mamcx• 12 years ago

Apart of the suggestion here, you have a problem in " but due to the nature and timeline for the project cannot do it for each and ever commit of course".

If the time is so tight that you can dedicate time to fix stuff (including the code quality) then any of the suggestion here are DOA.

Remember: If you want to chop wood for 10 hours, use 8 to sharp the axe.

Code, code, code, code non-stop is as efficient as chop wood without sharp it after a while.

Dedicate time to improve the code quality ALWAYS pay off. Think: "I'm time constrained, so continue the death march until we finish" ALWAYS make the project take longer.

---- A good idea is STOP each week, review the tasks, ask where are the pain, where is the code that is becoming a mess, what is complicated, then FIX that. Decreasing code/clearing it in the short AND long run will be a time saver.

Encourage change when the purpose is quality. Not stick to code/design that is a pain. Stop each week to breathe and see if move forward as now make sense, or is better to kill a portion of the code and rework it.

This is specially important when the TIME is critical. When time is not critical, you can do bad code because, hell, you can take the time! But when you have not time, any bad code is slowing you.

Then make this clear to the team.

Not be yourself the only that code review. Everyone can do it.

Take control when deploy. Not when your customer say so. Be predictable (ie: Always deploy in the start of the week, never at the end).

carlio2• 12 years ago

I think you can get a long way with "automated code reviews" - basically, linting and static analysis. There are various ones out there for your language - disclaimer: I wrote one for Python called https://landscape.io

They can take a little while to get set up exactly how you like them, but once you do, run them on your CI server after every commit and it'll output the warnings about style violations and so on. That's basically what you're after. It's not a replacement for a manual code review but it's certainly closer to what you want than nothing!

As other have said though, this only works if you and your team treat code quality as important and actually take the time to understand the output of the analysis tools. That's why I like the CI approach, so that you can see if the number of warnings is going up or down over time and react accordingly. That metric is a useful one to keep an eye on to get a view on the quality of your code, and if it gets too high you can schedule some time.

(PS if you are interested in learning more about "Automatic Code Reviews" for Python, here's a talk I did at EuroPython14: http://carlcrowder.com/pages/europython-2014-automatic-code-... )

kamakazizuru5• 12 years ago

this is great - I love coding in Python and would definitely use this in the future. Currently we're doing everything in Node.JS though - do you know of any tools for that ? We do use Jslint etc - but landscape certainly seems to go a step further..

carlio2• 12 years ago

You should check out https://codeclimate.com/ then, they support JS and Ruby. There's also https://scrutinizer-ci.com/ who do PHP and apparently Python and Ruby now too.

thisGuysAccount• 12 years ago

I just spent the better part of a month cleaning up some CI issues. By the time I go to bed tonight, I hope to have a CI process completed for two platform implementations of an application that's been under development for some time. Part of the process is static analysis and linting.

I've seen the static analysis and lint results already. Not pretty, but we can clear most of it up as we develop new features.

Definitely a process that anything beyond a trivial app needs in place.

j45• 12 years ago

Sometimes it's more about learning clever architecture than clever, or better code. Clever architecture can often help keep your code an order of magnitude simpler.

The issue you may run into is seasoned developers are the ones who learn this over time.

Conversations based around "why" help a team understand the importance of:

- creating a codebase that they will still want to / be able to work on without dealing with poor decisions of the past, o

- being kind to your future self

- realizing not everyone has had a relationship with a codebase for 3, 5, or even 10 years and what that might look like. In the real world, existing code bases are a reality. Optimizing for that reality, instead of the clean slate that university assignments, or startups provide can also be an important light bulb.

- peer reviews, and teaching the why you do things a certain way are far more likely to get the lasting result you are seeking.

alwendt2• 12 years ago

If some gross code gets into the tree, nominate it for a code reading with a team of three or four people. This helps a lot because the code gets improved and the reviewers' styles also improve.

alwendt2• 12 years ago

Nominate crufty code for code readings. They are great to improve the quality of the code and that of the reviewers.

canterburry• 12 years ago

To me, this is the difference between being a programmer and a software engineer. Software engineers think of the bigger picture and understand the overall impact of choices made in the code on other things. Programmers focus too much on just coding and don't see the bigger picture.

I am sure much of this has to do with experience and having had the opportunity to learn from good software engineering teams.

In my teams, I have handled this with mentorship. Recognize who in the team needs mentoring and assign them a code review buddy. If mentorship somehow doesn't work, you just have to manage those programmers and put them under tighter supervision. However, you can't expect much change until you set and communicate expectations. Be clear on what you expect and review against those benchmarks.

jonkaye68• 12 years ago

My advice is that before you propose solutions, you ought to really figure out what the problem is. Is it ignorance, laziness, or something else? I've looked over the various responses and haven't seen one angle -- have you asked your team about why they are not doing what you'd like? There's a lot of advice about read this, do that, etc. but you don't yet know why they are not following what you think is common sense. You might find something surprising by putting it on the table, or at least you'll find the reasons they give. Then you can address those. My advice is to put this out as a question to your team and see how they react. You might even find out that some of the patterns you think are best are not, i.e., your team has a reason you didn't think of. For example, I created a 10-element array to speed the search of a big table in which all keys began with a digit from 0-9. The more experienced guy told me that in the .NET world, the convention was to use key-value pairs instead of an array. To satisfy him, I changed it to a hash table, where I hashed the first digit. It seemed pretty silly to me, making a hash table for what to me was clearly a better approach using a simple indexed array. He had in his mind the "right" way to do it, eventhough I still think the hashtable in this case was silly.

arisarnado• 12 years ago

I think you need to come up with a coding guideline. After that, you review/discuss it with your team.

Also, you may need to create a "checklist" and encourage your team to follow it as a way of self-check on their work.

In this way, you will be able to concentrate your reviews on important things (such as logic, etc) since the trivial errors were already eliminated.

devb0x• 12 years ago

PRs work. Pull requests serves to manage quality but also can find issues.

Ask HN: How do I get my team to write better code?

Related Stories