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

Code review practices

Expand Messages
  • Charles Bradley - Professional Scrum Trai
    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
    Message 1 of 8 , Apr 26, 2013
    • 0 Attachment
      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]
    • Ian Mitchell
      I think there are 2 extreme cases and many in between. Checking an input form: There are many small edits such as the range of a date plus checking that the
      Message 2 of 8 , Apr 26, 2013
      • 0 Attachment
        I think there are 2 extreme cases and many in between.
        Checking an input form: There are many small edits such as the range of a
        date plus checking that the mouse-over help does say the corresponding
        thing, Code inspection works here. Time-consuming to test by repeated data
        entry.
        Indepth SQL interfaced code - test-driven design. Best to set up test case
        data which can be repeatedly used..


        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:
        >
        > * 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]
        >
        >
        >



        --
        Regards
        Ian Mitchell, FIITP ITCP
        ICT and Management Consultant
        2/40 Sylvia Road
        St Heliers
        Auckland, New Zealand
        0064 9 5851580
        http://www.Mitchell.co.nz
        http://www.AboutIT.co.nz
        http://www.SoftwareAsAService.co.nz


        [Non-text portions of this message have been removed]
      • Keith Ray
        People not doing XP/TDD often do not review unit tests... even if they have them. If mandating code review, I would require reviewing both code and unit tests
        Message 3 of 8 , Apr 26, 2013
        • 0 Attachment
          People not doing XP/TDD often do not review unit tests... even if they have them.

          If mandating code review, I would require reviewing both code and unit tests together.

          Suggested reading (if not doing XP):

          Handbook of Walkthroughs, Inspections, and Technical Reviews: Evaluating Programs, Projects, and Products by Freedom & Weinberg

          --
          C. Keith Ray
          * https://dl.dropbox.com/u/686328/keith_ray_resume.pdf
          * http://agilesolutionspace.blogspot.com/

          On 2013 Apr 26, at 4:33 AM, 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]
          >
          >



          [Non-text portions of this message have been removed]
        • 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 4 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 5 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 6 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 7 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 8 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.