WARNING - Opinions!!
- These are MY opinions
- Stop me, ask questions
- Sorry, no code to see here
How To Improve Code Quality
- What is quality code?
- Ways to improve it
What is quality code?
- Satisfies customer requirements
- Easy to change
- Defining feature of software compared to hardware
How to satisfy customer requirements?
- The great question, is there an answer?
- Yeah, know what you don't know. Be agile.
- Deliver early, deliver often.
- Get feedback early, get feedback often.
- Reprioritize as needed.
How to deliver early, deliver often
- Automate it. No, really automate it.
- Continuous integration - automated builds on check-in.
- Continous deploy - automated or push button deployments all the way to production
- Deploy multiple times a month, multiple times a day.
What is quality code?
- Satisfies customer requirements
- Easy to change
- Defining feature of software compared to hardware
How to make code easy to change
- Be able to assert code behaves the same before and after a change
- Code is maintainable
- Code is readable
How to assert behavior hasn't changed?
- Write tests
- Unit tests, integration tests, system tests, etc.
- Make them easy to run
- Automate them, e.g. continuous integration, continuous deploy
How to ensure maintainability?
- Afraid to change it? Then it's not maintainable
- Can't explain it? Then it's not maintainable
- Can't easily debug it? Then it's not maintainable
Unit tests can help with maintainability
- Changing it? Tests help prevent regressions
- Explain it? Tests provide example of happy paths and sad paths
- Debug it? Debug the test to get directly to a piece of lower level code
However, tests aren't the whole answer
- Tests only assist with their focus area
- To improve maintainability, we need to improve readability
What does readable code mean?
- Can code be universally readable?
- Are all books universally readable?
What does readable code mean? (pt. 2)
- Chemistry Book
- Knitting Book
- Economics Book
- Software Design Pattern Book
- Are all of those equally readable for everyone?
- No, they have required knowledge of the domain that contributes to the readability
What does readable code mean? (pt. 3)
- We write custom software to deliver value for a domain
- So each piece of software has required domain knowledge
- So, readability of code isn't universal
What does readable code mean? (pt. 4)
- You need someone with domain knowledge first
- Then you can evaluate readability
- So, to improve readability, you need another team member to read it.
- So... let's talk about code reviews in depth
What are Code Reviews?
- Another member of the team reads the code and provides feedback
- That feedback is incorporated back into the code
- Repeat until team is satisifed
- Think back to editing a writing assignment
Why do Code Reviews?
- Increase readability
- Keep scary code out
- Keep code that can't be explained out
- Keep code that can't be debugged out
- ... Improve Code Quality
Why do Code Reviews? (pt. 2)
- Other Benefits as well
- Find bugs earlier
- Intrinsic cross training
- Break down of silos
- Mentor/on board team members
What isn't a code review
- Not a performance or review metric for software engineers
- Not a way to belittle others
- Not a way to settle scores
- Not a way to rubber stamp each others work
- Not just the job of one person on the team
Mindset of the coder
- Review comments are an opening line in a discussion
- Review comments can be ignored
- Review comments are NOT an order or directive to be blindly followed
- You still own the completion of the task/feature, not the reviewer
Mindset of the coder
- You are NOT your code
- If a suggestion results in undoing of your previous work, that's ok
- No ONE is perfect, you will miss something
- The quantity of feedback has NOTHING to do with the size of your change
- A suggestion might be for code you didn't change, that's ok
- Remember, the goal is to improve the code quality
Mindset of the reviewer
- You are providing feedback, not orders or directives
- Not all of your feedback may be accepted or acted upon
- Never a need for personal attacks or inappropriate language, it distracts from your point and wastes time
- Try not to pre-filter your feedback, if you think of it, put it down
- Try and review equally, regardless if you are reviewing team lead's code or new team member's code
What is the reviewer looking for?
- Does code implement the actual task/feature?
- Does code actually run? Do you know how to run or debug code?
- Have you tried to break code? Does code handle errors?
What is the reviewer looking for? (pt. 2)
- Is code readable?
- Is code understandable?
- Would you be afraid to maintain the code?
What is the reviewer looking for? (pt. 3)
- Is there anything missing?
- Should similiar changes be made elsewhere?
- Should some changes NOT be made?
What is the reviewer looking for? (pt. 4)
- Does code have performance issues?
- Does code have security issues?
- Does code follow applicable standards?
What is the reviewer looking for? (pt. 5)
- Does code follow team patterns?
- Does code follow team naming conventions?
- Does code follow team formatting conventions?
What is the reviewer looking for? (pt. 6)
- Does code have unit tests?
- Do unit tests follow team conventions?
- Are unit tests testing happy paths?
- Are unit tests testing sad paths?
What gets reviewed?
- Every commit to the project
- If you add exceptions to the above, the loop hole will get abused
- If it really is simple, it really will be quick to review
Who should do reviews?
- At least one other coder on the project
- Coders should be reviewers and vice-versa
- Silo'ing into coder only, reviewer only will demotivate and create adversarial relationships
- More reviewers is good if the code is critical
How are reviewers selected?
- Whatever works for your team
- I've seen self assignment work
- I've seen team lead assignment work
- Just check to ensure reviews are actual resulting in code quality improvements
When do reviews happen?
- Before the code is committed to master or trunk
- After that and any leverage is lost
- We want tangible code quality improvements not a list of future tasks
When do reviews happen (pt. 2)?
- The reviewer should review independently
- Don't review in a real-time meeting
- Reviewer needs to be able to make multiple passes
- Reviewer needs to be able to take breaks
- Reviewer needs to be able to research
- Don't timebox a review before it starts. If it's going long, ask why
Resolving disputes
- Try and let coder and reviewer work it out
- Bring members of the team into the discussion, drive to consensus
- Last resort is decision from on high
Scope of Review Comments
- Will the fix radically increase the size or risk of the overall change?
- Maybe split it into another commit and do it separately, either before or after this change
- Don't use this an excuse to NOT do something that should be done
Code review is too long?
- Is it? Are you still getting value?
- Should the change have been smaller to start with? Do that next time
Code Review Mindset
- Leave it better than you found it
- Don't take feedback personally
- Almost every review should have at least ONE comment
- No ONE is perfect
Starting with code reviews
- Just start and adjust as you go
- The first reviews will be slow, very slow and very argumentative
- Team will be having arguments over things that don't exist yet: formatting and naming standards, standard patterns
- Team will be cross training each other
- Team will eventually converge on those things and reviews will smooth out
Recap - What is quality code?
- Satisfies customer requirements
- Easy to change
- Defining feature of software compared to hardware
Recap - Get feedback faster
- Automate builds
- Automate deployments
Recap - Easy to change
- Add tests
- Do code reviews