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

Re: [XP] Code review practices

Expand Messages
  • Keith Ray
    same book at publisher website: http://www.dorsethouse.com/books/hdbk.html see also: http://www.geraldmweinberg.com/Site/Home.html -- C. Keith Ray *
    Message 1 of 8 , Apr 26, 2013
    • 0 Attachment
    • gustavonarea_tech
      Where I work, we try to have another pair of eyes in everything we do, except for the occasional, trivial, no-brainer change. We use one of the following
      Message 2 of 8 , Apr 29, 2013
      • 0 Attachment
        Where I work, we try to have another pair of eyes in everything we do, except for the occasional, trivial, no-brainer change.

        We use one of the following approaches, depending on the task at hand:
        - Pair programming.
        - Solo development, with a design review prior to the implementation.
        - Solo development, followed by a review of the implementation after the change has been committed to the repository.

        When we do solo development, we normally use both design and implementation reviews.

        We do pair programming when one of the following conditions is met:
        - We want to transfer knowledge (e.g., about a library) from one participant to the other.
        - The task will involve removing or changing code spread across multiple locations (since a single person may miss occurrences or not spot all the consequences).
        - We need to add a new non-trivial component.

        We do solo development when we think pair programming isn't worth it (as is the case with a minority of the tasks) or when we can't form a pair (and the developer would be idling otherwise).

        If we do solo development, we'll also do a design review before implementation happens, unless there's one obvious way to make the change. This is, the implementer will propose a design to another developer, and once both are happy, the implementer will, err, implement it.

        And every time we do solo development, another person will review the implementation. Unless it's really trivial, as mentioned above.

        Sometimes we'll start doing pair programming, and then split once we've gotten to a point where it's no longer worth pair programming.

        In terms of planning for this, we do Scrum, and in the second part of the planning meeting (when we do the story breakdown), we'll estimate the effort required to complete each task considering the approach we intend to take.

        HTH,

        - Gustavo.


        --- In extremeprogramming@yahoogroups.com, Charles Bradley - Professional Scrum Trainer and Coach <charles@...> wrote:
        >
        >
        > Hi,
        >
        > I'm trying to come up with a list of different ways of doing code reviews as they relate to a Scrum definition of done.
        >
        > In practice, I've seen:
        >
        > * A scheduled meeting for a code review, several show up and review code
        >
        > * Paired Programming  (I realize XP'ers are biased to PP, and nothing wrong with that)
        >
        > * Live review by one other programmer just before code commit.
        > * Code review published to code review system (like Atlassian Crucible, etc) with deadline by which reviewers can submit feedback.
        > * Code review published to code review system (like Atlassian Crucible, etc)
        > where code cannot integrate until one person says "ship it"
        > * No code reviews
        > * Code reviews only on special occasions or in certain parts of the code
        > What other code review type practices have you seen?
        >  
        > -------
        > Charles Bradley
        > Professional Scrum Trainer
        > Scrum Coach-in-Chief
        > ScrumCrazy.com
        >
        > [Non-text portions of this message have been removed]
        >
      • James Grenning
        Here is an approach to reviews in a pair programming TDD environment that has worked well for some team I have worked with: - trust the pair to apply the
        Message 3 of 8 , Apr 29, 2013
        • 0 Attachment
          Here is an approach to reviews in a pair programming TDD environment that has worked well for some team I have worked with:

          - trust the pair to apply the coding standards and to refactor
          - review only the test cases
          - if the pairing was done by two less experienced TDDers, review the code too.
          - periodically run coverage check to see if any legacy code is being written

          By reviewing just the test cases
          - you lighten the review load
          - you review the architecturally significant aspect of the design
          -- interface of the code under tests
          -- the interactions of the code under test with other entities in the design
          -- check that the code do the right thing


          thanks, James

          --------------------------------------------------------------------------------------------
          James Grenning Author of TDD for Embedded C
          www.renaissancesoftware.net http://pragprog.com/titles/jgade/
          www.renaissancesoftware.net/blog
          www.twitter.com/jwgrenning
          >



          [Non-text portions of this message have been removed]
        • Vlietstra, Joe (ES)
          ... We use some form of code inspection (ala Fagan) almost exclusively. In theory, this makes defining done easy -- the inspection is done when the moderator
          Message 4 of 8 , Apr 29, 2013
          • 0 Attachment
            On 26 April 2013 23:33, Charles Bradley - Professional Scrum Trainer and Coach <charles@...> wrote:
            > Hi,
            > I'm trying to come up with a list of different ways of doing code
            > reviews as they relate to a Scrum definition of done.
            >
            > In practice, I've seen:
            > ...

            We use some form of code inspection (ala Fagan) almost exclusively.
            In theory, this makes defining "done" easy -- the inspection is
            done when the moderator verifies that the rework is complete.
            In practice, there are four different events that may be called
            "done":
            1. Developer completes code and sets up inspection
            2. Inspection (logging) meeting (possibly virtual) itself
            3. Developer fixes any issues identified from inspection
            4. Moderator verifies fixes
            Which event counts as "done" depends on the purpose. For example,
            when the developer completes code, he or she is available to pick
            up the next open task; so event 1 is "done" when working resource
            scheduling. At the other extreme, we use all 4 events (weighted
            80%, 10%, 0%, and 10%, resp.) when determining earned value (EV).

            Joe Vlietstra
          • Phlip
            ... I love it when people who don t need to help you actually finish features get to patrol their territory by reviewing your code changes, and gating them.
            Message 5 of 8 , May 9, 2013
            • 0 Attachment
              > 4. Moderator verifies fixes

              I "love" it when people who don't need to help you actually finish
              features get to patrol their territory by reviewing your code changes,
              and gating them. Blatant fail of the guideline we should unite
              authority and responsibility!

              --
              Phlip
              http://c2.com/cgi/wiki?ZeekLand
            Your message has been successfully submitted and would be delivered to recipients shortly.