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.
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.
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.
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.
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.
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.
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.
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
Your point 10 highlights exactly how I feel about it.
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-... )
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.
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!
Empirically, I've found that methods with ambiguous parameters (e.g. "findEvents(6)") are a much bigger problem (event amongst senior programmers).
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.
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.
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.
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.
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".
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"
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...
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.
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.
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.
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).
http://www.amazon.com/Code-Complete-Practical-Handbook-Const...
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.
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.
----- 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.
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?".
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.
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.
- 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)
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/
* 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!
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.
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.
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
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.
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.
http://www.barnesandnoble.com/w/clean-code-robert-c-martin/1...
Great easy read. Cheers
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.
pull requests also essential but it's still admin overhead for you to review everything
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.
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.
