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

RE: [XP] Re: I still hate code reviews

Expand Messages
  • Villanueva, Vincent
    yes, i really like it as well. thanks, vince ________________________________ From: extremeprogramming@yahoogroups.com
    Message 1 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]
    • 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 2 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 3 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 4 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 5 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 6 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 7 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 8 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 9 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 10 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 11 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 12 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 13 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.