Loading ...
Sorry, an error occurred while loading the content.

RE: [XP] code reviews vs. pairing

Expand Messages
  • Tim Moore
    Why not do both? -- Tim Moore / Blackboard Inc. / Software Engineer 1899 L Street, NW / 5th Floor / Washington, DC 20036 Phone 202-463-4860 ext. 258 / Fax
    Message 1 of 15 , Apr 5, 2002
    • 0 Attachment
      Why not do both?

      --
      Tim Moore / Blackboard Inc. / Software Engineer
      1899 L Street, NW / 5th Floor / Washington, DC 20036
      Phone 202-463-4860 ext. 258 / Fax 202-463-4863


      > -----Original Message-----
      > From: mgh520 [mailto:mh983@...]
      > Sent: Friday, April 05, 2002 4:12 PM
      > To: extremeprogramming@yahoogroups.com
      > Subject: [XP] code reviews vs. pairing
      >
      >
      > I'm curious about how people feel about doing code reviews as
      > a substitute for pairing. It seems generally accepted that
      > if you *must* write production code without a pair (vacation,
      > odd number of developers, etc), then the code should be
      > reviewed. But how about reviews as the norm instead of pairing?
      >
      > An interesting analogy is that of writing a document. How
      > many people would write a document for publication (or for a
      > school assignment, thesis, or whatever), and submit that
      > document as done after a first draft without a subsequent
      > re-reading of it? If you were writing that document as a
      > pair, would you ever consider that you didn't need to proof
      > read it again because there were 2 people working on the
      > first draft? Would you expect that the second person would
      > have been able to see the overall structure of the document
      > to guide the person typing, as well as catch any mistakes
      > such that your document would be done after only one draft?
      >
      > It seems obvious that you would never do this, yet that's how
      > we seem to write code all the time (pairing or not). I
      > frequently look back at code that I have written and am
      > amazed at the spelling errors in my comments, the obvious
      > stupid way I coded some piece, the comments I forgot to
      > update, the logic I completely forgot about, etc. It's
      > better with 2 sets of eyes, but we still miss a lot.
      >
      > The problem seems to be that whenever code reviews are
      > brought up, things suddenly get formal. I prefer the
      > informality of pairing -- just a couple of developer sitting
      > together writing tests and code. But what about an informal
      > code review as a substitute? Just a couple of developers, no
      > set format for the review, no documents that must be
      > produced, just a proofreading of that first draft of code?
      > Maybe a review of the junits that were written to see if
      > others at the review can come up with a new test that would fail.
      >
      > It seems like this would have a lot of advantages over
      > pairing. Sometimes there just is no substitute to giving
      > your brain a rest, then looking back over what you've done to
      > see if it still makes sense. I think the integrate often
      > idea (and working in small chunks) makes frequent code
      > review/code refactor cycles easier to do. Any thoughts?
      >
      > mike
    • Blum, Robert
      Hi Mike, ... [snip] ... Ah, but how many documents have built-in unit tests? If my documents would _tell_ me I made a mistake, _and_ I had somebody write it
      Message 2 of 15 , Apr 5, 2002
      • 0 Attachment
        Hi Mike,

        > I'm curious about how people feel about doing code reviews as
        > a substitute for pairing.

        [snip]

        > An interesting analogy is that of writing a document. How
        > many people would write a document for publication (or for a
        > school assignment, thesis, or whatever), and submit that
        > document as done after a first draft without a subsequent
        > re-reading of it?

        Ah, but how many documents have built-in unit tests? If my documents would
        _tell_ me I made a mistake, _and_ I had somebody write it together with me,
        I would be quite confident sending them out after the first draft.

        > If you were writing that document as a
        > pair, would you ever consider that you didn't need to proof
        > read it again because there were 2 people working on the
        > first draft?

        Under the above assumptions....

        > Would you expect that the second person would
        > have been able to see the overall structure of the document
        > to guide the person typing,

        Yes. That's her job at that time - she's not typing. Plus, we didn't really
        write the document in one sitting. We refactored it. We added additional
        bits. So we have done more than one draft _implicitly_, before we consider
        the document done.

        Or, if we're talking about a single story, we at least go back and clean up
        all the areas we didn't like when writing it. Which is enough for a short
        letter, the equivalent of a single task.

        > It seems obvious that you would never do this,
        Because, despite my poor attempts above, the two situations are not
        comparable.

        > yet that's how
        > we seem to write code all the time (pairing or not).

        I'm not quite sure that we're writing code all in the same way...

        > I
        > frequently look back at code that I have written and am
        > amazed at the spelling errors in my comments,

        Why did you need a comment in the first place? If it was a variable name,
        the compiler would catch it for you. (Or you would cast it in stone :). But
        even if you desperately need a comment, nobody prevents you from correcting
        its spelling. And yes, your pair _should_ catch that.

        > the obvious
        > stupid way I coded some piece,

        Is your pair asleep?

        > the comments I forgot to update,

        Again, if the comment needs to be updated to reflect the code, maybe the
        code should become the comment.

        > the logic I completely forgot about, etc.
        That's what UTs and ATs are for, aren't they?

        > It's better with 2 sets of eyes, but we still miss a lot.
        And testing is an additional safety net.

        > The problem seems to be that whenever code reviews are
        > brought up, things suddenly get formal.

        Maybe that happens because bringing a few people in a room to criticize a
        piece of work usually _is_ pretty formal?

        > I prefer the
        > informality of pairing -- just a couple of developer sitting
        > together writing tests and code. But what about an informal
        > code review as a substitute?

        How is that different from pairing?

        > Just a couple of developers, no
        > set format for the review, no documents that must be
        > produced, just a proofreading of that first draft of code?

        Which is what pairing provides? You want more eyes? Pair with the next
        developer.

        > Maybe a review of the junits that were written to see if
        > others at the review can come up with a new test that would fail.

        How often do you switch pairs?

        > It seems like this would have a lot of advantages over
        > pairing. Sometimes there just is no substitute to giving
        > your brain a rest, then looking back over what you've done to
        > see if it still makes sense.

        That's why you don't drive all the time. If your brain needs a rest, (or
        your pair is twitching to code), you switch. Quite often. So you see your
        code from different angles.

        I'm not saying code reviews won't work. It just seems to me that code
        reviews and pairing are one and the same thing. You just serialize the eyes
        going over the code, if you switch pairs often enough. Apart from that, I
        can't see much of a difference - if we're talking informal code reviews.

        - Robert
      • dhemeryy
        Hi Robert, ... In my experience, feedback from an outsider is more valuable than feedback from a creator of the work. If your overriding goal is to get
        Message 3 of 15 , Apr 5, 2002
        • 0 Attachment
          Hi Robert,

          > > I prefer the
          > > informality of pairing -- just a couple of developer sitting
          > > together writing tests and code. But what about an informal
          > > code review as a substitute?
          >
          > How is that different from pairing?
          >
          > > Just a couple of developers, no
          > > set format for the review, no documents that must be
          > > produced, just a proofreading of that first draft of code?
          >
          > Which is what pairing provides? You want more eyes? Pair with the
          > next developer.

          In my experience, feedback from an outsider is more valuable than
          feedback from a creator of the work. If your overriding goal is to
          get feedback, pick someone who will use the work, and who was not
          involved in creating it.

          But I don't know the exact economics of all of this. Each additional
          reviewer gives less return than the last. Maybe a pair is optimum.
          Maybe one outside reviewer would be better. I dunno. And pairing
          gives other benefits in addition to giving feedback.

          Dale
        • James Goebel
          ... We achieve the same goal in another way. We track who is working in what parts of the system and we mandate that programmers rotate through tasks. This
          Message 4 of 15 , Apr 5, 2002
          • 0 Attachment
            on 4/5/02 5:25 PM, dhemeryy at dale@... wrote:

            > Hi Robert,
            >
            >>> I prefer the
            >>> informality of pairing -- just a couple of developer sitting
            >>> together writing tests and code. But what about an informal
            >>> code review as a substitute?
            >>
            >> How is that different from pairing?
            >>
            >>> Just a couple of developers, no
            >>> set format for the review, no documents that must be
            >>> produced, just a proofreading of that first draft of code?
            >>
            >> Which is what pairing provides? You want more eyes? Pair with the
            >> next developer.
            >
            > In my experience, feedback from an outsider is more valuable than
            > feedback from a creator of the work. If your overriding goal is to
            > get feedback, pick someone who will use the work, and who was not
            > involved in creating it.
            >
            > But I don't know the exact economics of all of this. Each additional
            > reviewer gives less return than the last. Maybe a pair is optimum.
            > Maybe one outside reviewer would be better. I dunno. And pairing
            > gives other benefits in addition to giving feedback.
            >
            > Dale

            We achieve the same goal in another way. We track who is working in what
            parts of the system and we mandate that programmers rotate through tasks.
            This means that typically someone else will be working with the code you
            wrote this iteration. And more than reviewing it, they have become a
            stakeholder in every manner.

            James Goebel
            www.menloinnovations.com
          • dhemeryy
            Hi James, ... I can see how that would work, as long as enough of the team will have a hand in creating the code. In that case, everyone (more or less) is an
            Message 5 of 15 , Apr 5, 2002
            • 0 Attachment
              Hi James,

              > We achieve the same goal in another way. We track who is working in
              > what parts of the system and we mandate that programmers rotate
              > through tasks. This means that typically someone else will be
              > working with the code you wrote this iteration. And more than
              > reviewing it, they have become a stakeholder in every manner.

              I can see how that would work, as long as enough of the team will
              have a hand in creating the code. In that case, everyone (more or
              less) is an author, and there are no non-authors.

              Dale
            • Blum, Robert
              Hi Dale, ... If I write a piece of code with Alice, and then pair with Bob, is Bob an outsider? ... I m sure somebody, somewhere is already collecting data on
              Message 6 of 15 , Apr 5, 2002
              • 0 Attachment
                Hi Dale,

                > In my experience, feedback from an outsider is more valuable than
                > feedback from a creator of the work.

                If I write a piece of code with Alice, and then pair with Bob, is Bob an
                outsider?

                > But I don't know the exact economics of all of this. Each additional
                > reviewer gives less return than the last.

                I'm sure somebody, somewhere is already collecting data on this :)

                > Maybe a pair is optimum.
                > Maybe one outside reviewer would be better. I dunno. And pairing
                > gives other benefits in addition to giving feedback.

                As usual, you express everything I wanted to say, in about 5% of the size of
                my verbiage... Sigh!

                - Robert
              • Ron Jeffries
                ... IME, if we have a team of a half-dozen or so, we re not in much danger of everyone going blind to the same issues. There is /some/ danger, of course. An
                Message 7 of 15 , Apr 5, 2002
                • 0 Attachment
                  Around Friday, April 5, 2002, 6:00:26 PM, Blum, Robert wrote:

                  >> In my experience, feedback from an outsider is more valuable than
                  >> feedback from a creator of the work.

                  > If I write a piece of code with Alice, and then pair with Bob, is Bob an
                  > outsider?

                  IME, if we have a team of a half-dozen or so, we're not in much danger
                  of everyone going blind to the same issues. There is /some/ danger, of
                  course. An outside eye is almost always very helpful. I wouldn't
                  reduce code sharing to get it, but if I could trade reviews with
                  another team I might think about it.

                  The question is ... what might it give us ... and how would we know
                  we got it?

                  Ron Jeffries
                  www.XProgramming.com
                  The Great and Powerful Oz has spoken.
                • James Goebel
                  ... Dale, We also value code that is easy to understand and somewhat self documenting. Because we value this we test for it. The way we test is to have
                  Message 8 of 15 , Apr 6, 2002
                  • 0 Attachment
                    on 4/5/02 5:48 PM, dhemeryy at dale@... wrote:

                    > Hi James,
                    >
                    >> We achieve the same goal in another way. We track who is working in
                    >> what parts of the system and we mandate that programmers rotate
                    >> through tasks. This means that typically someone else will be
                    >> working with the code you wrote this iteration. And more than
                    >> reviewing it, they have become a stakeholder in every manner.
                    >
                    > I can see how that would work, as long as enough of the team will
                    > have a hand in creating the code. In that case, everyone (more or
                    > less) is an author, and there are no non-authors.
                    >
                    > Dale

                    Dale,

                    We also value code that is easy to understand and somewhat self documenting.
                    Because we value this we test for it. The way we test is to have complete
                    turn-over on some area of the code every iteration. So instead of the more
                    standard...

                    Iteration a) Bob works on the database and pairs with Sandy
                    Iteration b) Bob works on the database and pairs with Tom
                    Iteration c) Bob works on the database and pairs with Rich
                    Iteration d) Rich works on the database and pairs with Sandy

                    We will often have a sequence like this...

                    Iteration a) Bob works on the database and pairs with Sandy
                    Iteration b) Sandy works on the database and pairs with Rich
                    Iteration c) Tom works on the database and pairs with Tim
                    Iteration d) ...

                    This will slow down the new pair for a day or so. But if the new pair
                    cannot quickly become productive while having Experts Within Earshot than we
                    are simply fooling ourselves about how easy it will be for a client to
                    maintain this code six months from now. And so the new pair goes about
                    refactoring the code to make it more understandable.

                    In some ways this combines both ideas, pair programming and the advantages
                    of outside review. We often look to find two people who don't know a
                    section of code and have them work together in that code. Unlike an outside
                    review the performance criteria is not an opinion, we are measuring if the
                    new pair can continue to add functionality at a reasonable pace.

                    James Goebel
                    www.menloinnovations.com
                  • dhemeryy
                    Hi James, ... Very nice. A direct measure of maintainability, measured soon after the code is written. Dale
                    Message 9 of 15 , Apr 6, 2002
                    • 0 Attachment
                      Hi James,

                      > In some ways this combines both ideas, pair programming and the
                      > advantages of outside review. We often look to find two people who
                      > don't know a section of code and have them work together in that
                      > code. Unlike an outside review the performance criteria is not an
                      > opinion, we are measuring if the new pair can continue to add
                      > functionality at a reasonable pace.

                      Very nice. A direct measure of maintainability, measured soon after
                      the code is written.

                      Dale
                    • jeffgrigg63132
                      ... I ve found that getting an outsider to provide meaningful input on well-factored code is harder than you might think: I was on a C++ project (not using XP,
                      Message 10 of 15 , Apr 6, 2002
                      • 0 Attachment
                        --- "dhemeryy" <dale@d...> wrote:
                        > In my experience, feedback from an outsider is more valuable than
                        > feedback from a creator of the work. If your overriding goal is to
                        > get feedback, pick someone who will use the work, and who was not
                        > involved in creating it.
                        >
                        > [...] Maybe one outside reviewer would be better. I dunno. [...]

                        I've found that getting an outsider to provide meaningful input on
                        well-factored code is harder than you might think:

                        I was on a C++ project (not using XP, but my personal techniques have
                        always been pretty disciplined), and management decided to have some
                        out-of-state consultant review all our code and give feedback.
                        (...mostly because they couldn't think of anything else for him to
                        do, but that's another story. ;-) I was looking forward to it.

                        He got into the reviewing and giving feedback to the various
                        programmers. But then something funny happened: When he hit my
                        code, he went through several modules without saying anything, and
                        then asked management for permission to skip all code written by me,
                        because he couldn't think of any significant improvements. He said
                        that it was a waste of his time to review my code. I was shocked and
                        disappointed. I could think of *all kinds of things* wrong with the
                        code that needed to be fixed. But then I knew the code better, and
                        how it all interacted.

                        Is this an issue of the code not being clear enough for casual
                        observation to reveal its warts? Or are there issues of code quality
                        that you can't see until you get familiar enough with it to see the
                        subtle currents and eddies of its flow? Or am I deceiving myself
                        into "seeing" and making "improvements" that really aren't? (I don't
                        think so. If I can see it, I know it exists. ;-)
                        - jeff



                        "Drag me, drop me, treat me like an object"
                        -- Melbourne PC '96 exhibition T-shirt

                        "There are two ways of constructing a software design:
                        One way is to make it so simple that there are obviously
                        no deficiencies,
                        and the other way is to make it so complicated that there
                        are no obvious deficiencies.
                        The first method is far more difficult"
                        -- C.A.R. Hoare
                      • Charlie Poole
                        Jeff ... I ve worked on a lot of projects where I was the most senior person. [You d understand if you knew how old I am] Anyway, I find that my code reviews
                        Message 11 of 15 , Apr 6, 2002
                        • 0 Attachment
                          Jeff

                          > --- "dhemeryy" <dale@d...> wrote:
                          > > In my experience, feedback from an outsider is more valuable than
                          > > feedback from a creator of the work. If your overriding goal is to
                          > > get feedback, pick someone who will use the work, and who was not
                          > > involved in creating it.
                          > >
                          > > [...] Maybe one outside reviewer would be better. I dunno. [...]
                          >
                          > I've found that getting an outsider to provide meaningful input on
                          > well-factored code is harder than you might think:
                          >
                          > I was on a C++ project (not using XP, but my personal techniques have
                          > always been pretty disciplined), and management decided to have some
                          > out-of-state consultant review all our code and give feedback.
                          > (...mostly because they couldn't think of anything else for him to
                          > do, but that's another story. ;-) I was looking forward to it.
                          >
                          > He got into the reviewing and giving feedback to the various
                          > programmers. But then something funny happened: When he hit my
                          > code, he went through several modules without saying anything, and
                          > then asked management for permission to skip all code written by me,
                          > because he couldn't think of any significant improvements. He said
                          > that it was a waste of his time to review my code. I was shocked and
                          > disappointed. I could think of *all kinds of things* wrong with the
                          > code that needed to be fixed. But then I knew the code better, and
                          > how it all interacted.
                          >
                          > Is this an issue of the code not being clear enough for casual
                          > observation to reveal its warts? Or are there issues of code quality
                          > that you can't see until you get familiar enough with it to see the
                          > subtle currents and eddies of its flow? Or am I deceiving myself
                          > into "seeing" and making "improvements" that really aren't? (I don't
                          > think so. If I can see it, I know it exists. ;-)

                          I've worked on a lot of projects where I was the most senior person.
                          [You'd understand if you knew how old I am] Anyway, I find that my
                          code reviews are really boring and that I get few comments. That
                          would be OK if I believed that I couldn't benefit from them - but
                          that's not the case. Pairing works better for me in this way. The
                          same junior person who doesn't open up in a review will be more
                          likely to ask questions. "Why is it this way?" can then lead me
                          come up with better ways of doing things.

                          Code review is a weaker form of pairing, but with notes. :-)

                          Charlie Poole
                          cpoole@...
                        • dhemeryy
                          Hi Jeff, ... Given the rest of your message, I want to find a more specific term than outsider . The best reviews will come from people who (1) were not
                          Message 12 of 15 , Apr 6, 2002
                          • 0 Attachment
                            Hi Jeff,

                            > I've found that getting an outsider to provide meaningful input on
                            > well-factored code is harder than you might think:

                            Given the rest of your message, I want to find a more specific term
                            than "outsider". The best reviews will come from people who (1) were
                            not involved in creating the thing, AND (2) have a direct and personal
                            interest in the quality of the thing.

                            > He got into the reviewing and giving feedback to the various
                            > programmers. But then something funny happened: When he hit my
                            > code, he went through several modules without saying anything, and
                            > then asked management for permission to skip all code written by me,
                            > because he couldn't think of any significant improvements. He said
                            > that it was a waste of his time to review my code.

                            Yes. Your reviewer doesn't satisfy my second criterion (the one not
                            conveyed by the term "outsider"). If his performance depended on the
                            quality of your code, he might look more closely, and at specific
                            characteristics that were not as important in his general review.

                            > Is this an issue of the code not being clear enough for casual
                            > observation to reveal its warts?

                            Perhaps the code's warts are not of interest to a casual reader. If I
                            were going to review an organization's code for general goodness (what
                            an awful thought!), I would pay most attention to the code where I
                            could offer the most benefit. I would want to identify code that
                            seemed to be obviously high quality, and quickly move on. I would
                            want to identify code that seemed to be obviously low quality, and
                            quickly move on. I would likely spend most of my time on code whose
                            quality seemed neither obviously high nor obviously low.

                            I suspect that your reviewer had a similar focus, and, from that
                            perspective, did just the right thing.

                            > Or are there issues of code quality that you can't see until you get
                            > familiar enough with it to see the subtle currents and eddies of its
                            > flow?

                            There are issues of code quality that are more relevant when you
                            depend directly and personally on the quality of the code.

                            Dale
                          • jennyw
                            ... I totally agree. They re different on so many levels ... in addition to what Robert mentioned, most documents are linear, while code generally isn t.
                            Message 13 of 15 , Apr 7, 2002
                            • 0 Attachment
                              On Fri, Apr 05, 2002 at 04:00:56PM -0600, Blum, Robert wrote:
                              > > It seems obvious that you would never do this,
                              > Because, despite my poor attempts above, the two situations are not
                              > comparable.

                              I totally agree. They're different on so many levels ... in addition to
                              what Robert mentioned, most documents are linear, while code generally
                              isn't. Also, code is destined to be read by a computer with a single
                              interpretation. Documentation is directed towards humans, and that can be
                              a bit fuzzy ...

                              > I'm not saying code reviews won't work. It just seems to me that code
                              > reviews and pairing are one and the same thing. You just serialize the eyes
                              > going over the code, if you switch pairs often enough. Apart from that, I
                              > can't see much of a difference - if we're talking informal code reviews.

                              Pair programming isn't just about catching errors -- it's also to reduce
                              the sense of code ownership.

                              With regard to code reviews, code reviews can be emotional -- reviewers
                              might be less inclined to criticize for fear of hurting feelings, and the
                              coder might become defensive.

                              In the bigger picture, though, there are advantages to no one person
                              owning any bit of code. This gets rid of issues like territoriality,
                              reduces hurt feelings, and also gets people in the habit of knowing the
                              whole system, or at least a much larger part of it. Junior programmers
                              can gain more confidence because they're encouraged to work on some of the
                              same sections as more senior programmers (which they might not get a
                              chance to do if they aren't pair programming). Also, a lot of programmers
                              learn by doing, and they'll probably learn more about sections of code if
                              they have to write it rather than just read it.

                              Jen
                            • jennyw
                              ... Also, even if asking Why is it this way? doesn t lead to a better way of doing things, it can help a junior programmer learn faster than they might on
                              Message 14 of 15 , Apr 8, 2002
                              • 0 Attachment
                                On Sat, Apr 06, 2002 at 01:01:43PM -0800, Charlie Poole wrote:
                                > that's not the case. Pairing works better for me in this way. The
                                > same junior person who doesn't open up in a review will be more
                                > likely to ask questions. "Why is it this way?" can then lead me
                                > come up with better ways of doing things.

                                Also, even if asking "Why is it this way?" doesn't lead to a better way
                                of doing things, it can help a junior programmer learn faster than
                                they might on their own. There are other ways a junior programmer -- or
                                programmers at any level -- might learn from being paired with people with
                                a diverse range of experience.

                                Additionall, pair programming takes away territory issues. Since no one
                                person wrote any section of code (or relatively few sections), everyone
                                feels an equal sense of entitlement when it comes to making changes. I
                                guess you can get something of this feeling with task rotation, as someone
                                mentioned, but only if the programmers are on an equal playing field to
                                begin with.

                                Code reviews are also emotional. Because one person wrote the code,
                                feelings can be hurt. Some reviewers might be hesitant to say something
                                for fear of hurting feelings, and some people being reviewed might be more
                                defensive than they otherwise might.

                                Jen
                              • Ron Crocker
                                ... The outsider does not suffer from cognitive dissonance (the effect of being too close to the work to see the problems; also seen as the property that I can
                                Message 15 of 15 , Apr 8, 2002
                                • 0 Attachment
                                  dhemeryy wrote:
                                  > In my experience, feedback from an outsider is more valuable than
                                  > feedback from a creator of the work. If your overriding goal is to
                                  > get feedback, pick someone who will use the work, and who was not
                                  > involved in creating it.

                                  The outsider does not suffer from cognitive dissonance (the effect of
                                  being too close to the work to see the problems; also seen as the
                                  property that I can keep looking at this sentence forever and mis the
                                  misspelling.)

                                  Having a "disinterested" 3rd party provides a more objective review.
                                  Since most projects can't afford a totally disinterested party,
                                  non-author review is generally sufficient. That's the principal role of
                                  the pair partner, IMO. The partner should be providing that continuous
                                  feedback on *whatever* you need feedback on -- more tests, simpler,
                                  "does that really work?" Pairing *can* suffer from cognitive dissonance,
                                  or it might not. The more your pairs suffer this effect, the more you
                                  need another feedback mechanism.

                                  Ron C.
                                Your message has been successfully submitted and would be delivered to recipients shortly.