Code review in practice
-
Upload
edorian -
Category
Technology
-
view
990 -
download
4
description
Transcript of Code review in practice
Code Review
in practice Froscon 2011
Volker Dusch / @__edorian
Me?
2 Introduction
Volker Dusch
@__edorian
3 Introduction
PHP for around 9 years
4 Introduction
I‟m currently into TDD, CI,
Clean Code and shipping
5 Introduction
…amongst other stuff
Just go and buy those
6 Introduction
*Book covers used under fair use
Ask questions at any time!
7 Introduction
We‟re gonna talk about
Code Review
8 Agenda
Why do it?
9 Agenda
What types exist?
10 Agenda
How to do it?
11 Agenda
My story!
12 Agenda
Why do Code Review?
13 Why do Code Review?
Define „Code Review‟
14 Why do Code Review?
'Code Review' describes the systematic
examination of source code
Because code is written
by Humans
15 Why do Code Review?
Because code is read
by Humans
16 Why do Code Review?
… a lot
It‟s easy to write code
for machines
17 Why do Code Review?
… they will understand it
…well they will execute it
But Humans think
differently of code
18 Why do Code Review?
Mandatory Code Review Joke
19 Why do Code Review?
Used under Fair Use: © Focus Shift
So.. Code Review
20 Why do Code Review?
It‟s meant to improve
21 Why do Code Review?
readability, maintainability
and stability or your code
22 Why do Code Review?
... so code quality
Improve
the knowledge of
software developers
23 Why do Code Review?
Improve
achieved by…
24 Why do Code Review?
more people looking at
the code
25 Why do Code Review?
before the customer
experiences the changes
achieved by
finding more bugs during
development
26 Why do Code Review?
achieved by
making sure the code is
understandable by humans
27 Why do Code Review?
achieved by
bringing devs together to
talk code
28 Why do Code Review?
leading to…
achieved by
easier team growing
29 Why do Code Review?
or team building …
achieved by
higher consistency
30 Why do Code Review?
formatting, architecture, …
achieved by
easier & more mentoring
31 Why do Code Review?
because training people rocks!
achieved by
Types of Code Review
32 Types of Code Review
Over the shoulder
33 Types of Code Review
do you have a moment?
great! grab a chair
This is what usually
happens anyways
34 Types of Code Review
doesn‟t matter what you call it
Over the shoulder
People usually self organize
Create Events?!
35 Types of Code Review
Over the projector reviews
Over the shoulder
Pair Programming
36 Types of Code Review
Code Review with 100% more
real time than similar products
Instant feedback cycle
37 Types of Code Review
Every step of the way
Pair Programming
Automated Code Review
38 Types of Code Review
computers can be so cruel
a.k.a. static code analysis
39 Types of Code Review
phpmd, pDepend, phpcs,
phpunit, phpcpd, phpdcd phploc, phpcb, pfff
phantm, ppw I bet you couldn‟t read that during the presentation
phpqatools.org
talk to people here!
40 Types of Code Review
„nuff said
Before we talk about tools
41 Tools for Code Review
Amount of code to review?
42 Type of Code Review
Cycle based
43 Type of Code Review
Every Iteration maybe?
Get everyone to together
44 Type of Code Review
Cycle based
It might take a while
45 Type of Code Review
Cycle based
but you get the big picture
46 Type of Code Review
Cycle based
I think I‟m done with this
47 Type of Code Review
What do you think?
Feature based
Feature based
48 Type of Code Review
Whenever something of value
is done for the first time
Amout depends on
feature / task sizes
49 Type of Code Review
Feature based
Avg. 4 hours per feature?
50 Type of Code Review
Feature based
Around two reviews per day
Can be a short cylce
51 Type of Code Review
Feature based
With big features / tasks you
might run into troube
52 Type of Code Review
Feature based
That‟s 3 weeks old!
I‟m not even sure I wrote it
Gives devs. a changes to
get everything proper
53 Type of Code Review
Feature based
Nice to make sure it really
meets the businesses case
54 Type of Code Review
Feature based
Commit based
55 Type of Code Review
Review every single checkin
56 Type of Code Review
Commit based
just merges to master?
57 Type of Code Review
Commit based
„master‟ == „trunk‟
and those are feature based
reviews I‟d say
Fast feedback
58 Type of Code Review
Commit based
High traffic
59 Type of Code Review
Commit based
Commit messages matter
60 Types of Code Review
Commit based
Commit messages matter
A LOT!
61 Types of Code Review
Commit based
Tell people why a
change was made
62 Types of Code Review
Commit based
If I want to know what it does
I‟ll read the code
Make small commits
63 Types of Code Review
Commit based
As you should anyways
Small as in under 100 LOC
64 Types of Code Review
Commit based
What? You have classes
bigger than that?
Small as in change 3-4
places at most
65 Types of Code Review
Commit based
You shouldn‟t need to touch
everything. It‟s improper
But that just would be
nice, it works anyways
66 Types of Code Review
Commit based
Just don‟t review
reformattings
67 Types of Code Review
Commit based
We‟ll get back to that
68 Types of Code Review
Commit based
So let‟s talk Tools
69 Tools for Code Review
Review Board
70 Tools for Code Review
Open Source (MIT!)
Eclipse Plugin
Post Commit Review
Discussions and so on
Review Board
71 Tools for Code Review
Image used under fair use: http://www.reviewboard.org/screenshots/
Review Board
72 Tools for Code Review
Image used under fair use: http://www.reviewboard.org/screenshots/
Gerrit
73 Tools for Code Review
Open Source
Requires git as scm
Powerfull
74 Tools for Code Review
Gerrit
Screenshots created from: https://review.source.android.com/
75 Tools for Code Review
Gerrit
Screenshots created from: https://review.source.android.com/
Fisheye / Crucible
76 Tools for Code Review
Commercial
By Atlassian (JIRA)
… if you use JIRA take a look
SmartBear
CodeCollaborator
77 Tools for Code Review
Commercial
Review Board meets Enterprise
Eclipse & Visual Studio Plugins
78 Tools for Code Review
CodeCollaborator
Image under fair use from: http://smartbear.com/images/products/codecollaborator/ccollab-feature-sidebyside.png
email pass around
79 Tools for Code Review
You had me at HELO
scm sends out a mail for
every commit
80 Tools for Code Review
or push or feature or whatever
Is anyone here familiar
with “mailing lists”
81 Tools for Code Review
email pass around
Does your mail client have a
“threaded view” button?
82 Tools for Code Review
All right, all we need
email pass around
Point being:
Everyone has mail
83 Tools for Code Review
email pass around
No additional tools
84 Tools for Code Review
email pass around
That‟s also why IDE Plugins rock
No interface learning
85 Tools for Code Review
email pass around
Your process!
Not the one of the tool
86 Tools for Code Review
email pass around
But if a tool enforces a process
it might not be a good tool
Very fast cylce times
87 Tools for Code Review
email pass around
20 sec to 2 minutes per commit
Enough with the tools
already! Let‟s go!
88 Tools for Code Review
The first rule
of Code Review is
89 How to review code
90 How to review code
The first rule of code review
91 How to review code
The first rule of code review
Well.. not really
but it helps a lot!v
92 How to review code
The first rule of code review
Get everyone involved!
93 How to review code
The first rule of code review
No code is scared
94 How to review code
The first rule of code review
Everyone gives feedback
start with your juniors so they
learn and don‟t just agree
95 How to review code
The first rule of code review
Remember:
It‟s about the code
Not who wrote it
96 How to review code
What to look for?
97 How to review code
What to look for?
Image used under CC-BY-ND http://creativecommons.org/licenses/by-nd/2.0/en/
Creator: Oliver Widder http://geekandpoke.typepad.com/geekandpoke/2010/11/how-to-make-a-good-code-review.html
98 How to review code
It depends on the type
of code review
But let‟s get though my
suggestions and we‟ll see
What to look for?
99 How to review code
Does it implement the
business case?
Issue Tracking/Stories help a lot
What to look for?
100 How to review code
Intent and purpose over
actual words on paper
or hard disk…
What to look for?
101 How to review code
Does it fit into the
applications architecture?
My code always uses
SpecialDbConnect7
because I wrote that class!
What to look for?
102 How to review code
Overlooked edge cases?
$date = new DateTime can
throw an exception?!
I <2 off by one errors
What to look for?
103 How to review code
Is it tested?
Is is maintainable?
My class does everything we‟ll
ever need, no need to worry
about OO right?
What to look for?
104 How to review code
Does it conform to
coding rules?
What to look for?
105 How to review code
Does it conform to
coding rules?
What to look for?
You seem to be doing a
machines job. Do you want
some help?
106 How to review code
Let tools do what tools
can do.
What to look for?
They are better at some things
and nobody gets mad at
someone when they nag you
a lot. Just make good rules
107 How to review code
The things your rules can‟t
catch.. you‟ll notice
What to look for?
Promise
108 How to review code
Is it going to confuse your
users in unintended ways?
What to look for?
Confusing them in intended
ways is called major release
Cookie lifetime now 5 minutes?
Reddit says is more secure!!1
109 How to review code
Is there a simpler way?
What to look for?
"When debugging, novices
insert corrective code; experts
remove defective code"
Richard Pattis
110 How to review code
performance impacts?
What to look for?
SQL is hard, at times
Look out for everything that
leaves PHP (io)
111 How to review code
Duplicate functionality?
What to look for?
The bigger your projects the
more ways of achieving the
same result?
112 How to review code
And for the closer:
What to look for?
113 How to review code
Is it easy to understand
What to look for?
$important = getTRWTF(„daily‟);
114 How to review code
Does it take you longer
than a minute to grasp?
Easy to understand?
Is it the code or the commit
message that doesn‟t help?
115 Questions so far?
All right
Story time
116 STOP! STORYTIME!
Soo… code review
at our company
117 Code Review in our Comp
Do you remember 2006?
118 Code Review in our Comp
Let me help you out
2006, meet Froscon <?php
if ($handle = opendir('.')) {
while (false !== ($file = readdir($handle))) {
if ($file != "." && $file != "..") {
$files[] = $file;
}
}
// closedir($handle); php closes on request end
}
if(!isset($files)) {
die();
}
?>
119 Code Review in our Comp
How it started back then
120 Code Review in our Comp
I was a working student
How it started
121 Code Review in our Comp
One day we installed
„WebSvn‟
How it started
Subversion repository browser
122 Code Review in our Comp
And that came with a
simple RSS Feed
How it started
123 Code Review in our Comp
I started asking
a lot of questions
How it started
And nobody stopped me
124 Code Review in our Comp
And even more questions
How it started
At some point I started sending
out one mail per dev per week
with comments and questions
about the code they commited
125 Code Review in our Comp
At some day it „suddenly‟
was a part of my job
How it started
I don‟t think I ever heard the
term „Code Review‟ before
that point. I was just asking ;)
126 Code Review in our Comp
So there I was
looking at feeds a lot
How it started
127 Code Review in our Comp
In the next Year our
company grew. A lot!
128 Code Review in our Comp
I had Scalability Issues
Growing
class __edorian {
[…] // Those darn Singletons
private function __clone() {}
}
129 Code Review in our Comp
So something needed to
be done
Growing
130 Code Review in our Comp
We already had
collective code ownership
Growing
131 Code Review in our Comp
At least nobody thought
it was „his‟ code
Growing
132 Code Review in our Comp
We also didn‟t want to
stop with Code Review
Growing
133 Code Review in our Comp
So everybody agreed to
do peer-reviews
Growing
We already talked a lot before
but now we added per
commit reviews
134 Code Review in our Comp
I‟ve tried many tools
Growing
135 Code Review in our Comp
Nothing worked out well
Growing
I also didn‟t have a vision what
exactly I was looking for.
That didn‟t help
136 Code Review in our Comp
Everything felt complicated
and time intensive
Growing
137 Code Review in our Comp
I was spending WAY to
much time on reviews
Growing
138 Code Review in our Comp
So we needed a solution
Growing
139 Code Review in our Comp
That was 9 month ago
Getting it solved
140 Code Review in our Comp
Enter Qafoo
Getting it solved
141 Code Review in our Comp
It took two days!
Getting it solved
142 Code Review in our Comp
We discussed all possible
solutions with the team
and agreed that email
pass-around would work
Getting it solved
143 Code Review in our Comp
Getting it solved
We implemented a svn-post-commit-hook using
the “php-commit-hooks” from @korend svn://kore-nordmann.de/php-commit-hooks
Also check out vcs_wrapper, a part of Arbit
http://kore-nordmann.de/blog/vcs_wrapper_development.html
We needed to extending one class to hack in
your mail solution and since then I haven‟t
touched that code, it just worked
Well ok, we adjusted the “commiter <-> reviewer”
mapping array many times and created special
review circels for some projects
144 Code Review in our Comp
Then we created a workflow
for our environment
Getting it solved
145 Code Review in our Comp
Oh, environment
Getting it solved
146 Code Review in our Comp
Exchange and Outlook
Getting it solved
147 Code Review in our Comp
- Distribution list in exchange
- commit hook mails there
- Mails get marked as “you should review
that” in the subject line using filter “ |fo|ba|etc| Repo Rev Commit”
- Outlook marks those mails as important.( ! ) - Dev respons with status code (#ok,
#note, #error) and an explanation
- that‟s really really fast btw.
- read, CTRL+L, type #ok, CTRL+ENTER, done
- Continue mailing until the issue is resolved
Workflow
148 Code Review in our Comp
It looks like this
149 Code Review in our Comp
One review cylce
150 Code Review in our Comp
Getting feedback
151 Code Review in our Comp
The commit
152 Code Review in our Comp
What we don‟t do
153 Code Review in our Comp
We don‟t review…
What we don’t do
“Cleanup commits” - We trust our tools
“Buisiness case” - Takes to long. Trust devs
“Maintainability” - For the older stuff
“Performance Impact” - If it isn‟t obvious
With those changes it works out quite nicely
154 Your turn Froscon
So far that‟s our Story
Now I‟d like to hear yours
155 Questions?
Just a moment:
Any open questions?
156 Tell me!
Do you already do code reviews?
Share your exerience!
What tools, what types, what works?
Problems you ran into?
157 Thanks a lot!
Thank you for your time!