Code reviews
This is a modal window.
The media could not be loaded, either because the server or network failed or because the format is not supported.
Formal Metadata
| Title |
| |
| Alternative Title |
| |
| Title of Series | ||
| Number of Parts | 170 | |
| Author | ||
| License | CC Attribution - NonCommercial - ShareAlike 3.0 Unported: You are free to use, adapt and copy, distribute and transmit the work or content in adapted or unchanged form for any legal and non-commercial purpose as long as the work is attributed to the author in the manner specified by the author or licensor and the work or content is shared also in adapted form only under the conditions of this | |
| Identifiers | 10.5446/50802 (DOI) | |
| Publisher | ||
| Release Date | ||
| Language |
Content Metadata
| Subject Area | ||
| Genre | ||
| Abstract |
|
NDC Oslo 2014112 / 170
2
3
5
6
8
11
12
16
21
22
23
27
31
35
37
42
43
45
47
48
49
50
52
54
56
57
58
61
65
66
67
71
74
77
80
81
83
84
85
87
88
89
90
91
94
95
96
97
98
100
102
107
108
112
114
115
116
118
120
121
122
123
126
127
128
130
133
135
137
138
139
140
141
142
143
144
145
147
148
149
150
153
155
156
157
158
159
160
161
162
163
166
169
170
00:00
Machine codeTerm (mathematics)Multiplication signScaling (geometry)Programmer (hardware)Fitness functionCore dumpPlanningComputer animation
01:12
Machine codeMultiplication signInformation technology consultingProgrammer (hardware)Core dumpComputer animation
01:59
Virtual machineProcess (computing)Interactive televisionBasis <Mathematik>Commitment schemeCore dumpVirtual machineFormal languageMachine codeComputer animation
03:20
TheoryWordCore dumpExpert systemMachine codeBlogField (computer science)WritingSource codeComputer animation
04:10
Graph coloringUsabilityCore dumpProgrammer (hardware)Right angleMachine codeComputer animation
05:15
Machine codeTwitterMachine codeCodecState of matterProgrammer (hardware)Process (computing)GodExtension (kinesiology)Multiplication signComputer programmingComputer animation
06:31
Machine codeProgrammer (hardware)Graph coloring
07:24
Point (geometry)Interrupt <Informatik>Machine codeRevision controlTap (transformer)Speech synthesisKnotEscape characterMultiplication signCore dumpProgrammer (hardware)Point (geometry)Product (business)Moment (mathematics)Boom (sailing)DataflowInterrupt <Informatik>Standard deviationPower (physics)Meeting/InterviewComputer animation
09:53
Machine codePower (physics)Software bugMultiplication signBoss CorporationBlogView (database)Core dumpSound effectGoodness of fitYouTubePeer-to-peerSoftware frameworkProgramming languageComputer animation
10:52
Sound effectMachine codeProgramming languageSound effectSoftware frameworkFamilyBitPressureProcess (computing)Core dumpGame theoryPoint (geometry)1 (number)Multiplication signComputer animation
12:40
FeedbackProgrammer (hardware)Machine codeSound effectBoundary value problemComputer programmingView (database)Total S.A.Core dumpCAN busProjective plane1 (number)FeedbackComputer animation
13:48
SoftwareBasis <Mathematik>Data managementProcess (computing)Dependent and independent variablesTraffic reportingProjective planeContrast (vision)Multiplication signCore dumpBoiling pointNetwork topologyDisk read-and-write headSystem callComputer animation
15:05
Type theoryMachine codeSingle-precision floating-point formatMultiplication signBoiling pointCore dumpFormal grammarMachine codeMereologySystem callProjective planeLattice (order)NeuroinformatikLine (geometry)CausalityPlanningRandom matrixSingle-precision floating-point formatVideo projectorComputer animationMeeting/Interview
18:04
System callMachine codeDirected graphType theoryCore dumpView (database)Point (geometry)Positional notationMultiplication signComputer animation
19:19
Machine codeCore dumpAndroid (robot)NumberMereologyMeasurementSound effectSystem callMultiplication signOpen sourceFormal grammarMachine codeSource codeGoogolProjective planePoint (geometry)Self-organizationEndliche ModelltheorieOrder (biology)Software developerGraph (mathematics)Revision controlSpecial unitary groupCASE <Informatik>Right angleDemo (music)Firewall (computing)SynchronizationComputer animation
23:27
Open setPasswordBranch (computer science)Machine codeExplosionView (database)Keyboard shortcutInheritance (object-oriented programming)Computing platformAndroid (robot)Kernel (computing)Electronic mailing listDemo (music)String (computer science)Normal (geometry)Line (geometry)Formal verificationInsertion lossDisk read-and-write headUniform resource locatorComputer fileGraphical user interfaceWindowBookmark (World Wide Web)Data compressionThread (computing)Object (grammar)Process (computing)Menu (computing)Strategy gameMathematicsComa BerenicesMessage passingWeb 2.0Control flowRevision controlFeedbackBranch (computer science)Formal languageProjective planeMachine codeCore dumpTask (computing)Key (cryptography)Programmer (hardware)Local ringElectronic mailing listTelecommunicationOrder (biology)Social classAliasingOpen sourceLine (geometry)PasswordWritingDemo (music)Software repositoryVirtual machineRemote procedure callGodAndroid (robot)Multiplication signNamespaceValidity (statistics)Endliche ModelltheorieSource codeMathematicsImplementationDataflowSystem callRight angleSoftwareGoogolView (database)Keyboard shortcutCommutatorComputer programmingProcess (computing)Computer animation
30:12
Message passingFlagOpen setKeyboard shortcutMenu (computing)Patch (Unix)Group actionMachine codeBranch (computer science)Modal logicStrategy gameMathematicsDean numberDrum memoryComputer fileTerm (mathematics)Personal area networkInheritance (object-oriented programming)ExplosionBoss CorporationString (computer science)PasswordPlanar graphFormal verificationEmailLine (geometry)SpacetimeWhiteboardProjective planeCorrespondence (mathematics)Web pageSoftware bugMereologyPasswordComputer fileServer (computing)SpacetimeMachine codeLengthLine (geometry)Revision controlImplementationoutputMessage passingFormal verificationCore dumpBit2 (number)Information securityVulnerability (computing)Right angleSpring (hydrology)BlogForcing (mathematics)Default (computer science)Programmer (hardware)Point (geometry)CASE <Informatik>Valuation (algebra)Different (Kate Ryan album)Source codeComputer animation
36:41
PasswordFormal verificationExplosionMachine codeKeyboard shortcutOpen setStrategy gameMathematicsComputer fileBranch (computer science)Message passingModal logicObject (grammar)PiDemo (music)Data compressionThread (computing)Local ringDisk read-and-write headString (computer science)Normal (geometry)Price indexInsertion lossDirac delta functionImage resolutionProcess (computing)Patch (Unix)Random numberEmailMenu (computing)Source codeAndroid (robot)Core dumpMachine codeMetadataUniform resource locatorLogic gateEnterprise architectureBranch (computer science)Software testingBitMathematicsData miningDifferenz <Mathematik>Data conversionMereologySimilarity (geometry)FeedbackSoftware bugDemo (music)Patch (Unix)Right angleMultiplication signIntegrated development environmentBoom (sailing)Self-organizationGoodness of fitWorkstation <Musikinstrument>Process (computing)Web 2.0Cellular automatonAreaChainSystem callSingle-precision floating-point formatCommutatorSpacetimeFile viewerComputer fileBlogCASE <Informatik>2 (number)Dressing (medical)Water vapor
42:55
Point (geometry)Formal languageMachine codeShared memoryRight angleMathematicsProgrammer (hardware)Core dumpParameter (computer programming)Arithmetic meanTwitter2 (number)Category of being1 (number)Multiplication signScaling (geometry)Traffic reportingSound effectPhysical systemMessage passingMereologyWeb pageComputer architectureDifferent (Kate Ryan album)FeedbackElectronic mailing listSubject indexingModule (mathematics)ChecklistOrder (biology)CASE <Informatik>Computer programmingProcess (computing)Group actionComputer fileOnline helpGoodness of fitWhiteboardLattice (order)Data conversionAndroid (robot)SynchronizationException handlingMaxima and minimaProjective planeSoftware bugDifferenz <Mathematik>Task (computing)Scripting languageSoftware developerFerry CorstenChord (peer-to-peer)Office suiteTouchscreenDemosceneWordRow (database)Electronic program guideFocus (optics)QuicksortView (database)NeuroinformatikSoftware frameworkFile viewerBlock (periodic table)Personal area networkSystem call5 (number)Directed graphTape driveDataflowObservational studyNumberComputer animation
Transcript: English(auto-generated)
00:03
So I'm just going to start because I have a lot of material to go through. And maybe, almost certainly, one hour is not enough. So cancel all your plans. No, I'm just kidding. Actually, this will be a pretty light talk. So, hi everyone.
00:22
And thank you for coming out to this session at this late hour of the day. So I know that you've been going to sessions all day and you're probably pretty tired. I understand that. So this talk is going to be not very technically focused. It's going to be a people talk about core reviews.
00:45
Okay? So just relax and enjoy. So my name is Enrico Campidoglio. I work at a company called Treton 37. Which is, Treton is the Swedish spelling of 13. So 13, 37 for those of you, yes, I see some people nodding.
01:02
For you geeks out there, you know that the spelling of delete. So I work mainly in two roles. 80% of my time is spent working as a programmer. And 20% of the time is spent being a consultant.
01:22
So but I'm mostly a programmer, I would say. And today, I'd like to talk to you about core reviews. Not the core review you might think of. So not the core review where you as a consultant are asked to buy a company. Please come in and look at all our code and make a review. That's not the core reviews I'm talking about.
01:42
I'm talking about the core reviews you do daily with your team members. Okay? Because, why not? So core reviews. Or, as I like to say, you want to merge?
02:02
Let's talk about it. Okay? Not so fast, cowboy. And we are going to talk about two things, or two kinds of aspects related to core reviews. The first one, for all of you agile people, you immediately recognize this.
02:23
Individuals and interactions. And we are going to talk about processes and tools. Or, in more common language, human stuff and machine stuff. Because core reviews are mostly about people, but there are also some tools involved in that.
02:44
So that's what we are going to talk about. First, let me ask you. How many of you are doing core reviews with your team members on a regular basis? Okay, so that's about, I would say, 40%.
03:00
And keep your hand up if you're doing it daily, or on every commit. So that's about the same, I would say 30%, 35%. Okay, let's say 32%. Is it enough? Yeah. So 32% of you are doing it on a daily basis. So the rest of you, I'm guessing, you are not doing that.
03:21
So, why not? Why are you guys not doing core reviews? You know, in the publishing industry, no written word ever sees the light of day. Until it's been reviewed by at least a dozen people. Which are experts in the field where they are writing a book on, or an article.
03:42
Some people even have their blog posts reviewed by others before they publish them. Because they are afraid of the bashing, you know? Just double checking. So, if you're doing this with a written word, and code is written word, why are we not reviewing our code?
04:00
Why are we just pushing and merging happily every day? I have a theory about that. There are a few problems with core reviews. And, with most human things, it's hard to just point out what the problem is until it hits you in the face.
04:25
Okay? So, let's try that. Let's try to pinpoint what the problems are. The first problem I see with core reviews is ego. Now, we all know about ego.
04:40
Programmers are a proud bunch. And, core reviews, sometimes, they just rub programmers wrong. But, we all know that ego is inversely proportional to the amount of knowledge you have. Do you think about that? The less the knowledge, the greater the ego.
05:02
The greater the knowledge, the lesser the ego. Okay? So, that's one aspect of it. Okay? And, there is something we can do about it, which we'll talk about later. So, ego. The other thing is, some programmers tend to identify themselves with the code they write.
05:23
Okay? As if the code they write is an extension of themselves. It's their essence. Now, that makes me think a little bit. Because, think about, if you look at the code that you wrote, say, five years ago. Chances are, you're going to look at the code you wrote five years ago and think, whoa!
05:45
Thank God I don't write that code anymore. But, does that mean that five years ago you were a bad programmer? I'm sure you weren't. Because, at that time, you did your best possible job with the amount of knowledge you had,
06:03
whatever the state of the industry was, whatever the trends were, you did your best. So, you weren't a bad programmer five years ago, even though now you look at that code and you think it's crap. Or, it was you who wrote it. And, by the same token, the code you wrote today, if you look at it in five years, you're going to think,
06:23
oh man, I was walking around holding monkeys by hand. Okay? So, you are not your code. The third aspect is fear of mistakes. Okay? There is this fear of, gosh, I don't want my colleagues to see that I made this awful mistake
06:46
because they're going to laugh at me, and they're going to lose the respect they have for me, and all those things. So, that's another thing that's kind of, those three things, ego, identifying yourself with your code, and fears of mistakes, those are three people aspects that are kind of blockers for code reviews.
07:04
So, just don't do that. There is another aspect, and that's called the culture. The culture of your company, the culture of your team, or lack of culture. Because sometimes programmers, when they do code reviews, they tend to get personal.
07:25
Okay? And speak in absolutes. This is wrong. This way of doing this thing is wrong. Let me tell you how to do it instead. And that taps on the ego, which creates conflicts.
07:43
So, one thing to remember when doing code reviews is that do not review the person, review the code. Those two are separate things. And do not speak in absolutes. Only fools speak in absolutes, which in itself is an absolute. Okay?
08:02
So, those are, let's say, the people things. There is another aspect, and it's time. Now, code reviews take time, and we can't escape from that fact. So, this is how productivity looks like for a programmer in your average day.
08:20
You see those dips? Those are the points in time when someone taps on your shoulder and asks you a question, or asks you to do a code review. And those peaks, you see, they are late at night. That's when everybody's sleeping. So, programmers work best when they don't have interruptions,
08:43
because then they get into the flow, and they stay in the flow, and they stay productive. If someone is coming every five minutes, hey, sorry, can you just review my code? Boom, suddenly you lose your flow, and it may take you half an hour to get back into it, and you have to look at someone else's code, which you really don't want to do right now.
09:02
That's a problem. So, all those things are problems that prevent teams from doing code reviews. Let's step back for a moment. What do you get from code reviews? Why should we even bother? Let me tell you three things that you get by having code reviews as a standard practice.
09:24
First, code quality goes up, and that, I can tell you, is really true. You can also think about it. Two pair of eyes will find more defects or more problems than one pair of eyes. Three pair of eyes, and ten pair of eyes, and one hundred pair of eyes,
09:42
they will find everything there is to find in that code. So, code quality will go up. Also, because you can enforce code conventions, right? So, code reviews give you this power. Actually, I watched that as a kid.
10:02
That's called He-Man. Do anyone in the room remember He-Man? Oh, yes, you are my people, yes. The 80s. The 80s. Good times. Then, you have bugs. We talked about defects. Bugs go down, okay?
10:21
Code quality goes up, bugs go down. And if you find a bug in a code review, it's much better than if your customers find the bug, or the entire world finds the bug, okay? So, it's better to catch bugs early rather than later,
10:41
especially if you are on television or on YouTube. Did CNN actually show that? This is the CNN logo. That would be amazing. And then you have this aspect of, as you sit with your peers and you're walking through the code and you're talking about conventions and how to do things,
11:01
you share knowledge. So, you share the knowledge you have about the domain or about the programming language or the framework or whatever it is. You share it into the team, okay? So, if you have someone in your team that is really expert about something,
11:21
they will transfer this knowledge to other people, the hungry ones, hungry for knowledge, okay? Code reviews are a good way to do that. So, we have seen there are problems, there are people problems, there is ego, there are cultural problems, speaking absolute,
11:41
attacking the person instead of going after the code review, and then there is this time problem. Let's see how we can tackle that. Let's turn those things around, okay? There is actually a way. First, let's tackle this ego thing. Now, if you have code reviews in place at your workplace,
12:04
people will feel observed. They will feel that at some point, someone else are going to look at their code. Therefore, they will be a little bit more careful about how they write it. This is called the big brother effect. Now, it may not always be positive
12:21
because it can create anxiety, but it actually improves the code quality. So, just by having code reviews as a process creates this pressure on being a little bit more careful before you push that committee into the master, okay? Big brother effect.
12:40
The second thing is that code reviews trigger ambition. Why? Because you want to impress your programmer friends, right? When you do code reviews, you want them to think that this code is awesome. You don't want to sit in a code review and people are like, yes, it works, but, you know.
13:03
So, it triggers this ambition that many programmers have, if not all of them have, to impress and to feel accomplished, okay? Push their boundaries. Code reviews have that effect and they tap on the human aspects. And then, when you actually do code reviews,
13:24
try to be constructive. Give constructive feedback. Be nice. Those things will make code reviews easier
13:41
if you just are nice to the one or the ones you're doing code reviews with. And in the end, it's important to remember this. In a project, everyone is on the same boat. You can safely assume that in a project, all team members are there to do their best
14:02
because everyone wants the project to succeed and everyone wants the software to be the best possible software that can come out of that team. Now, this idea is very at the very basis of the agile methodologies where you have the trust.
14:20
Trust in people in that people want to do their best, which was a contrast from the heavy process methodologies from the past where there were managers controlling everyone and checking their time reports. Why? Because they didn't trust that people wanted to do their best. Those methods were based on the assumption
14:42
that people are slackers, so they need to be controlled and whipped otherwise they won't work. That got completely torn on its head with agile where you put trust into the team. The team will take responsibility and they will do their best. So let them do that, okay?
15:01
So core reviews is a tool that also ties to that. Now, let's talk about time. Now, we said core reviews, they take time and there is really much we can do about it. However, there are three kinds of core reviews you can do. Actually, there are six, but let's boil it down to three.
15:22
So the first kind of core review is called the formal core review. And that's when you open up Outlook and you book a room, a meeting. You invite the people who are supposed to be in the core review, the people who wrote the code and people who are supposed to review it.
15:43
You book a projector even as a resource in Outlook. Sometimes you even book refreshments in Outlook as a resource. Then you all meet and you sit down and they all go, so, show us your code.
16:01
And that's a pretty scary situation to be in. Now, those guys are serious, they even have a typewriter, that means they really know their shit, okay? So, that's kind of intimidating. And very, very time consuming. However, that's the kind of core review that will find the most amount of defects.
16:25
So, that's one. The other kind of core reviews are those who are called over-the-shoulder core reviews. And those are slightly less formal. You don't have to book a meeting, you don't have to book a room,
16:41
you basically just get up and go to one of your colleagues and ask, sorry, could you please review my code? And then what happens? You sit down at your computer, your colleagues are sitting next to you, and you are walking them through your code. So, I did this, and I did that line.
17:00
So, that's okay. That works. It certainly requires less time and less planning. And you will find an okay amount of code defects. Why? Because if the person who wrote the code is the one walking through the code, they will, of course, skip through parts of it,
17:23
and they will go fast and say, ah, don't look at that, it doesn't matter. So, the people who are doing the core reviews, they don't have time to think by themselves about that code. They're just being held by hand and quickly walk through the diffs so that we can get along and get over with it, okay?
17:41
Now, like in this particular picture, this guy goes, huh, for example, not one single semicolon, okay? I hope they won't notice that. And then the guy over there who is paying attention goes, you're not going to merge that, son.
18:02
So, that's over the shoulder core reviews. The third type of core reviews, actually, they don't have a name. I call them the async core reviews because they happen asynchronously from when the code is written. And the person who wrote the code doesn't even have to be physically there
18:24
when the core review is done. However, I found it pretty difficult to find a way to represent async. How do you represent async? So instead, I put out a picture of async. So, what are those async reviews?
18:42
Those are the reviews that are done offline. If you're all using GitHub and you do a pull request, that is in itself an asynchronous core review. Why? Because you just do the pull request and you go about your business. At some point in time, later, someone will look at your code,
19:01
make a notation, and let you know what the problems are. So, they happen temporarily asynchronously. Okay? That's kind of hard to say. But still, it's true. Those are very effective. Why? Let's look at it. Now, this is a very scientific measurement.
19:23
Now, it may sound like I'm pulling these numbers out of my hat. But those are actually being measured. So, if you have this graph where the Y represents a number of defects found in a core review, and the X is the time effort that takes,
19:43
up on the right corner, we have the formal core reviews. They take the longest time, but they find the most defects. On the lower, we have the over-the-shoulder. They are very quick and easy and hippy, and we do them all the time. But they find also the least amount of defects, because of the reason we talked about.
20:03
Right in the middle, we have the async or the un-sync. Okay? So, I propose that the kind of core reviews that should happen in a workplace are those async core reviews. Now, let me give you a demo of how such a core review could happen.
20:22
Now, I'm going to be by myself, so I'm going to play two roles. But I just want to show you a tool that you can use to do this kind of core reviews. So, now it's tool time. The best time. Now, you may use GitHub for your projects.
20:40
Anyone is using GitHub for their project? At work, not open source? Okay. One. Great. And how many of you are using GitHub? Private repos? Okay, so a few. I'm guessing you're taking advantage of the pull request model to do core reviews.
21:01
How many of you are doing that? Okay, so three, four. Yeah. Because not every organization wants or can use GitHub for their internal projects. So, there are some tools that help you do core reviews in this fashion inside the firewall.
21:20
So, you don't have to have your code out in some other place. And one of them is called Garrett. Now, Garrett is a core review tool that is a fork of another core review tool, which is a fork of another one which was developed by Google internally.
21:46
So, it's closed source. It never saw the light of day. But Google is using it internally for their own development. Now, at that time, I can't remember the name of the project. Something with M. Anyway, it was tied to Perforce.
22:00
Because Google was using Perforce as source control. So, this tool they built was tied with that. Now, the creator of Python took that code and open sourced parts of it and tied it instead to subversion. Yeah, but that made sense at that point in time.
22:22
You know what the saying is? At that time, it sounded like a good idea. Then someone else, namely the Android open source project nonetheless, those people chose Git for their source control.
22:40
And they wanted to use the same tool. So, what did they do? They forked it and they tied it to Git. Now, as Git is becoming more popular every day, that's what I'm going to show you. So, there are alternatives if you're using subversion. The other tool is also open source.
23:02
And it works with subversion. This one works with Git. And it's being used by the Android open source project amongst others. So, let's look at how a sync code review looks like.
23:21
But first, let me show you this tool. So, let me just mirror that. Okay. That looks strange. Let me just increase that. So, this is the Garrett web.
23:42
A web AUI that's running on my machine. And by the way, this is Garrett, the open source. It's on the web. And that's the Android open source project version of Garrett. So, there is a web UI.
24:05
Where you can see incoming reviews, outgoing reviews. So, there is a list of commits that are pending for review. And that's the key of the asynchronous model. As you work during your day, you don't have to get interrupted.
24:22
You are in your flow. You do your work. When you feel that you have five minutes, ten minutes time, you open up a tool like that and you look what are the pending core reviews. And you just grab one. And you do review in your time and pace. Nobody is leading you to the code. You have time to reason about it.
24:41
And you can give your feedback and then you go on. So, you see the advantages of it. No one is interrupted. Still communication happens. So, now I'm gonna pretend that I'm a programmer. Well, I actually am a programmer. So, I'm gonna pretend that I'm in a project.
25:00
And don't be scared by the black. It will soon turn light. So, I'm in a project and I'm using Git, okay? So, I am in a branch called demo. And I have a source file, okay? It's called credentials.cs.
25:21
Can you all guys read that font? It's okay? Also in the back? Great. So, this is a C sharp class. It doesn't really matter what the language is. I'm gonna make a change to this class. It represents credentials. And I have a task that requires me to implement password validation.
25:46
So, that's how I write the code. Now, don't remember, no judging. Don't judge my code just yet, okay? So, this is how I whip together this code. And I'm ready to save it.
26:02
So, I made a change. You see on the left, there are some new lines over there. Now, I do the usual Git then. So, if I say cm, it's just an alias for commit-m, okay? I see it a lot in the command line, so I don't really write commit-m. So, I just make aliases. So, let's say add password validation, okay?
26:23
So, I'm gonna commit that. And now in my demo branch, I have a new commit which is not being pushed yet. Now, the remote or the original Git repo is being hosted by Garrett. So, in order for this to work, you need to let Garrett have the keys to your Git repo.
26:46
So, if you have a Git repo that's sitting on your local network, you have Garrett sit in front of it and catch all the commits that come in. Okay? Let me just show you.
27:02
Also, to verify that it is really up. Oh, remote show origin. Yes. So, you see the remote is sitting on my local. So, .local. Listening on port 8080. And that's exactly the same which is over here.
27:28
Okay? So, let's go back to it. So, how do I... Now, I want to... Someone has to look at this code.
27:40
I don't want to just merge it and push it and be done with it. I need someone else to look at it. What do I do? So, if I sit in the command line, I could just write git review. Now, I know this looks like total magic. But I'm gonna really show you what git review does. So, review is an alias, of course, like everything else.
28:01
It's an alias for what? You might ask. It's an alias for this. You say push to origin, but you don't push to the same branch where you're working in. You're not even, for God's sake, pushing to master. You're pushing to a different branch that does not yet exist.
28:25
And it is called for. That's the namespace of the branch. And master. Now, this for master is a Gerrit convention, which means that commit, I would like it to merge to master.
28:42
Just like when you do a pull request, you say, what do you want to merge it to? That is the Gerrit convention. If you say for dash slash and the name of a branch, that would be interpreted by Gerrit as, okay, you want to merge this to master, but you need a core review first. So, instead of doing push origin and all that, you can just write an alias
29:04
and instead say git review. So, now this has been pushed to Gerrit version of git into that branch. Now, let's look. Now, I'm off to the beach. Now, I have no worries.
29:21
Someone else, some poor, poor guy, is going to look at the, you know, they have a coffee break. Now, if I feel like doing a core review. So, they go into Gerrit. Oh, and they see. Now, this looks like outgoing because it's the same account who made,
29:40
it should turn out incoming, but I'm within the same account, okay? So, this is both me and the people reviewing. So, there is an incoming review by me. I want to merge into master, okay? Let's open it up. Whoa, I know, I know.
30:00
Gerrit looks like crap because programmers are not designers and you know the story. However, it reminds an awful lot of Gmail. Why? Because those are originally the people who worked at Google who made the original project. So, if you need a shortcut, you can just say a question mark
30:25
and you get a very Gmail-like experience. So, this is how your correspondent pull request page. However, there is actually more here. Let's take a look. First, you say it needs a core review.
30:41
I have a chance to review. Here, I have the diff over here. If I open it up, I can even review the message, okay? Like I say, oh, that message is not clear enough. But in this case, I'm going to say, you know, this message is good enough. So, you just say shift them and move to the next diff, okay?
31:05
And it shows you the next file in your commit and you are ready to review. So, you see, I added this with some white space. Ooh. That's already a point of review. So, I'm going to make a comment and say, dude,
31:25
what's with the white space? I hate that. And you just save. Now, this reminds me an awful lot of the GitHub request. You can annotate on the lines. Works really well. Then I go into the method itself.
31:43
I'm done picking the low-hanging fruit. Now, let's actually look at this code. Now, the people who are awake among you may recognize this bug. I'll give you a couple of minutes.
32:02
Yes, sir. This is the go-to-fail bug. Or the C sharp version of it. Now, a few months ago, Apple was involved in a little scandal regarding the security of their SSL implementation on iOS. Does anyone recall that?
32:22
A few? Okay, for those of you who don't remember that, don't know that. So, Apple had a security hole in the SSL implementation, which was on everyone's iPhones and iPads, which allowed basically to bypass the verification of who the server is.
32:41
Now, part of SSL is to check that the server really is what the server says that it is. Now, there was a bug that allowed some code to bypass that. And that way, you could have a server that said ebay.com with SSL, but in reality, it was a completely different server, taking all your inputs.
33:02
Now, debug turned out to be exactly or a version of... Now, that code was written in C. This code was in C sharp, but that's basically the same bug. Now, do you see that? What happens... What happens if I send a password
33:24
that doesn't have... That does have the required length? Well, it will set the valid to true, all right? And if it doesn't have it,
33:42
it will return false. However, since I don't have braces, this one will be executed when this is false. However, this will always be executed. Always. Because if you don't put braces on the if,
34:03
it's only the first line after the if that's being selected. The next line, it's not part of the if. It's always there. So this way, you can completely bypass the validation, which happens afterwards.
34:23
So what happens? I send in a password that only has the required length. This will be set to true. This will not run because it only runs when it's false. However, when I come to this line, it's going to say return true.
34:41
So this is the SSL bug. And unbelievable, someone just made it in C sharp. So I'm going to say, whoa, whoa. What happens if the password has the required length?
35:04
Okay? So I made another comment. So I'm done reviewing this file. I did it completely on my own. And now I'm ready to publish those. So you see I have a draft. I have two comments that are not yet published. And the review is still pending.
35:23
So now I'm ready to say, you see, Garrett gives you a score. Plus one means it looks good to me, but I need someone else's opinion. So this is more advanced than the pending, than the pull requests. This is a little bit of a workflow built in.
35:42
So you can ask for another pair of eyes, or you can just say, looks good to me, approved. Or you can say, you know, this works, but you're not following the conventions. And then like in this case, no way.
36:00
This cannot be, this shall not pass. Okay? And you say post. Done. Now I am back to the programmer, and I look at Garrett, or actually you can tie it to email, and I see that, whoa, rejected.
36:23
What happened? Okay? Of course, you can imagine that it will appear in another one. Then I open it up, and I look at the files, and I see, okay, there are two comments over here. Let me open it up. You see Enrico made those.
36:41
Let me open it. Oh, you're right. You're right. I totally see that. Okay? So you can also reply and say, sorry, man, I didn't read the news. Okay? So the same idea, you can have a conversation here with comments,
37:02
and you can also publish the comments. So you can have a conversation ongoing. Notice also this. If in the meantime something happens in the branch, I can always cherry-pick it into another branch.
37:23
So since Garrett is tied up to Git, it can take advantage of the things that Git has. For example, you can decide that, you know, I want to take that commit and take it into another branch of mine for review, and you can just do that from a web UI. Okay?
37:40
At the same time, if a new commit had happened, you will have a new button here that says rebase. So you can keep up with what's happening in the branch as the review can take a few days or the time it needs. So it's a little bit more involved. And you can add more reviewers and so on and so forth.
38:00
So it's a little bit richer than the pull request. So what happens now that I want to patch that? I want to fix the problem. Now, let me go back to my file, and let's fix it, okay? Let's fix it really quick.
38:21
Let's just remove the duplicate return, okay? Now, you may have opinions about how the code actually looks. This is not really what I would call idiomatic C-sharp. But for the sake of this demo, let's just keep it the way it is.
38:41
Also, I had a... You see here this white space? I had a comment on that. So let's just empty that. So now I don't have it. Let me save it. So I removed the white space, and I fixed the bug. So now how do I push this commit?
39:01
So I made, of course, a change. If you've been a servant before, you may notice that when I pushed my previous commit, this happened. Now, this is a piece of metadata that Garrett appends to all commits,
39:23
and that's what it uses to track different commits that are part of the same core review. So a core review gets feedback, because we're going to address that and make a new commit, but those commits are still part of the same review process. You don't want new reviews to come up. And that ID is what Garrett uses to track that.
39:41
So as long as your commit has the same ID, they will be treated as the same core review. Issue. Okay? So the easiest way to do that is to just add it and just amend. Just redo the latest commit with the same ID
40:01
and with a different diff. And I'm just going to say review again. You see here this time, it says updated changes. And this is the unique URL for that review. Okay? So now let's go back to our core review.
40:25
Now, I see that a new commit has been done right about now, part of the same review process. And if I look at the two files,
40:43
let's say this one is ready to be here. This is good. Let's look at that one. Now it looks better, okay? So now the problems have been addressed. You see the comments are gone because this is a new diff. I'm going to say this is good. Good to me. Now, this verifies another extra thing.
41:02
Verified, you may not want to use it, but you could have, for example, an extra check that says this code passes all the tests and the quality gates. So not only you can review it, it could be good by review, but you also need to verify that it works in the test environment or whatever.
41:22
And you can have an external build tool like Jenkins or TeamCity do that because Garrett has a REST API, so you can have TeamCity, for example, run the build, and if it passes, automatically set the verify flag. Good enterprise stuff. Enterprise. So now that it has been passed,
41:43
now it's ready to be submitted. And this is cool because now I can simply say submit. Boom. Merged. See here? Now, this has been merged into the branch where it was supposed to be merged. In this case, master.
42:01
So the person who wrote the code doesn't have to also receive the feedback and say, okay, now it's okay. I push it. You can just push it directly from Garrett. So if I go back to my workstation and I go to master, I can simply say git pull
42:24
and I got my new commit. Okay? I just pulled it from master because it got merged. Good? So this was a brief review, a brief demo of how you could carry those code reviews
42:43
inside the organization without having necessarily to use GitHub or similar tools. You can simply use tools like Garrett if you're using git or other tools if you're using, for example, subversion to do this kind of asynchronous code reviews
43:01
inside your team. As I've just seen, they happen at their own pace and time and they don't disturb anyone. And still a conversation happens. By the way, this is also good if you are a distributed team where people are physically in different parts of the world where it's very hard or difficult to have a natural meeting even over Skype.
43:20
This works really well for distributed teams. So let me leave you, because we are actually almost done already, which is good, so we can grab a coffee before everyone else. Let me leave you with three concrete tips
43:40
that you can apply when you do your code reviews. How many times have I said code reviews? Did anyone keep count? I'm guessing about 50. How many times can I really say it before it becomes, you know, noise? So, first one is review small portions of code at a time.
44:03
But that also implies that the commits are small. Of course, if you are going, like I've seen, unfortunately, commits that consist of 130 files where the comment is many changes,
44:21
that doesn't help anyone. It doesn't help the reviewers. It doesn't help the people who are going to merge it. It doesn't help anyone. So keep your commit small and focused. One commit, one logical change. It will make it easier to review as a single unit, like in this case. So review small portions of code at a time.
44:42
Second tip I have for you is to keep a checklist. Now, you can find those online for every language on the planet because some of those bullet points, those are specific for the language you have. So, for example, for C sharp or for .NET, you will have a point like check for throwing argument exceptions
45:02
or check for null exceptions or make sure the braces are in the right place for C sharp. Now, of course, if it's JavaScript, that list is empty. Sorry.
45:21
The third tip I have for you is when you do a core review, keep it under one hour. Why? It may sound like an arbitrary number, but the human mind works well when you focus on the task at maximum one hour because after one hour,
45:40
if you keep staring at the code, you won't see anything. It will be just blah. So one hour max and then just leave it and come back to it later, which is easier if you are doing the kind of core reviews that I showed you. You don't have anyone sitting next to you waiting. So keep those under one hour and you'll be fine.
46:02
With that, I'd like to say thank you for your attention. And we have a good 20 minutes for questions, if you have any. I'll take them now. Yes?
46:22
Yes? If you have some kind of artifact that you are looking at when you do those kinds of artifacts. So the question is if you do architecture reviews or design reviews, can they also be done that way? Yes, if you have an artifact that is shared that represents what you are talking about because you kind of need...
46:44
If you have some kind of document that has been worked on, which may be the architecture or the guidelines, then yes. But that's another thing. If you're thinking about the whiteboard session where everybody is throwing around ideas and brainstorming,
47:00
that must be done, of course, in person. This is about reviewing documents, reviewing text that evolves over time. So you mean if you have different artifacts?
47:29
Okay. So you mean if one core review explodes into a deeper discussion of the architecture of the system, you mean that?
47:40
Which can happen, programmers being programmers. Those back and forth can go on forever. And it happens also on GitHub. So at that point, a tool doesn't help you. You actually need to talk to the human beings. Those work because they don't scale to infinity.
48:01
So those discussions at some point, we are dealing with people. Which is kind of ironic, you know. Many people say, hey, I became a programmer because I didn't want to deal with people, only with computers. And then they find out that programming is all about dealing with people.
48:20
I'm like, oh. Okay. Yes. More questions? Yes?
48:44
Okay. Yes. So the question is what if I make a change and I want to review before I go on with that change?
49:01
Yes, yes, yes. Okay, because I need to build on top of that afterwards. So the question is, so if I make a few changes, I make small commits, and now I want someone to review those before I push them because I need some other work to happen
49:21
or someone else in the team needs those changes in order to build upon it. Can a tool help? Is that the question? Yes. Yes. In the async or async, it depends on your musical preferences, a tool like Gerrit supports the concept
49:42
of a blocking core review. So two core reviews, one core review comes in, another change comes in and says, I need that core review to pass before this passes. So there is the concept of depends on or blocks, and you will see those in the same review page.
50:01
You will see this core review is blocking those two. So if you've seen in the Android project, the list was full of core reviews with dependencies. So the people who are reviewing, of course they're going to have to prioritize those where others are waiting on. So those are built into the tools, absolutely.
50:32
So yes, so of course if you take, so the question is, sometimes it takes too long and people just continue to carry on
50:41
without waiting for the feedback. And that's a human aspect of it, of course. The fact that those can happen asynchronously doesn't mean that you can wait weeks before core reviewing. Because if the code base changes quickly and core reviews need to happen for the reason we explained,
51:01
like code quality and knowledge sharing and all this, then you need to prioritize that as part of your day, of your activities. Like you're checking email, you're checking Twitter, you do a core review, you check Twitter, you do a core review, you work, you get a coffee, you check Twitter, and then you make a core review. So you have to find a workflow that works for you during the day, right?
51:23
It's not just because you have a tool that automatically works itself out. You need a process. I hate to say a process. You need some kind of discipline, of course. This just helps to not interrupt anyone and to give the reviewer time to think by themselves
51:41
instead of having someone sitting and pointing at the screen, look at this and look at that. So that's what the tool helps with. Any more questions? Yes? Yes, you. Oh, sorry, there were two aligned? Sorry, you raised your hand exactly at the same time and one was the Zeta index of the one was higher,
52:03
so I didn't see. Sorry, it's for you first. Yes. Yeah. So the question is how much time do you spend
52:21
doing core reviews and does it become a bottleneck? So if you keep the changes small, the core reviews will be quick. It will even maybe take a couple of minutes. Small diff, yes, looks good, go on. On the other hand, if the changes are big, then they become cumbersome,
52:41
but that's something that you can address. You just agree on your team, make small changes, make small commits. You just process them at some point. Of course, you need to pay attention, just not say, okay, okay, okay, just to get rid of them. Of course, you need to look a little bit. But if the diffs are small, it takes no time.
53:00
And another thing, if you have this process in place where people are constantly reviewing each other's code, constantly doing that, they will create synergy. So the longer you do it, then the better the team members are in sync with each other because they have been looking at each other's code
53:20
and they talk about stuff that maybe one person does differently than another programmer, okay? So they will discuss, why do you do that? Ah, but I like to do that. So they talk about those issues and then they agree upon a convention.
53:40
So the longer you have this in place, the more synergy there is in the group and the less issues you will find in code reviews because the code will start to look homogeneous, okay? It will follow the conventions. Another good side effect is that spreading the knowledge not only of the language or the framework, but also the knowledge of different parts of the system.
54:04
Something that I see very often is the department, especially on large projects. A bug report comes in, it's in module foo and everybody goes, oh, that's not me. I didn't write the module.
54:21
I have no idea how it works because I only worked on module bar, okay? And that happens all the time. If you have people reviewing each other's code, they will know how different parts of the system are written. So they have a chance to go in and fix it. So you don't have that, oh, not me, sorry, I'm going home.
54:44
They will have a chance to fix it, go in each other's code and fix it because they've seen it before. They kept along with the development of it. So that's another good side effect. I don't know if it answers the question. Does it answer the question? Yeah. It's hard to measure like weekly,
55:02
but the code reviews that I do, they take from 30 seconds to the longest ones are like 20, 25 minutes. Then I get tired or I get bored. I do something else. But with a tool like that, I can just go off and do something else and then come back to reviewing and just pick up where I left off.
55:23
So there was another question on the back? No? Maybe I answered too much. Are there any other questions? Well, if you think about the questions or you want to discuss this with me, just ping me on Twitter
55:41
or just find me on the venue, okay? Thank you very much.