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

Re: [XP] Re: Suggestion/ideas for refactoring?

Expand Messages
  • Paul Michali
    ... Wow! What a good memory you have. Yes, this is that same application. Whenever I get free time, I go back to this and tweak it and try new things. My
    Message 1 of 7 , Sep 27, 2001
    • 0 Attachment
      kevinxp@... wrote:
      >
      > This algorithm reminds me of a certain program to send commuting
      > traffic notifications to a pager... :-)

      Wow! What a good memory you have. Yes, this is that same application.
      Whenever I get free time, I go back to this and tweak it and try new
      things. My latest kick has been to add message filtering, so that the
      pager messages are shorter. I'm also trying to learn how to use Ant,
      CruiseControl, CVS, and get continuous integration going. It is so
      refreshing to play with Java, after a few months of C and ASM coding.


      > I would probably try to make it more modular, passing more parameters
      > and relying on object members less. It also seems like there really
      > wants to be a class that represnts the current and old versions of
      > each sample, rather than a class representing all the samples at a
      > given moment. Yeah, I'd probably head that way.

      Hmm. Right now I have a data member that holds the previous primary
      report and an array that holds the previous secondary reports. I then
      extract (and create local objects) to hold the current reports for
      comparing.

      I wonder how I should go with this? Have some kind of ReportHistory
      object that maintains the last report that caused a notification?
      Maybe it holds the current sample too? Keep these as members of this
      class or make them self contained entities that this class uses?

      Right now, the design creates a Scout class that controls the whole
      sampling and notifying process.

      > I would, of course, recode it into Java and out of this really ugly
      > pseudocode cr*p. ...

      You can check out my response to Kent, where I gave the Java source
      for the current implementation of the algorithm.

      In looking at your pseudo code below, am I correct in observing that
      you're doing a "Decompose Conditional" refactoring to move some of
      the conditional logic into separate methods and then handling some of
      the complexity down in those methods? It also looks like you've
      broken down the conditions a bit differently (I check to see if the
      primary is still bad, which compares current to previous - you check
      current and then compare it in the called method). I'm not sure I
      see an advantage in doing it that way (not that there isn't an
      advantage, it's just that one isn't so obvious that even I can see
      it :^).

      Thanks for the effort as it is giving me something to chew on...

      > Report:
      > IF current.P1.cond is bad THEN
      > ReportP1StatusChanges
      > ReportAllSxChanges
      > ELSE IF old.P1.cond is bad THEN
      > report current.P1
      > ENDIF
      >
      > ReportP1StatusChanges:
      > IF current.P1.statusMsg != old.P1.statusMsg THEN
      > report current.P1
      > ENDIF
      >
      > ReportAllSxChanges:
      > FOR each of the secondary samples
      > IF IsSxWorthReporting(x)
      > report current.S#
      > ENDIF
      > ENDFOR
      >
      > IsSxWorthReporting(x):
      > IF old.P1.cond is bad && HasSxChanged(x) THEN
      > return TRUE
      > ENDIF
      > IF current.S(x).status is NOT empty THEN
      > return TRUE
      > ENDIF
      > return FALSE
      >
      > HasSxChanged(x):
      > IF current.S(x).cond != previous.S(x).cond THEN
      > return TRUE
      > ENDIF
      > IF current.S(x).status != previous.S(x).status THEN
      > return TRUE
      > ENDIF
      > return FALSE
      >


      PCM (Paul Michali)

      Carrier Voice Gateway Business Unit (CVGBU)
      Cisco Systems, Inc.
      250 Apollo Drive
      Chelmsford, MA 01824

      Phone : (800) 572-6771 x 45817 (978) 244-5817 [direct]
      Paging: (800) 365-4578 [voice] pcm@... [email page]
    • kevinxp@qualitycode.com
      ... It seems like the basic class should contain a single condition/status pair. I think this is what you call a Report. Above that, the code seems to be
      Message 2 of 7 , Sep 27, 2001
      • 0 Attachment
        --- In extremeprogramming@y..., Paul Michali <pcm@c...> wrote:
        > I wonder how I should go with this? Have some kind of ReportHistory
        > object that maintains the last report that caused a notification?
        > Maybe it holds the current sample too? Keep these as members of this
        > class or make them self contained entities that this class uses?

        It seems like the basic class should contain a single condition/status
        pair. I think this is what you call a Report. Above that, the code
        seems to be asking me for a class that contains (at least) the current
        and previous reports for a given...route? I could see naming this
        class ReportHistory.

        > In looking at your pseudo code below, am I correct in observing that
        > you're doing a "Decompose Conditional" refactoring to move some of
        > the conditional logic into separate methods and then handling some of
        > the complexity down in those methods? It also looks like you've
        > broken down the conditions a bit differently (I check to see if the
        > primary is still bad, which compares current to previous - you check
        > current and then compare it in the called method). I'm not sure I
        > see an advantage in doing it that way (not that there isn't an
        > advantage, it's just that one isn't so obvious that even I can see
        > it :^).

        I had two goals in my refactoring: 1) eliminate duplication, and 2)
        clear naming. I suspect the changes you mention were triggered by
        seeing the same test appear in two places. I eliminated all the
        duplication that I could first, and then went back and chose
        reasonable names for the helper methods.

        I think I converted the "still bad" into a test for current !=
        previous, to be more explicit. After I did that, I could see that it
        really was duplicate code, so I moved around (I think).

        Kevin
      • Paul Michali
        ... It looks like it is better for me to have the current report and the last report that caused a notification. I haven t decided if I should have a class
        Message 3 of 7 , Sep 28, 2001
        • 0 Attachment
          kevinxp@... wrote:
          >
          > --- In extremeprogramming@y..., Paul Michali <pcm@c...> wrote:
          > > I wonder how I should go with this? Have some kind of ReportHistory
          > > object that maintains the last report that caused a notification?
          > > Maybe it holds the current sample too? Keep these as members of this
          > > class or make them self contained entities that this class uses?
          >
          > It seems like the basic class should contain a single condition/status
          > pair. I think this is what you call a Report. Above that, the code
          > seems to be asking me for a class that contains (at least) the current
          > and previous reports for a given...route? I could see naming this
          > class ReportHistory.

          It looks like it is better for me to have the current report and the
          last report that caused a notification. I haven't decided if I should
          have a class that holds both of those items or to just have an array of
          "last" reports, which is sorta what I'm doing now.



          > I had two goals in my refactoring: 1) eliminate duplication, and 2)
          > clear naming. I suspect the changes you mention were triggered by
          > seeing the same test appear in two places. I eliminated all the
          > duplication that I could first, and then went back and chose
          > reasonable names for the helper methods.
          >
          > I think I converted the "still bad" into a test for current !=
          > previous, to be more explicit. After I did that, I could see that it
          > really was duplicate code, so I moved around (I think).


          I decided to follow your lead and I took the original method with
          all the if statements and pulled out parts into methods. I too, started
          to notice that there was some duplication, or near duplication in the
          resulting methods. I worked a bit to reduce the duplication and now
          have a few more ideas that I can employ. Thanks for the info, as it
          has given me ideas on how to approach the refactoring.

          Here's what I had originally (w/o the debug calls):

          public void checkPathSegments() {
          dispatcher.clearMessage();

          BasicReport primaryCurr = getLatestReportFor(PRIMARY_PATH);
          BasicReport primaryPrev = getPreviousReportFor(PRIMARY_PATH);

          if ( primaryCurr.conditionStillBad(primaryPrev) ) {

          if ( messageChangeToReport(primaryCurr, primaryPrev) ) {
          addToNotificationMessage(primaryCurr);
          }

          for (int i = 1; i < segments.size(); i++) {

          BasicReport secondaryCurr = getLatestReportFor(i);
          BasicReport secondaryPrev = getPreviousReportFor(i);

          if ( secondaryCurr.conditionChanged(secondaryPrev) ||
          messageChangeToReport(secondaryCurr, secondaryPrev)
          ) {
          addToNotificationMessage(secondaryCurr);
          saveAsPreviousReport(secondaryCurr, i);
          }
          }
          } else if ( primaryCurr.conditionStillOK(primaryPrev) ) {

          cat.debug("Primary still OK");

          } else if ( primaryCurr.conditionNowOK(primaryPrev) ) {

          addToNotificationMessage(primaryCurr);

          } else { // Condition has changed, but not to OK

          if ( validMessage(primaryCurr) ) {
          addToNotificationMessage(primaryCurr);
          }

          for (int i = 1; i < segments.size(); i++) {

          BasicReport secondaryCurr = getLatestReportFor(i);
          if ( validMessage(secondaryCurr) ) {
          addToNotificationMessage(secondaryCurr);
          saveAsPreviousReport(secondaryCurr, i);
          }
          }
          }

          saveAsPreviousReport(primaryCurr, PRIMARY_PATH); // ****
          dispatcher.notifyObservers();
          }

          -------------------------------------------------------------------
          Here's what I have now (still a work in progress, of course):


          public void makeObservations() {

          cat.debug("Taking observations");
          dispatcher.clearMessage();

          checkAllPathSegments();

          dispatcher.notifyObservers();
          cat.debug("Observations complete");

          }

          public void checkAllPathSegments() {
          BasicReport primaryCurr = getLatestReportFor(PRIMARY_PATH);
          BasicReport primaryPrev = getPreviousReportFor(PRIMARY_PATH);

          if ( primaryCurr.conditionStillBad(primaryPrev) ) {

          reportAnyChangeInPrimary(primaryCurr, primaryPrev);
          reportAnyChangeInSecondaries();

          } else if ( primaryCurr.conditionStillOK(primaryPrev) ) {

          cat.debug("Primary still OK");

          } else if ( primaryCurr.conditionNowOK(primaryPrev) ) {

          addReportToNotification(primaryCurr, PRIMARY_PATH);

          } else { // Condition has changed, but not to OK

          addValidReportFor(primaryCurr, PRIMARY_PATH);
          addValidSecondaryReports();
          }

          }


          public void reportAnyChangeInPrimary(BasicReport curr, BasicReport
          prev) {

          if ( messageChangeToReport(curr, prev) ) {
          cat.debug("Primary path message changed");
          addReportToNotification(curr, PRIMARY_PATH);
          }
          }

          public void reportAnyChangeInSecondaries() {
          for (int i = 1; i < segments.size(); i++) {

          BasicReport secondaryCurr = getLatestReportFor(i);
          BasicReport secondaryPrev = getPreviousReportFor(i);

          if ( secondaryCurr.conditionChanged(secondaryPrev) ||
          messageChangeToReport(secondaryCurr, secondaryPrev) ) {
          cat.debug("A secondary path condition or message
          changed");
          addReportToNotification(secondaryCurr, i);
          }
          }
          }


          public void addValidReportFor(BasicReport report, int whichPath) {
          if ( validMessage(report) ) {
          addReportToNotification(report, whichPath);
          }
          }

          public void addValidSecondaryReports() {
          for (int i = 1; i < segments.size(); i++) {

          BasicReport secondaryCurr = getLatestReportFor(i);
          if ( validMessage(secondaryCurr) ) {
          addReportToNotification(secondaryCurr, i);
          }
          }
          }


          public void addReportToNotification(BasicReport report, int
          whichPath) {
          cat.debug("Adding notification for path " + report.name());
          addToNotificationMessage(report);
          saveAsPreviousReport(report, whichPath);
          }



          -------------------------------------------------------------------
          In looking at the above code, I see a few things that I want to
          look into.

          - Maybe passing the dispatcher object that does the notification in
          as an argument, instead of maintaining as a data member?

          - May want to add the primary report to the same array that holds
          all the secondary reports, so that it can be treated in the same
          manner (e.g. index zero could be the primary and the secondary
          reports start at index 1).

          - Join addValidReport() and addValidSecondaryReport(), if use an
          array to treat the primary report the same as others.

          It does seem clearer (to me) so far. I'll keep plugging away at it.
          But, right now I've got to work on updating the test cases...


          PCM (Paul Michali)

          Carrier Voice Gateway Business Unit (CVGBU)
          Cisco Systems, Inc.
          250 Apollo Drive
          Chelmsford, MA 01824

          Phone : (800) 572-6771 x 45817 (978) 244-5817 [direct]
          Paging: (800) 365-4578 [voice] pcm@... [email page]
        • Adam Judson
          ... Some thoughts. ... (PRIMARY_PATH); ... (PRIMARY_PATH); I see this and I want to change it to : primaryReport.getNewData(); primaryReport.getPreviousData();
          Message 4 of 7 , Sep 28, 2001
          • 0 Attachment
            --- In extremeprogramming@y..., Paul Michali <pcm@c...> wrote:
            > Here's what I have now (still a work in progress, of course):
            >

            Some thoughts.

            > public void checkAllPathSegments() {
            > BasicReport primaryCurr = getLatestReportFor
            (PRIMARY_PATH);
            > BasicReport primaryPrev = getPreviousReportFor
            (PRIMARY_PATH);

            I see this and I want to change it to :

            primaryReport.getNewData();
            primaryReport.getPreviousData();

            I don't think I should be getting the data FOR the report object,
            I think it should be gettting it itself.

            Hmmm, having said this, I don't really think this is what I want.
            We probably need some sort of external agent to get the info
            for us (HTML pages are the source no?)

            It doesn't seem like report, and HTML parsing have a lot in common...


            >
            > if ( primaryCurr.conditionStillBad(primaryPrev) ) {
            >
            > reportAnyChangeInPrimary(primaryCurr, primaryPrev);
            > reportAnyChangeInSecondaries();
            >

            - why am I passing data to an object to ask about the data? This data
            and the logic feel like they belong together.

            - I'm just comming off of building a transaction system, and it was
            quite handy to keep the construction of the message seperate from
            the protocol object we used to send/receive the transaction.
            The other thing that helped was building a hierachy of transaction
            elements, to build transactions from. The "split" object knew
            how to examine a specific piece of input data to determine which
            of several paths the rest of the transaction should take.
            e.g. we looked at the type of customer to determine if we then
            send the commercial, or personal information.

            This is making me see your project as kind of the same thing.

            You have a path. This path knows how to check it's status (populate
            itself), either by using some sort of external agent, or some sort
            of standard input (string). (I'd favor the latter because it allows
            us to keep the agent completely seperate.)

            The path is made up of several parts, the primary path, and the
            secondary path(s). Maybe these paths are made of pieces too
            (different sections of road?).

            The path knows how to produce a notification report (probably just
            a string? - "Primary is clear"). This composite path object keeps
            track of what the state used to be, and what it is now, and
            understands the rules for when to produce a notification report.

            (Okay, so this turned out not to be a refactoring suggestion after all
            but I've started so I may as well finish).

            So what I'm thinking is:

            class PathElement {
            public void populate(String data);
            public String getNotificationString();
            private PathData ivPreviousData;
            private PathData ivCurrentData;
            }

            class PathData {
            public Boolean isWorse(PathData data);
            public Boolean isBetter(PathData data);
            public String description();
            }

            class CompositePath extends PathElement {
            public void populate(String data);
            public String getNotificationString();
            public add(PathElement element);
            }

            class MyPath extends SplitPath {
            public MyPath(){
            addPrimary (new MyPrimaryPath());
            addSecondary(new MySecondary1());
            addSecondary(new MySecondary2());
            }
            public hasChanged() {
            return !ivOldNotificationString.equals(getNotificationString());
            }
            }


            main() {
            HTMLAgent agent = new HTMLAgent();
            MyPath path = new MyPath();
            NotificationAgent notifier = new NotificationAgent();
            while (true){
            String data = agent.getData();
            path.populate();
            if (path.hasChanged()) {
            String notification = path.getNotificationString();
            notifier.notify(notification);
            }
            }
            }

            I hope I haven't misunderstood the problem to badly.

            And having written this, I'm not sure how I'd get here from where
            you are in small steps. So maybe this isn't helpfull after all.

            Adam
          • kevinxp@qualitycode.com
            ... Would this be clearer if you passed a message object around? This message would be cleared, populated, and then sent to observers. ... Whew. Still kind
            Message 5 of 7 , Sep 29, 2001
            • 0 Attachment
              --- In extremeprogramming@y..., Paul Michali <pcm@c...> wrote:
              > Here's what I have now (still a work in progress, of course):
              >
              >
              > public void makeObservations() {
              > dispatcher.clearMessage();
              > checkAllPathSegments();
              > dispatcher.notifyObservers();
              > }

              Would this be clearer if you passed a "message" object around? This message
              would be cleared, populated, and then sent to observers.

              >
              > public void checkAllPathSegments() {
              > BasicReport primaryCurr = getLatestReportFor(PRIMARY_PATH);
              > BasicReport primaryPrev = getPreviousReportFor(PRIMARY_PATH);
              > if ( primaryCurr.conditionStillBad(primaryPrev) ) {
              > reportAnyChangeInPrimary(primaryCurr, primaryPrev);
              > reportAnyChangeInSecondaries();
              > } else if ( primaryCurr.conditionStillOK(primaryPrev) ) {
              > cat.debug("Primary still OK");
              > } else if ( primaryCurr.conditionNowOK(primaryPrev) ) {
              > addReportToNotification(primaryCurr, PRIMARY_PATH);
              > } else { // Condition has changed, but not to OK
              > addValidReportFor(primaryCurr, PRIMARY_PATH);
              > addValidSecondaryReports();
              > }
              > }

              Whew. Still kind of a big method. Seems like the getLatest... and getPrevious
              belong in a different method from all the rest.

              If a single object contained both primaryCurr and primaryPrev, you could ask
              that object whether things were better, the same, or worse, with a simple
              method (and no parameters). Same with secondary.

              What if this method was responsible for marking each report/segment/path as
              "needs to report" or not. Then, a later method could sweep through and only
              build a message from the ones that need to be included.

              > -------------------------------------------------------------------
              > In looking at the above code, I see a few things that I want to
              > look into.
              >
              > - Maybe passing the dispatcher object that does the notification in
              > as an argument, instead of maintaining as a data member?

              For large classes with lots of logic, I tend to view member variables as being
              like globals (bad). Parameters and locals tend to improve testability and reuse,
              and most important: clarity. Often, they are a hint that the class shouldn't be
              so big.

              > - May want to add the primary report to the same array that holds
              > all the secondary reports, so that it can be treated in the same
              > manner (e.g. index zero could be the primary and the secondary
              > reports start at index 1).

              I really like this, as it avoids a special case. It would probably eliminate a fair
              amount of code.

              > It does seem clearer (to me) so far. I'll keep plugging away at it.
              > But, right now I've got to work on updating the test cases...

              I assume you mean _adding_ test cases. You wouldn't change the code in
              ways that break tests, and then go back and fix the tests, would you? Much
              better to change the tests first, and then update the code to match.

              Fun project.

              Kevin
            • Paul Michali
              ... Yes HTML is the source. In the terminology that I use, I call this web page data a report, because in the domain, it is a report of road conditions. I do
              Message 6 of 7 , Oct 1, 2001
              • 0 Attachment
                Adam Judson wrote:
                >
                > --- In extremeprogramming@y..., Paul Michali <pcm@c...> wrote:
                > > Here's what I have now (still a work in progress, of course):
                > >
                >
                > Some thoughts.
                >
                > > public void checkAllPathSegments() {
                > > BasicReport primaryCurr = getLatestReportFor
                > (PRIMARY_PATH);
                > > BasicReport primaryPrev = getPreviousReportFor
                > (PRIMARY_PATH);
                >
                > I see this and I want to change it to :
                >
                > primaryReport.getNewData();
                > primaryReport.getPreviousData();
                >
                > I don't think I should be getting the data FOR the report object,
                > I think it should be gettting it itself.
                >
                > Hmmm, having said this, I don't really think this is what I want.
                > We probably need some sort of external agent to get the info
                > for us (HTML pages are the source no?)

                Yes HTML is the source. In the terminology that I use, I call this
                web page data a report, because in the domain, it is a report of
                road conditions.

                I do like what you have above. My version has a procedural flavor to
                it and I should probably change that.


                > >
                > > if ( primaryCurr.conditionStillBad(primaryPrev) ) {
                > >
                > > reportAnyChangeInPrimary(primaryCurr, primaryPrev);
                > > reportAnyChangeInSecondaries();
                > >
                >
                > - why am I passing data to an object to ask about the data? This data
                > and the logic feel like they belong together.

                There have been suggestions by others and it has made me think about
                having both a ScoutingReport object and a ReportHistory or some such
                object that knows about current and old conditions for a single path
                (and could tell me if it is still bad, better, worse, etc).


                > I hope I haven't misunderstood the problem to badly.

                Not too far off. Instead of there being one path that is composed of
                several sections, I have multiple, discrete paths, each with their
                own independent status and condition info.


                >
                > And having written this, I'm not sure how I'd get here from where
                > you are in small steps. So maybe this isn't helpfull after all.

                On the contrary. It gave me some other ideas to think about and is
                making me rethink about some of the objects I have and whether they
                are doing what they should be doing.

                Thanks for the reply!


                PCM (Paul Michali)

                Carrier Voice Gateway Business Unit (CVGBU)
                Cisco Systems, Inc.
                250 Apollo Drive
                Chelmsford, MA 01824

                Phone : (800) 572-6771 x 45817 (978) 244-5817 [direct]
                Paging: (800) 365-4578 [voice] pcm@... [email page]
              • Paul Michali
                ... Hmm. I was thinking about creating and passing the dispatcher object around (it know who to send the E-mail messages to, the name of the mail server, and
                Message 7 of 7 , Oct 1, 2001
                • 0 Attachment
                  kevinxp@... wrote:
                  >
                  > --- In extremeprogramming@y..., Paul Michali <pcm@c...> wrote:
                  > > Here's what I have now (still a work in progress, of course):
                  > >
                  > >
                  > > public void makeObservations() {
                  > > dispatcher.clearMessage();
                  > > checkAllPathSegments();
                  > > dispatcher.notifyObservers();
                  > > }
                  >
                  > Would this be clearer if you passed a "message" object around? This message
                  > would be cleared, populated, and then sent to observers.

                  Hmm. I was thinking about creating and passing the dispatcher object
                  around
                  (it know who to send the E-mail messages to, the name of the mail
                  server, and
                  the message that needs to be sent). Now you have me thinking about
                  whether
                  I should pass a dispatcher or pass a Message object and then send it to
                  the
                  dispatcher object (of which the Message is just a "dumb" object that
                  carries
                  the message).



                  > If a single object contained both primaryCurr and primaryPrev, you could ask
                  > that object whether things were better, the same, or worse, with a simple
                  > method (and no parameters). Same with secondary.

                  Yeah. Some sort of ReportHistory object is starting to look better and
                  better. I may give it a try today or tomorrow.


                  >
                  > What if this method was responsible for marking each report/segment/path as
                  > "needs to report" or not. Then, a later method could sweep through and only
                  > build a message from the ones that need to be included.

                  I was thinking about this at one time, but I wasn't sure if it would pan
                  out or not. I'll keep it in the back of my head.


                  > > It does seem clearer (to me) so far. I'll keep plugging away at it.
                  > > But, right now I've got to work on updating the test cases...
                  >
                  > I assume you mean _adding_ test cases. You wouldn't change the code in
                  > ways that break tests, and then go back and fix the tests, would you?

                  Nope. I created a set of parallel methods with new logic that does the
                  same thing as the old. As a result, some of the new methods are not
                  being tested, so I need to create some more test cases to test that
                  logic.


                  > Much
                  > better to change the tests first, and then update the code to match.

                  Yeah you're right (slapping my own hand). I ended up just extracting
                  code to methods and naming them so that they made sense, joined some
                  methods and then started looking at the tests to see how I could make
                  sure I covered these "new" methods. I should work harder towards
                  writing the test first and then coding to meet that test. With the
                  refactoring, it's hard (for me) to see how to write the test before I
                  do the refactoring step. I guess if I had a pair partner, it would
                  really have helped here :^(

                  I also didn't have great test coverage of the original code (a smell),
                  because it did so much (like the original checkAllSegments() method
                  where it collected a new report, compared them, built a message, and
                  sent it out).

                  You have some good ideas that I'm going to pursue, thanks!


                  PCM (Paul Michali)

                  Carrier Voice Gateway Business Unit (CVGBU)
                  Cisco Systems, Inc.
                  250 Apollo Drive
                  Chelmsford, MA 01824

                  Phone : (800) 572-6771 x 45817 (978) 244-5817 [direct]
                  Paging: (800) 365-4578 [voice] pcm@... [email page]
                Your message has been successfully submitted and would be delivered to recipients shortly.