Merken

Another pair of eyes: Reviewing code well

Zitierlink des Filmsegments
Embed Code

Automatisierte Medienanalyse

Beta
Erkannte Entitäten
Sprachtranskript
however when I'm happy to introduce and and work is a Python developer coming from Bristol and he will present a talk about another pair from review cold well thank you thank you thank everyone as a matter of and I'm going to look at it but why might have bacteria you well the inspiration for this this is really a bunch of people I used to work with mostly contributors to the twisted project and I was really lucky I got to experience the really rigorous approach to review it also present interested and I feel that I become a much better program thanks so that and I've also
had some really bad for review experiences kind experiences that make you not want to going to work the next day and I never ever want to make someone feel like that but when it's your role to judge someone else's work it's really easy to accept them if you're not careful and I guess that some of you have experienced a code review maybe even modes and because it's kind of becoming a fundamental part of the the software engineering process but maybe because of that it's often seen as boring grunt work and not a skill that you can hone get better at and but at my job I've spent almost as good as much time reviewing code as they do right but when I go to programming forums on Twitter revolve close about how to become a better programmer I very very little about how to become a better review and when the program starts out there often paired with a mental to teach them how to code for given kind of like easy fix bugs that they can get trained on but we're really expected to jump into the deep end and know how to review code without ever having been told so today I'm going to talk about it . what code review this how I review code some common pitfalls and traffic people fall into when they're reviewing and tools which can help make your life easier and how code review can make you a better programmer so what is code
review it's a practice where someone looks over someone else's code and they look at things which can be improved and we usually do this before merging code because we want to catch bugs as early as possible yes we got automated tests that could review can sometimes be the final gatekeeper before code hits the real world we also use it to catch non by defects which can harm the projected performance issues or maintainability problems or something like that but could review in and just about making the 1 change that that we're reviewing better it's about making all the software that we should have and it's important to think about how we can use care of you to improve ourselves and the contributors that we work with and I start talk last year at the right the dogs conference and by the whole Roland because I think it's also in Europe by and he was talking about how we should stop trying to decode ninjas and rocks lawyers of things that you see on job interview when job posts and we should start trying to become nurses code tree surgeons and I kind of see the reviews always just like that is not as thrilling as they're like Ninja feature shipping something at midnight but it's really important to the health of the project and all this happens within the context of the projects goal which usually includes shipping code faster release fast enough so let's back up and go over
quickly how we get the review stage and in the last few companies of work that you get something like this
that of programmer is new to the project brand new and she's looking to make have 1st contribution so she picks a task from the TaskTracker music you have here but it could be here at trailer whatever use and this 1 is written by the name of the user's latest employer needs to be shown enough that you present the feature of user profiles so she
creates a branch so that she can make changes to the code without warning about breaking and using branches for development is a great way to create like a low risk environment for people to experiment and it let's review it easily look at the proposed change the difference between what's being written and what's currently there and so on and
start searching around the closed and she finds that there is already a method to get sort employment history great and there's a regular function that you can see here the returns a dictionary for a user's profile it's later handed by like the view layer and shown in the and so
Jackson new she thinks might work you can see the 2 lines and it takes the base employment history and it displays the 1st item of lives in a new field called latent employer and you run the
application locally and to check and profile and it shows her latest employer value so we
know that this works elements in that it works do we need to spend time reviewing it well in my opinion poll code that's written as part of a team should be reviewed I've heard objections this particularly when the change that is trivial or if it's absolutely urgent and I'm going to propose that very very little of what we do is actually trivial programming is very hard for to listening and authors have an inherent bias toward thinking that that code is good even if we waste a few minutes reviewing a rule change that really is trivial we save hours later debugging about that was shipped when someone for their code was really simple but it actually had a fight and even if with absolutely certain that the code comparator a review means that at least 2 people know where this new code is and if the curve is really urgent to share and there really isn't someone around to review it maybe it is worth mentioning maybe you shipped data-destroying bug and you really need to reverse it right now but at the very least I'd recommend that you do post merger review that is going the internet so by 2 elements she submits her code for review and she's never because is the 1st contribution the project that she's made of course that mention that and she puts the code up for review so now
we got to decide who does the review who get permission to merge code is a whole problem that I won't go into but it's got to be someone with that mission that is where I've worked anyone can do anyone can review code and merging and it's a such trust based system and as I mentioned as programmers with bias toward thinking about code is working but we're also biased toward thinking that our code is readable comments are readable ovarian at all names understandable to the ideal review is someone who has had no part in writing the code and if we chose someone who's
recently worked on the changed files then they're likely to have really great context which will help them find high number of blood and according to market of study that I read the most useful review comments I guess the most bugs found in most cases come from people have actually previously reviewed the modified files but when we choose people who who are our recent all those Orasan reviewers that conflicts with our goal to share knowledge with the tee and that might be a case that Italy's something consider but a
good work around that problem is when you allow developers who are unfamiliar with the code to do a review you don't designate them as the gatekeeper you have someone else have to say this looks good to me that's emerges In fact when I joined the team I love to jump straight into reviewing as much code as possible and when there's no pressure of knowing that I have to take responsibility I'm not 1 thing it's hostility good someone else will do that it's a great way to learn how things are done it teaches you let's say about its quality by our standards and how the process works from writing code always through
shipping it so that I want to review elements changes this is held about stop by reading this
that without reading the suspect there's no way that I can know if the code meets all the requirements hopefully expects an issue tracker and has descriptions and examples of how the new functionality might be used for hatchery reproduce the budget there's a but with extent and then I
check the CI whatever that is would have to use them and I hope that is passing but sometimes we know that and always passing studies I check that no new issues and the I really should be a signal of whether something that was already there is broken so it's good to check that the the
next I'll look for evidence the suspects been implemented and ideally that means looking at the test changes it is about to be fixed I want to see that as a passing test which would have failed if the bug present and if there a new feature that I want to see test that cover the whole spectrum
it seems like the spec is ambiguous and i've interpreted it differently to be all that and this is a good time to start a discussion about what we want to do with your author or who was responsible for shipping the product may be the product owner project managers where it is in your
organization and then I look at the implementation I think about whether it has functionality that is interesting and that's important because even if the code works now we wanted to be safe from regressions and I kind of think about which missing test cases to convince me of that and then I think about whether there is any risk that the changes could break existing functionality yes we've got the but is not always perfect and it's often a good thing to think about and I think about is the new coach and easy to maintain and can range from things like you know of a variable names understandable but also have to think about what the structure of the code is is it is it really branching does have a lot of side effects is the item potent is new code in the place that I'd expect to find it and these are all things that you think about when you're writing the code to get a lot of the advantages of pair programming but it's asynchronous and then I checked that everything user-facing is documented that might not be relevant to your project but it's in
mind and I'm also at the same time 1 reviewing the code I'm looking out new techniques and patterns and tools that I don't know yet and it's always really nice to say thanks to the author when you learn something from reading that and then all right to comment about anything that might be improved and I'll ask questions and stuff discussion about anything I'm unsure about and at the
end all given conclusions and usually is 1 of these options was a great deal of good to me on the all these make the request changes and then you can all make request changes and then resubmit for another review and then sometimes in the right place at the cover of you that I'm not qualified for analysis someone else looks at
it and I'd say that the 1st step to to getting better at review in woods previous steps I mentioned this to take it seriously like we often do the rest of our cross if you learn about so so for engineering practices from blog posts and that for both if you learn from formal studies then look for a formal studies that were reviewed importantly ask for feedback on your review just like you also feedback on your code the I'll
examine this new guy and I immediately spot a problem the Twitter handle features above this type of case but new field is sentence cases and so I know that
the whole class is not up to scratch my comments on reforming the heat flow so what happened next well
she sees my comment and she takes a self for from making such a trivial mistakes why can't I do anything right things maybe I'm not good enough to be here and then she searches the code base for latest employer the name of the new field and she sees that it's written just like she rightly everywhere else that comes up in the beginners don't have
the confidence to protect themselves from being from feeling like a review and some experience developers don't so I should do as a community and is be wary of impostor syndrome and use code review as a forum to make this industry and nicer place but you don't want that bad things be merged just to avoid hurting someone's feelings once it is always
a 1 nice thing about how and when science made an effort is always something nice you can say Is that always address the patch and not a person at a previous company I was lucky enough to be thrown into the deep end of project and that predict quite steep learning curve but it took me some time in the mind set that my code was being reviewed and not me personally but what else could be
better with this review well on it doesn't know what to do next you
members search around the database and 1 of our goals radicals is to ship code quickly so why not make it clear exactly what she's got to do next I slow down the process and I've added unnecessary work for and also
you might spot that this is about a few characters later basically if there is no analysis empty again employers that I'm gonna get an index error when I try to act as the 1st item but I start my review when I saw the 1st problem and that means is that the whole lecture round of review which delays shipping the code and
called sometimes is a trade when you think about when to stop the batch is really large and based on a completely wrong assumptions in scrutinizing every line probably isn't worth so
Oscar back and forth in a comment about the index and so on algorithm in this mode and I say that the code review successfully stopped above getting into production right and so
I start next round of review and comment that a tri-accept across has performance overhead that we don't want and the message line is greater than 80 characters that violates and finally I mention that using again from the list of employers to remember that function she found in news isn't very nice and use of property instead and on surface these
might seem like reasonable comments but it really is not a great reviews because as a review I
need to consider the context and I need to consider what's important right now that doesn't purity sometimes maintainability so that its speed who knows and this right here is present like code of remember back so there's no need for me to be overly concerned with tiny tiny performance issues of kind of
wasted some time with the suggestion that had a well it's a guide a small rule set and followed by fine but if your own rules and I can't we recommend using that tool or set of tools to enforce them and that's partly because humans are very badly and we missed things that tools catch is partly because if we told them we can free ourselves up to be more important things
and making fun of my favorite tools it can find a lot of tools and enforces the frantic guidelines some of them as rules In this coverage of
docPipe align coverage and
requires the which checks for out like vulnerabilities and
dependencies and landscape which looks for a common code smells and mistakes that people often make and the even at the
docks if you're using the text as docking which Leinster documentation and seems link checker spell checker and everything that satisfy marked down asciidoc whatever
tool use but these tools will suggest changes that are actually work my personal preference is just always just follow the tool suggestions and overall your code is probably going to be nicer and you say the cognitive burden of lynching by on but if you
really want most of these tools allow you to ignore a particular issue like Iraq all wireless don't worry that the French name is too long and if the suggestion keeps coming up in your project and you team there might even just stable some other text in most of the tools
or change them and use get then you can integrate useful for corporate you can't actually make you can actually make it so that co company merged if the tools by foreign problems and
finally reviewable is a really powerful tool which has great features that Gerhard doesn't have like they are doing a review you can say I just want to see the changes since I last data
reveal so we go back to my review we see a suggestion of an unrelated change changing existing gets to a property if you want to know really changed it's probably best to just make an issue of you track that and deal with it later a review might be a great place to spot required unrelated change the no changes that alter question because asking for related changes slows down shipping because and also makes the next review round less pleasant effect as a review uh I should request easily reviewable changes a lot
reviewable changes well
statistically more defects of found when change that a shorter and as a review
and you can ask the Apache splinter up if it's difficult to review and you might be able to help you decide where this this should be to see someone refactors some code before adding to it you might also the refactoring is in its own separate
and when you spend a lot of time reviewing code you start to get a good idea of what's going to be easy to review and you start to write code like that and reference easy to review coach means reviews can be more effective final but the use of that and when we focus on the
reviewers life easier we write code that can be extended without changing was that you much always known as maintainable code and a trivial of example of this is the Silicon
Valley Colorado article written about 1 and then when I get a new 1 in terms of you the 1 on the comedy and the when I new item I did see just 1 yard items great
and the same for forgot about giving
running on time and on and on the of the best reviews are just laser focused on providing the bare minimum needed to ship but they also don't add unnecessary walkers to merging and when you explicitly state that suggested improvement is optional and you can also choose whether let's say to learn suggested tool or to just skip it and ship now then that really useful ERM and those parts can be adjusted depending on the context and let's say if you've got a brand new developed the project from boarding someone well you can be super picky and get them in sync with the rest of team all you can be easy easy 1 common because you want to feel happy and get started quickly and every review comes with many how would I have done that when we read someone else's approach so that the answer that question we notice that they're approach better release someone then we learn techniques and patterns from and I find
teams which value review get really much better reviews back and they get reviews done more quickly and with a great example of this is true since high school board disco every month and and you get points for submitting but you have to get the most points for doing a review thank you very
much have the how often I have enough for the yet but also my slides up there uh look probably be about something unlikely that there are any questions from the room of a few minutes for questions I disagree but was that's causing the usual Indians that's twisted to twist it is a an open source project and it's like a network framework and they want to encourage people to contribute so just a fun they got a high school board and say is being the best contributed this month of the most prolific contributors not to get 100 points something if you um in the back the book the following yet if you join the points if you submit some kind and 5 and the points if you closer together you have to get thousand points of your review because they have a backlog of code that's been submitted not reviewed and there's no point having new code is the code that's already been submitted is I agree that can up so when you to a review a total do also suggest to close to God it interested selective yes my complaints that law is not part of your job as a review of what is done on this so I don't always do it sometimes the test hopefully tell me enough if there aren't interest and then there's a good reason the code and let's say it's part of like how we can throw away all maybe it interacts with an external service and just be really costly to to build a fake that's what I like to have is a reproducible test manual test case let's say it's ElasticSearch connect ElasticSearch check this in the web interface of the urine like whatever and I like to get my editor of like my web browser run it and check that that that work and also if I've got a suggestion and like 100 per cent sure about this I like to modify the code so if my suggestion might were in the brain that stage all and see if I can break the tag by modifying the implementation or make it has still passed by modifying the implementation in a way that the functionality break and then I found a forum that I did in my and thanks for the great job I have a question regarding the size of reviews how to handle it because some if you have a feature that needs to be implemented about the change that would be too large so however to handle a case like that to make earlier we regress was only a partial implementation of the feature are which use with which is adjusted so I like to always the master a triangle whatever you for your main branch working and so 1 way that people do it is to make a branch for the whole feature and then that's just and is like master and then you make branches off that which have all the partial features and you put those branches into that Brown was brought interested given the name and then eventually when you happy with them all you got this huge and you most patterns master and what about the time spent on the wall of what it's just like that you can move into like reviewing their aligned texts and Pereira Ryan cool so depend on a few things and 1 I never want to be tied to the review was designed 5 45 minutes in my review is useless buttons but as the scrutiny how how it really depends on the context again if I'm reviewing something right at the core of my project i needs be great all spend as much time as needed or if it's let's say an API there's going to be really tough to change later because users are going to depend on it then also be were really really did if something in the middle like interval pretty tight code or something like that then I can say more like trust all that can open this up in a web browser and all seems to work so the final and sending that close and and like this and and the effects of began a program your team and maybe you want to be really picky eaters to help the wrong or something like that and on the left last question also uh so imagine you know like several issues and all those issues can be resolved in maybe 1 or 2 lines of code you say like civil commits or just 1 community always associated in different ways some people like to review the Committee is quite nice we've got a set of commands to be able to just revert um or see history why was that why did that come up that that wanted to review all of MIT's but usually in my reviews of these work now I don't deal with commit so that's different practices most people do but but I don't I'm sorry to have that so we are running out of things really much further insights you
Code
Vorlesung/Konferenz
Projektive Ebene
Softwareentwicklung
Softwareentwickler
Sichtbarkeitsverfahren
Softwaretest
Videospiel
ATM
Programmiergerät
Prozess <Physik>
Huffman-Code
Softwareentwicklung
Kontextbezogenes System
Code
Computeranimation
Programmfehler
Softwarewartung
Webforum
Twitter <Softwareplattform>
Software
Rechter Winkel
Prozess <Informatik>
Code
Mereologie
Projektive Ebene
Software Engineering
Task
Programmiergerät
Gruppe <Mathematik>
Demoszene <Programmierung>
Projektive Ebene
Benutzerprofil
Computeranimation
Prototyping
Benutzerprofil
Lineares Funktional
Subtraktion
Sichtenkonzept
Code
Mathematisierung
Demoszene <Programmierung>
Verzweigendes Programm
Softwareentwickler
Programmierumgebung
Quick-Sort
Benutzerprofil
Code
Computeranimation
Softwaretest
Datenfeld
Code
Demoszene <Programmierung>
Indexberechnung
Kartesische Koordinaten
Gerade
Computeranimation
Autorisierung
Programmiergerät
Mathematisierung
Schlussregel
Softwareentwicklung
Paarvergleich
Element <Mathematik>
Code
Computeranimation
Internetworking
Programmfehler
Objekt <Kategorie>
Mereologie
Projektive Ebene
Kurvenanpassung
Beobachtungsstudie
Managementinformationssystem
Prozess <Physik>
Güte der Anpassung
Versionsverwaltung
Zahlenbereich
Kontextbezogenes System
Elektronische Publikation
Code
Computeranimation
Programmfehler
Druckverlauf
Code
Endogene Variable
Softwareentwickler
Standardabweichung
Lineares Funktional
Deskriptive Statistik
Mathematisierung
Impuls
Vorlesung/Konferenz
Element <Mathematik>
Maßerweiterung
Code
Computeranimation
Prototyping
Softwaretest
Beobachtungsstudie
Kombinatorische Gruppentheorie
Punktspektrum
Computeranimation
Programmfehler
Soundverarbeitung
Autorisierung
Softwaretest
Lineares Funktional
Selbst organisierendes System
Mathematisierung
Implementierung
Softwareentwicklung
Interpretierer
Biprodukt
Code
Computeranimation
Datenmanagement
Zustandsdichte
Lineare Regression
Projektive Ebene
Datenstruktur
Implementierung
Autorisierung
Mathematisierung
Mustersprache
Mathematisierung
Knoten <Statik>
Code
Computeranimation
Analysis
Konfiguration <Informatik>
Überlagerung <Mathematik>
Benutzerprofil
Beobachtungsstudie
Rückkopplung
Datenfeld
Twitter <Softwareplattform>
Web log
Datentyp
Mathematisierung
Code
Computeranimation
Formale Grammatik
Inklusion <Mathematik>
Datenfeld
Rechter Winkel
Klasse <Mathematik>
Mathematisierung
Wärmeübergang
Ext-Funktor
Code
Patch <Software>
Bereichsschätzung
Webforum
Projektive Ebene
Patch <Software>
Kurvenanpassung
Softwareentwickler
Code
Computeranimation
Prozess <Physik>
Datenhaltung
Radikal <Mathematik>
Ext-Funktor
Code
Computeranimation
Benutzerprofil
Automatische Indexierung
Unrundheit
Stapelverarbeitung
Gerade
Computeranimation
Fehlermeldung
Analysis
Lineares Funktional
ATM
Kategorie <Mathematik>
Indexberechnung
Mailing-Liste
Unrundheit
Biprodukt
Code
Computeranimation
Benutzerprofil
Physikalisches System
Algorithmus
Automatische Indexierung
Rechter Winkel
Flächentheorie
Overhead <Kommunikationstechnik>
Ext-Funktor
Gerade
Message-Passing
Softwarewartung
Metropolitan area network
Message-Passing
Mailing-Liste
Pauli-Prinzip
Rechter Winkel
Eigentliche Abbildung
Singularität <Mathematik>
Kontextbezogenes System
Computeranimation
Algorithmus
Menge
Elektronischer Programmführer
Code
NP-hartes Problem
Schlussregel
Elektronischer Programmführer
Computeranimation
Zustandsdichte
Softwareschwachstelle
Code
Binder <Informatik>
Nabel <Mathematik>
Code
Computeranimation
Zustandsdichte
Funktion <Mathematik>
Mathematisierung
Projektive Ebene
Extrempunkt
Code
Computeranimation
Metropolitan area network
Gruppe <Mathematik>
Desintegration <Mathematik>
Mathematisierung
Computeranimation
Soundverarbeitung
Metropolitan area network
Message-Passing
Mailing-Liste
Pauli-Prinzip
Kategorie <Mathematik>
Eigentliche Abbildung
Singularität <Mathematik>
Mathematisierung
Unrundheit
Computeranimation
Softwareentwickler
Mathematische Logik
Uniforme Struktur
Code
Mathematisierung
Mathematisierung
Extrempunkt
Refactoring
Code
Computeranimation
Videospiel
Code
Güte der Anpassung
Code
Computeranimation
Gammafunktion
Extrempunkt
Mustersprache
Mereologie
Mathematisierung
Indexberechnung
Projektive Ebene
Extrempunkt
Nummerung
Kontextbezogenes System
Term
Synchronisierung
Computeranimation
Punkt
Total <Mathematik>
Browser
Mathematisierung
Implementierung
Gesetz <Physik>
Whiteboard
Framework <Informatik>
Code
Computeranimation
Webforum
Prozess <Informatik>
Mustersprache
Kontrollstruktur
Gerade
Soundverarbeitung
Einfach zusammenhängender Raum
Softwaretest
Lineares Funktional
Benutzeroberfläche
Datennetz
Open Source
Verzweigendes Programm
Softwareentwicklung
Kontextbezogenes System
Ausgleichsrechnung
Dreieck
Rechenschieber
Dienst <Informatik>
Menge
Mereologie
Grundsätze ordnungsmäßiger Datenverarbeitung
Speicherabzug
Projektive Ebene
Informationssystem

Metadaten

Formale Metadaten

Titel Another pair of eyes: Reviewing code well
Serientitel EuroPython 2016
Teil 95
Anzahl der Teile 169
Autor Dangoor, Adam
Lizenz CC-Namensnennung - keine kommerzielle Nutzung - Weitergabe unter gleichen Bedingungen 3.0 Unported:
Sie dürfen das Werk bzw. den Inhalt zu jedem legalen und nicht-kommerziellen Zweck nutzen, verändern und in unveränderter oder veränderter Form vervielfältigen, verbreiten und öffentlich zugänglich machen, sofern Sie den Namen des Autors/Rechteinhabers in der von ihm festgelegten Weise nennen und das Werk bzw. diesen Inhalt auch in veränderter Form nur unter den Bedingungen dieser Lizenz weitergeben
DOI 10.5446/21081
Herausgeber EuroPython
Erscheinungsjahr 2016
Sprache Englisch

Inhaltliche Metadaten

Fachgebiet Informatik
Abstract Adam Dangoor - Another pair of eyes: Reviewing code well Many of us have been taught to code, but we know that software engineering is so much more than that. Programmers can spend 5-6 hours per week on code review, but doing that is almost ignored as a skill, and instead it is often treated as a rote chore. How many of us have seen poor reviews - those which upset people, don't catch bugs or block important features being merged? This talk explores the social and technical impacts of various code review practices as well as helpful tooling. The goal is to provide a structure to help improve how teams review code, and to introduce the costs and benefits of code review to anyone unfamiliar with the practice. There are always trade-offs to be made - e.g. think how costly a security flaw in this code could be to your organisation - perhaps intense scrutiny is not necessary for prototypes soon to be thrown away. It is useful to consider the trade-offs in order to optimise for a particular problem domain. Perhaps right now it is more important to look for issues with maintainability, functionality or performance. I talk about how some fantastic code reviews from mentors, colleagues and strangers have helped me become a better programmer and team member, as well as occasions where code review has been detrimental by slowing things down and causing arguments. This is aimed at everyone from beginner to advanced programmers.

Ähnliche Filme

Loading...