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

Re: I still hate code reviews

Expand Messages
  • Phlip
    ... Okay - code review early and often. I know you didn t say this, but only code reviewing after a bug bites will set up negative rewards...
    Message 1 of 23 , Aug 4, 2009
    • 0 Attachment
      > While we pair on a lot of code, anything we don't pair on (defect fixes
      > typically) is reviewed.

      Okay - code review early and often. I know you didn't say this, but only
      code reviewing after a bug bites will set up negative rewards...
    • David Morash
      ... For defects we generally write tests to catch the defect, then fix it refactoring as needed, prior to having both, the fix and tests reviewed. I don t
      Message 2 of 23 , Aug 4, 2009
      • 0 Attachment
        Phlip wrote:
        >> While we pair on a lot of code, anything we don't pair on (defect fixes
        >> typically) is reviewed.
        >
        > Okay - code review early and often. I know you didn't say this, but only
        > code reviewing after a bug bites will set up negative rewards...

        For defects we generally write tests to catch the defect, then fix it
        refactoring as needed, prior to having both, the fix and tests reviewed.

        I don't quite follow, are you suggesting we should code review more
        often versus pairing more often?
      • Phlip
        ... When pairing often, bugs cause code reviews. These reviews now have teeth - they are not about coding aphorisms, they are about details that actually went
        Message 3 of 23 , Aug 4, 2009
        • 0 Attachment
          David Morash wrote:

          > I don't quite follow, are you suggesting we should code review more
          > often versus pairing more often?

          When pairing often, bugs cause code reviews. These reviews now have teeth - they
          are not about coding aphorisms, they are about details that actually went wrong.

          If the general code reviewing hurts, however, do it more often.

          --
          Phlip
        • Bill Caputo
          ... And when code reviews are about aphorisms, they re really about those in the team enforcing (dictating?) team culture to those just currently on the
          Message 4 of 23 , Aug 5, 2009
          • 0 Attachment
            On Wed, Aug 5, 2009 at 12:28 AM, Phlip<phlip2005@...> wrote:
            > When pairing often, bugs cause code reviews. These reviews now have teeth - they
            > are not about coding aphorisms, they are about details that actually went wrong.

            And when code reviews are about aphorisms, they're really about those
            "in the team" enforcing (dictating?) team culture to those just
            currently "on the team." And while I freely admit that code reviews
            are useful, I hate them too - when they too often take this toothless
            (from a quality perspective) form.

            Incidentally, I am not passing judgment one way or the other on the
            "in-teaming" behavior per se (I think it's inevitable in some form);
            it's just that via code reviews, its very inefficient and often
            produces reactions such as Phlip is describing - which ironically
            undermines team cohesion (not that this deters those who love to get
            your code up on a projector and berate you about your adherence to the
            coding standard in front of an admiring crowd).

            Best,
            Bill
          • Phlip
            ... Ding! (BTW, nothing personal, but Kanban is on my must eternally ignore and never learn what it is list... Hence the distraction!)
            Message 5 of 23 , Aug 5, 2009
            • 0 Attachment
              Bill Caputo wrote:

              > And when code reviews are about aphorisms, they're really about those
              > "in the team" enforcing (dictating?) team culture to those just
              > currently "on the team."

              Ding!

              (BTW, nothing personal, but Kanban is on my "must eternally ignore and never
              learn what it is" list... Hence the distraction!)
            • Chris Wheeler
              ... Ya, I m sure it was the rules you broke, and not the content of your comment, that pissed off your manager. Chris. [Non-text portions of this message have
              Message 6 of 23 , Aug 5, 2009
              • 0 Attachment
                On Mon, Aug 3, 2009 at 1:12 AM, Phlip <phlip2005@...> wrote:

                > . A while later he caught me writing yet another comment. Both comments
                > said the same thing: "This code has a fixable issue, but I'm too busy
                > getting
                > micromanaged to do anything about it."
                >


                Ya, I'm sure it was the rules you broke, and not the content of your
                comment, that pissed off your manager.

                Chris.


                [Non-text portions of this message have been removed]
              • Phlip
                ... Of course I didn t actually say that. And you have been scornful of me for a while here - please look to your own behavior!
                Message 7 of 23 , Aug 5, 2009
                • 0 Attachment
                  >> . A while later he caught me writing yet another comment. Both comments
                  >> said the same thing: "This code has a fixable issue, but I'm too busy
                  >> getting
                  >> micromanaged to do anything about it."
                  >
                  > Ya, I'm sure it was the rules you broke, and not the content of your
                  > comment, that pissed off your manager.
                  >
                  > Chris.

                  Of course I didn't actually say that. And you have been scornful of me for a
                  while here - please look to your own behavior!
                • Villanueva, Vincent
                  yes, i really like it as well. thanks, vince ________________________________ From: extremeprogramming@yahoogroups.com
                  Message 8 of 23 , Aug 5, 2009
                  • 0 Attachment
                    yes, i really like it as well.
                    thanks,
                    vince

                    ________________________________

                    From: extremeprogramming@yahoogroups.com
                    [mailto:extremeprogramming@yahoogroups.com] On Behalf Of Phlip
                    Sent: Wednesday, August 05, 2009 12:18 PM
                    To: extremeprogramming@yahoogroups.com
                    Subject: [XP] Re: I still hate code reviews




                    >> . A while later he caught me writing yet another comment. Both
                    comments
                    >> said the same thing: "This code has a fixable issue, but I'm too busy
                    >> getting
                    >> micromanaged to do anything about it."
                    >
                    > Ya, I'm sure it was the rules you broke, and not the content of your
                    > comment, that pissed off your manager.
                    >
                    > Chris.

                    Of course I didn't actually say that. And you have been scornful of me
                    for a
                    while here - please look to your own behavior!






                    [Non-text portions of this message have been removed]
                  • Ron Jeffries
                    Hello, Chris. On Wednesday, August 5, 2009, at 2:13:11 PM, you ... If I didn t know you so well, I d have read that as sarcastic ... Ron Jeffries
                    Message 9 of 23 , Aug 5, 2009
                    • 0 Attachment
                      Hello, Chris. On Wednesday, August 5, 2009, at 2:13:11 PM, you
                      wrote:

                      > Ya, I'm sure it was the rules you broke, and not the content of your
                      > comment, that pissed off your manager.

                      If I didn't know you so well, I'd have read that as sarcastic ...

                      Ron Jeffries
                      www.XProgramming.com
                      www.xprogramming.com/blog
                      We know less about the project today than at any time in the future.
                      -- Chet Hendrickson
                      You mean today is the dumbest day of the rest of my life?
                      -- Ron Jeffries
                    • Chris Wheeler
                      ... I m not scornful of you: your posts are often left of centre and slightly bizarre, and my brain has a hard time interpreting what you are saying. And I
                      Message 10 of 23 , Aug 5, 2009
                      • 0 Attachment
                        On Wed, Aug 5, 2009 at 3:18 PM, Phlip <phlip2005@...> wrote:

                        > >> . A while later he caught me writing yet another comment. Both comments
                        > >> said the same thing: "This code has a fixable issue, but I'm too busy
                        > >> getting
                        > >> micromanaged to do anything about it."
                        > >
                        > > Ya, I'm sure it was the rules you broke, and not the content of your
                        > > comment, that pissed off your manager.
                        > >
                        > > Chris.
                        >
                        > Of course I didn't actually say that. And you have been scornful of me for
                        > a
                        > while here - please look to your own behavior!
                        >

                        I'm not scornful of you: your posts are often left of centre and slightly
                        bizarre, and my brain has a hard time interpreting what you are saying. And
                        I *did* look to my own behaviour - I've written very similar things in code
                        comments in the past - including, but not limited to orders for pizza and
                        reviews of Van Halen albums - and so I didn't think it out of the realm of
                        possibility that you would have done the same thing!

                        It would have been wrong for me to pin bad-manager behaviours on my boss for
                        criticizing my comments.

                        Chris.


                        [Non-text portions of this message have been removed]
                      • David Morash
                        ... Hash: SHA1 ... Okay, I follow. For us, bugs don t trigger code reviews, they do trigger an analysis of the cause and may result in additional work to
                        Message 11 of 23 , Aug 5, 2009
                        • 0 Attachment
                          -----BEGIN PGP SIGNED MESSAGE-----
                          Hash: SHA1

                          Phlip wrote:
                          > David Morash wrote:
                          >
                          >> I don't quite follow, are you suggesting we should code review more
                          >> often versus pairing more often?
                          >
                          > When pairing often, bugs cause code reviews. These reviews now have teeth - they
                          > are not about coding aphorisms, they are about details that actually went wrong.
                          >
                          > If the general code reviewing hurts, however, do it more often.
                          >

                          Okay, I follow. For us, bugs don't trigger code reviews, they do
                          trigger an analysis of the cause and may result in additional work to
                          "fix" other areas but the team members just do this, there is no
                          enforcement.

                          I can't see how a review would be more effective than simply fixing the
                          defect and other areas suffering the same problem AND communicating that
                          to the team. I don't see why a team would choose reviews over that,
                          unless they really didn't trust each others abilities.

                          Now I have been on teams where I didn't trust the other team members
                          abilities and vice versa, but reviews don't fix that.
                          -----BEGIN PGP SIGNATURE-----
                          Version: GnuPG v1.4.7 (MingW32)
                          Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

                          iD8DBQFKeiMjV8hFFknmM/sRAotiAKC+BQ3eaF5Am8Ax5dOMhyVjIUGHOwCgijov
                          mUI3+l8XBQsjuR0duasCbRo=
                          =O6/0
                          -----END PGP SIGNATURE-----
                        • David Morash
                          ... Hash: SHA1 ... Maybe everyone could just assume good intent on the part of others? ... Version: GnuPG v1.4.7 (MingW32) Comment: Using GnuPG with Mozilla -
                          Message 12 of 23 , Aug 5, 2009
                          • 0 Attachment
                            -----BEGIN PGP SIGNED MESSAGE-----
                            Hash: SHA1

                            Ron Jeffries wrote:
                            > Hello, Chris. On Wednesday, August 5, 2009, at 2:13:11 PM, you
                            > wrote:
                            >
                            >> Ya, I'm sure it was the rules you broke, and not the content of your
                            >> comment, that pissed off your manager.
                            >
                            > If I didn't know you so well, I'd have read that as sarcastic ...
                            >

                            Maybe everyone could just assume good intent on the part of others?
                            -----BEGIN PGP SIGNATURE-----
                            Version: GnuPG v1.4.7 (MingW32)
                            Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

                            iD8DBQFKeiMtV8hFFknmM/sRAoLlAKCRlHanQoCcRbcWX1qYVP7ZqO2wmQCcCV9f
                            sJuS1UlbGuZGBAYAo2uruhw=
                            =ebjj
                            -----END PGP SIGNATURE-----
                          • Adam Sroka
                            On Wed, Aug 5, 2009 at 8:04 AM, Bill ... Right. It s a bully pulpit. There is no value in that. The point of sharing your code and your insights with one
                            Message 13 of 23 , Aug 5, 2009
                            • 0 Attachment
                              On Wed, Aug 5, 2009 at 8:04 AM, Bill
                              Caputo<list-subscriber@...> wrote:
                              >
                              >
                              > On Wed, Aug 5, 2009 at 12:28 AM, Phlip<phlip2005@...> wrote:
                              >> When pairing often, bugs cause code reviews. These reviews now have teeth
                              >> - they
                              >> are not about coding aphorisms, they are about details that actually went
                              >> wrong.
                              >
                              > And when code reviews are about aphorisms, they're really about those
                              > "in the team" enforcing (dictating?) team culture to those just
                              > currently "on the team." And while I freely admit that code reviews
                              > are useful, I hate them too - when they too often take this toothless
                              > (from a quality perspective) form.
                              >

                              Right. It's a bully pulpit. There is no value in that. The point of
                              sharing your code and your insights with one another is to facilitate
                              communication. These sorts of code reviews are likely to have the
                              opposite effect.

                              > Incidentally, I am not passing judgment one way or the other on the
                              > "in-teaming" behavior per se (I think it's inevitable in some form);
                              > it's just that via code reviews, its very inefficient and often
                              > produces reactions such as Phlip is describing - which ironically
                              > undermines team cohesion (not that this deters those who love to get
                              > your code up on a projector and berate you about your adherence to the
                              > coding standard in front of an admiring crowd).
                              >

                              If we are pair programming and sitting together then why can't we just
                              bring it up when we see it? "Hey, this code stinks!" And have the
                              conversation then. Otherwise, when we have a need to change it we just
                              change it. Why beat a dead horse?
                            • Adam Sroka
                              ... Crazy talk ;-)
                              Message 14 of 23 , Aug 5, 2009
                              • 0 Attachment
                                On Wed, Aug 5, 2009 at 5:26 PM, David Morash<davidmorash@...> wrote:
                                >
                                >
                                > -----BEGIN PGP SIGNED MESSAGE-----
                                > Hash: SHA1
                                >
                                > Ron Jeffries wrote:
                                >> Hello, Chris. On Wednesday, August 5, 2009, at 2:13:11 PM, you
                                >> wrote:
                                >>
                                >>> Ya, I'm sure it was the rules you broke, and not the content of your
                                >>> comment, that pissed off your manager.
                                >>
                                >> If I didn't know you so well, I'd have read that as sarcastic ...
                                >>
                                >
                                > Maybe everyone could just assume good intent on the part of others?

                                Crazy talk ;-)
                              • Jeff Grigg
                                ... I have to admit that the original post made me think that the actual comment in the actual code *DID* say something like that. ... Me too. It was a bad
                                Message 15 of 23 , Aug 5, 2009
                                • 0 Attachment
                                  >>> --- Phlip <phlip2005@...> wrote:
                                  >>>> [...] Both comments said the same thing: "This code has a
                                  >>>> fixable issue, but I'm too busy getting micromanaged to do
                                  >>>> anything about it."

                                  >> -- Chris.
                                  >>> Ya, I'm sure it was the rules you broke, and not the content
                                  >>> of your comment, that pissed off your manager.

                                  > --- Phlip <phlip2005@...> wrote:
                                  >> Of course I didn't actually say that. [...]

                                  --- Chris Wheeler <christopher.wheeler@...> wrote:
                                  > [...] my brain has a hard time interpreting what you are saying.

                                  I have to admit that the original post made me think that the actual comment in the actual code *DID* say something like that.

                                  > [...] - I've written very similar things in code comments
                                  > in the past - [...] - and so I didn't think it out of the
                                  > realm of possibility that you would have done the same thing!

                                  Me too. It was a bad idea. And she caught me. Very embarrassing.


                                  > It would have been wrong for me to pin bad-manager behaviors
                                  > on my boss for criticizing my comments.

                                  "So you want me to get rid of the TODO comments? Really? Well, OK, but you realize that I'll have to ***DO*** what it says in the TODO comments -- and that that may take a while. As long as you're OK with the cost and the schedule delay, I'm *always* up for doing more work. Honestly, I am; I just thought that it wouldn't provide sufficient ROI at this time -- that's why I deferred it. But if you want that work done now, I'd be glad to do it."

                                  On the other hand, if it's a "this is why it's like this" comment, I'd ask what they'd like me to do -- other than just to delete the comment.
                                • Adam Sroka
                                  ... ... FWIW, there is almost always a better place to put TODO comments than as comments in the code. For example both Perl and Ruby support TODO
                                  Message 16 of 23 , Aug 5, 2009
                                  • 0 Attachment
                                    On Wed, Aug 5, 2009 at 7:34 PM, Jeff Grigg<jeffgrigg@...> wrote:
                                    >
                                    < snip >

                                    > "So you want me to get rid of the TODO comments? Really? Well, OK, but you
                                    > realize that I'll have to ***DO*** what it says in the TODO comments -- and
                                    > that that may take a while. As long as you're OK with the cost and the
                                    > schedule delay, I'm *always* up for doing more work. Honestly, I am; I just
                                    > thought that it wouldn't provide sufficient ROI at this time -- that's why I
                                    > deferred it. But if you want that work done now, I'd be glad to do it."
                                    >

                                    FWIW, there is almost always a better place to put "TODO" comments
                                    than as comments in the code. For example both Perl and Ruby support
                                    "TODO" in a test (Perl has TODO {} blocks via Test::Harness. RSpec has
                                    pending().) I don't like TODO comments in code for the same reason I
                                    don't like comments generally: they don't help me understand the code
                                    and they have no executable semantics.

                                    > On the other hand, if it's a "this is why it's like this" comment, I'd ask
                                    > what they'd like me to do -- other than just to delete the comment.
                                    >
                                    >
                                  • George Dinwiddie
                                    ... TODO comments work pretty well in Eclipse, where they appear in the task list. At least, they work pretty well if you don t let them persist. I like to
                                    Message 17 of 23 , Aug 5, 2009
                                    • 0 Attachment
                                      Adam Sroka wrote:
                                      > FWIW, there is almost always a better place to put "TODO" comments
                                      > than as comments in the code. For example both Perl and Ruby support
                                      > "TODO" in a test (Perl has TODO {} blocks via Test::Harness. RSpec has
                                      > pending().) I don't like TODO comments in code for the same reason I
                                      > don't like comments generally: they don't help me understand the code
                                      > and they have no executable semantics.

                                      TODO comments work pretty well in Eclipse, where they appear in the task
                                      list. At least, they work pretty well if you don't let them persist. I
                                      like to make them all go away before calling the story done. With
                                      legacy code, that's not always possible (and I'll generally use a
                                      different task tag, such as SMELL).

                                      - George

                                      --
                                      ----------------------------------------------------------------------
                                      * George Dinwiddie * http://blog.gdinwiddie.com
                                      Software Development http://www.idiacomputing.com
                                      Consultant and Coach http://www.agilemaryland.org
                                      ----------------------------------------------------------------------
                                    • Ron Jeffries
                                      Hello, David. On Wednesday, August 5, 2009, at 8:26:21 PM, you ... Were you modeling that behavior just then? :) Ron Jeffries www.XProgramming.com
                                      Message 18 of 23 , Aug 5, 2009
                                      • 0 Attachment
                                        Hello, David. On Wednesday, August 5, 2009, at 8:26:21 PM, you
                                        wrote:

                                        >>> Ya, I'm sure it was the rules you broke, and not the content of your
                                        >>> comment, that pissed off your manager.
                                        >>
                                        >> If I didn't know you so well, I'd have read that as sarcastic ...

                                        > Maybe everyone could just assume good intent on the part of others?

                                        Were you modeling that behavior just then? :)

                                        Ron Jeffries
                                        www.XProgramming.com
                                        www.xprogramming.com/blog
                                        Analysis kills spontaneity.
                                        The grain once ground into flour germinates no more. -- Henri Amiel
                                      • Adam Sroka
                                        ... True, but they also show up if you put them in the tests ;-) You can also create task lists independent of the code in Eclipse. Or, something I have done
                                        Message 19 of 23 , Aug 5, 2009
                                        • 0 Attachment
                                          On Wed, Aug 5, 2009 at 8:04 PM, George Dinwiddie<lists@...> wrote:
                                          >
                                          >
                                          > Adam Sroka wrote:
                                          >> FWIW, there is almost always a better place to put "TODO" comments
                                          >> than as comments in the code. For example both Perl and Ruby support
                                          >> "TODO" in a test (Perl has TODO {} blocks via Test::Harness. RSpec has
                                          >> pending().) I don't like TODO comments in code for the same reason I
                                          >> don't like comments generally: they don't help me understand the code
                                          >> and they have no executable semantics.
                                          >
                                          > TODO comments work pretty well in Eclipse, where they appear in the task
                                          > list. At least, they work pretty well if you don't let them persist. I
                                          > like to make them all go away before calling the story done. With
                                          > legacy code, that's not always possible (and I'll generally use a
                                          > different task tag, such as SMELL).
                                          >

                                          True, but they also show up if you put them in the tests ;-) You can
                                          also create task lists independent of the code in Eclipse. Or,
                                          something I have done in the past is to create my own @Todo annotation
                                          class to use with JUnit tests.
                                        • David Morash
                                          ... Hash: SHA1 ... I hope so! Sorry, that wasn t directed at any one person (but I didn t make that clear). I just felt like there was bit of tension at parts
                                          Message 20 of 23 , Aug 5, 2009
                                          • 0 Attachment
                                            -----BEGIN PGP SIGNED MESSAGE-----
                                            Hash: SHA1

                                            Ron Jeffries wrote:
                                            > Hello, David. On Wednesday, August 5, 2009, at 8:26:21 PM, you
                                            > wrote:
                                            >
                                            >>>> Ya, I'm sure it was the rules you broke, and not the content of your
                                            >>>> comment, that pissed off your manager.
                                            >>> If I didn't know you so well, I'd have read that as sarcastic ...
                                            >
                                            >> Maybe everyone could just assume good intent on the part of others?
                                            >
                                            > Were you modeling that behavior just then? :)

                                            I hope so!

                                            Sorry, that wasn't directed at any one person (but I didn't make that
                                            clear). I just felt like there was bit of tension at parts of this
                                            thread, but to quote a useful phrase I've learned here, "I could be
                                            wrong. I often am."

                                            -----BEGIN PGP SIGNATURE-----
                                            Version: GnuPG v1.4.9 (GNU/Linux)
                                            Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

                                            iEYEARECAAYFAkp6VdQACgkQV8hFFknmM/v2UACgveFxwyph26tntkTQ1G8iIoT9
                                            LQMAoL2/eHeRImoz2l10sZAxtDJEL2GO
                                            =rmcJ
                                            -----END PGP SIGNATURE-----
                                          • Tim Ottinger
                                            Sometimes the rules need to be changed.Maybe the OP should consider using the next retrospective as a place & time to change the rules. Of course I think that
                                            Message 21 of 23 , Aug 11, 2009
                                            • 0 Attachment
                                              Sometimes the rules need to be changed.Maybe the OP should consider using the next retrospective as a place & time to change the rules.
                                              Of course I think that my rules on comments are the most reasonable: http://agileinaflash.blogspot.com/2009/04/rules-for-commenting.html

                                              I used to teach code review techniques, and I believed very strongly in their use. Then I got over it somehow.

                                              I've only seen reviews remain profitable in non-agile teams. In a proper agile team with no silos and promiscuous pairing, one may find some issues, but at a cost greater than the benefit received. Understandably life-critical systems are special. Is the code review a poor substitute for having good ATs/UTs and more eyes on-task? Would you catch anything in the code review that shouldn't be caught in a test?

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