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

Re: [XP] [refactoring] More on: Introduce Parameter Object

Expand Messages
  • Roger Lipscombe
    ... I suspect it s more a problem with the fact that the client code can call show_differences before they ve called set_queries. They ll get unexpected
    Message 1 of 15 , Mar 1, 2001
    • 0 Attachment
      > --- In extremeprogramming@y..., kari.hoijarvi@v... wrote:
      > > Here is an example of mutable member variables, written in pseudo
      > Java and
      > > using pseudo ADO for db access.
      > >
      > > What's wrong with it? Can you spot the bug?
      > >
      > > class query_comparer {
      > > private String m_q1, m_q2:
      > > private Recordset m_r1, m_r2;
      > >
      > > public set_queries(String q1, String q2) throws DBException
      > > {
      > > m_r1 = db.Execute(q1);
      > > m_r2 = db.Execute(q2);
      > > m_q1 = q1;
      > > m_q2 = q2;
      > > }
      > >
      > > public show_differences(......
      > > };
      >
      > Are you referring to the possibility that m_q1 and m_q2 could be
      > initialized incorrectly if the values of q1 and q2 are modified by
      > the respective db.Execute() calls? Just a guess...

      I suspect it's more a problem with the fact that the client code can call
      show_differences before they've called set_queries. They'll get unexpected
      results.

      The correct code - assuming I'm right - (in C++, since I can only read, not
      write Java)...

      using std::string;

      class query_composer {
      const string m_q1, m_q2;
      Recordset m_r1, m_r2;
      public:
      query_composer(string q1, string q2) : m_q1(q1), m_q2(q2)
      {
      m_r1 = db.Execute(q1);
      m_r2 = db.Execute(q2);
      }

      void show_differences(.......
      };

      Now the items are just as private as before, and we've forced the user to
      initialise the object correctly.
    • Eric Bennett
      ... Well there s a suspect colon there, but that s too easy. ... Is it that the object state is not well defined if an exception is thrown? Most suspect part
      Message 2 of 15 , Mar 1, 2001
      • 0 Attachment
        On Thu, 1 Mar 2001 kari.hoijarvi@... wrote:

        > What's wrong with it? Can you spot the bug?
        >
        > class query_comparer {
        > private String m_q1, m_q2:

        Well there's a suspect colon there, but that's too easy.

        > private Recordset m_r1, m_r2;
        >
        > public set_queries(String q1, String q2) throws DBException
        > {
        > m_r1 = db.Execute(q1);
        > m_r2 = db.Execute(q2);
        > m_q1 = q1;
        > m_q2 = q2;
        > }
        >
        > public show_differences(......
        > };

        Is it that the object state is not well defined if an exception is thrown?
        Most suspect part is that the m_q* members are set only if the query
        executions succeed. But whether that's a problem or not depends on
        query_composers' responsibility for handling errors and on who does the
        cleanup.

        - Eric B.

        --
        "Pasteurized From Concentrate"
      • Robert Watkins
        ... That s easy... it s because Java (the language of the examples) passes everything by value. Consider the following code: public void doSomething(int x, int
        Message 3 of 15 , Mar 1, 2001
        • 0 Attachment
          Barbini Uberot wrote:

          > I need more detail on Introduce Parameter Object Refactor ;))
          > This is one of not-trivial refactoring more useful to me.
          >
          > Anyway I disagree about a rule:
          > why pass all data field in constructor and declare them final?

          That's easy... it's because Java (the language of the examples) passes
          everything by value.

          Consider the following code:
          public void doSomething(int x, int y) {
          x = 5; // Deliberately silly step taken.
          y = 3;
          }

          int a = 2;
          int b = 4;
          doSomething(a, b);
          System.out.println(a + ", " + b); // prints "2, 4" not "5, 3".


          Now, a simple use of Introduce Parameter Object would have a change in
          behaviour. Consider:
          public void doSomething(ParameterObject param) {
          param.x = 5;
          param.y = 3;
          }

          int a = 2;
          int b = 4;
          ParameterObject myParam = new ParameterObject(a, b);
          doSomething(param);
          System.out.println(param.x + ", " + param.y); //prints "5, 3", not "2, 4"

          Martin Fowler recommends that you use the refactor
          RemoveAssignmentToParameters, by declaring the parameters final first. This
          gets the language to enforce the expected behaviour.

          If the parameters are final, then when you introduce the ParameterObject,
          they should be final there.

          Also remember that ParameterObjects really should be tossed away after use.
          The calling code in the example should have been:

          int a = 2;
          int b = 4;
          doSomething(new ParameterObject(a, b));
          System.out.println(a, b);


          > My parameter objects tends to develop some behavior and an
          > autonomous live,
          > so now I just start with default value in constructor and
          > accessor for every
          > data changeable.

          If your object evolves, then it can become more powerful. Introduce
          Parameter Object doesn't address this.

          > Moreover creating and destoing an object all the times produces a
          > considerable overload if I have to call these methods many
          > times for second.
          > It's not a problem of performance but of memory fragmentation.

          That is an issue. Introduce Parameter Object can cause problems with this.
          In this case, I would suggest making the parameter object mutable, but don't
          introduce the side effects. In either case, this is beyond the scope of
          Introduce Parameter Object.

          Finally, remember that not all refactorings are applicable all of the time.

          Robert.

          --
          "Duct tape is like the Force: it has a light side, a dark side,
          and it holds the universe together"
          Robert Watkins robertdw@... robertw@...


          [Non-text portions of this message have been removed]
        • kari.hoijarvi@vaisala.com
          So here s my answer. Congratulations for Eric Bennett and Robert Sartin for getting the right answer. Honorable mention to Laurent Bossavit for noticing thread
          Message 4 of 15 , Mar 2, 2001
          • 0 Attachment
            So here's my answer. Congratulations for Eric Bennett and Robert Sartin for
            getting the right answer.

            Honorable mention to Laurent Bossavit for noticing thread safety: ADO and
            ODBC are designed thread-safe, but all the ODBC drivers are not!

            The test to expose bug:

            1. Call set_queries with "select * from table1" and "select * from table2"

            after the call:
            m_q1 == "select * from table1"
            m_q2 == "select * from table2"
            m_r1 == (data from table1)
            m_r2 == (data from table2)

            2. lock table4 by administrator, so that it cannot be queried

            3. Call set_queries with "select * from table3" and "select * from table4"

            after the call the state is inconsistent, since m_r2 = db.Execute(q2)
            throws.
            m_q1 == "select * from table1"
            m_q2 == "select * from table2"
            m_r1 == (data from table3) // here's the inconsistency
            m_r2 == (data from table2)

            Finding this bug with testing is hard. You have to run the method twice and
            make the second query to throw.

            Now when it comes to exception safety, there are 4 guarantees:

            1. transactional: system makes a rollback to original states
            2. basic: objects cannot be used but can be deleted after an exception
            3. nothrow: exception cannot occur
            4. no quarantee: objects cannot even be deleted after an exception

            query_comparer satisfies only the basic quarantee. Sometimes the basic
            quarantee is enough, sometimes it's not. Here are my suggestions for fixing
            it:

            1. transactional quarantee: Turn set_queries into a constructor and make the
            class immutable. If mutability is needed, make the class transactional:

            class mutable_query_comparer {
            private query_comparer m_iqc;

            public set_queries(String q1, String q2) throws DBException
            {
            m_iqc = new query_comparer(q1, q2);
            }

            public show_differences(......
            };

            Now, if the immutable_query_comparer throws, m_iqc does not change.

            One reason I love functional programming, immutable classes and storing
            mutable data in DB is, that transactional quarantee is easy to do.

            2. basic quarantee: improve communication. Make sure, that show_differences
            does not work if the state is inconsistent:

            public set_queries(String q1, String q2) throws DBException
            {
            m_r1 = null;
            m_r2 = null;
            m_q1 = null;
            m_q2 = null;
            m_r1 = db.Execute(q1);
            m_r2 = db.Execute(q2);
            m_q1 = q1;
            m_q2 = q2;
            }

            public show_differences(.....
            {
            // check validity of every member here.

            Obviously if you code this way you better have a valid reason to do it.

            3. nothrow:

            Dijkstra does not like exceptions, since they interfere badly with
            correctness proof. He prefers to program "sufficiently large computer" that
            never fails because of resource exhaustion. Since it's impossible to build
            it, he runs his programs in the "hopefully sufficiently large computer" that
            aborts instead of throwing. This is a very valid approach to many types of
            programs, for example compilers.

            4. no guarantee:

            This is the de facto quality standard in the C++ world. For example take a
            look into CSimpleMap in ATL: It cannot be used if the constructors of Key or
            Val classes can throw. It cannot be used with anything else either, since
            there is a resource leak bug in Remove method. It's excellent teaching
            material if you need to teach inspection techniques for someone.

            With Java, writing no quarantee classes requires creative uses of finalize.

            I have held 4 public classes during last year and nobody got this right.
            Exception safety is hard and with C++ it's exceptionally hard.

            Kari

            > Here is an example of mutable member variables, written in pseudo
            Java and
            > using pseudo ADO for db access.
            >
            > What's wrong with it? Can you spot the bug?
            >
            > class query_comparer {
            > private String m_q1, m_q2:
            > private Recordset m_r1, m_r2;
            >
            > public set_queries(String q1, String q2) throws DBException
            > {
            > m_r1 = db.Execute(q1);
            > m_r2 = db.Execute(q2);
            > m_q1 = q1;
            > m_q2 = q2;
            > }
            >
            > public show_differences(......
            > };
          • Laurent Bossavit
            ... Nice ! The test doesn t sound hard to write once you know how; it does appear that it might be hard to figure out that s what you need. But please read on.
            Message 5 of 15 , Mar 2, 2001
            • 0 Attachment
              > The test to expose bug:
              >
              > 1. Call set_queries with "select * from table1" and "select * from table2"
              > 2. lock table4 by administrator, so that it cannot be queried
              > 3. Call set_queries with "select * from table3" and "select * from table4"

              Nice ! The test doesn't sound hard to write once you know how; it
              does appear that it might be hard to figure out that's what you
              need. But please read on.

              > Finding this bug with testing is hard. You have to run the method twice and
              > make the second query to throw.

              "Aha" moment : we don't mean the same thing by "hard to test".

              In understood you to mean "hard to reproduce in test code the
              behaviour that leads to the bug, so a UT can reliably expose it".
              The problem of testing concurrency issues is my canonical
              example of that.

              I now understand that you meant "hard to ensure that test-first
              alone, as opposed to an awareness of reliability issues and the
              dangers of ignoring them, will result in reliable code".

              I had a second "Aha" moment looking at your code; I realized I
              might have seen this problem before and used essentially the
              same tactics. Tell me if this looks familiar. Excerpts below, before
              and after :

              * Before

              public class DispatcherServlet extends HttpServlet implements SingleThreadModel {

              // Why is this failing even with SingleThreadModel !?

              String remoteUser = "";
              String remotePass = "";

              HttpServletRequest request;
              HttpServletResponse response;

              public void service(HttpServletRequest request, HttpServletResponse response)
              throws ServletException, java.io.IOException
              {
              this.request = request;
              this.response = response;
              // ...
              }

              private void getCredentials() {
              try {
              remoteUser = getUserFromHeader();
              remotePass = getPasswordFromHeader();
              }
              catch (Exception e) {}
              }

              * After a first fix : use the "basic guarantee" tactic :

              private void getCredentials() {
              remoteUser = "";
              remotePass = "";
              try {
              // ...etc...

              * After a second fix : Introduce Parameter Object

              public class DispatcherServlet extends HttpServlet {

              private class RequestData {
              public String remoteUser = "";
              public String remotePass = "";

              public HttpServletRequest request;
              public HttpServletResponse response;
              }

              public void service(HttpServletRequest request, HttpServletResponse response)
              throws ServletException, IOException
              {
              RequestData myData = new RequestData();
              // ...
              }

              private void getCredentials(RequestData myData) {
              // ...as before...



              ========================================
              Bless n'a pas de modele : son ambition
              est d'en devenir un.
              ========================================
              Laurent Bossavit - Software Architect
              >>> laurent.bossavit@... <<<
              >>> 06 68 15 11 44 <<<
              >> ICQ#39281367 <<
              Agence Bless http://www.agencebless.com/
              ========================================
            • kari.hoijarvi@vaisala.com
              ... From: Laurent Bossavit [mailto:laurent.bossavit@agencebless.com] ... I have had code like this too. I consider this very hard to test. ... Yes. Exception
              Message 6 of 15 , Mar 2, 2001
              • 0 Attachment
                -----Original Message-----
                From: Laurent Bossavit [mailto:laurent.bossavit@...]

                > "Aha" moment : we don't mean the same thing by "hard to test".

                > In understood you to mean "hard to reproduce in test code the
                > behaviour that leads to the bug, so a UT can reliably expose it".
                > The problem of testing concurrency issues is my canonical
                > example of that.

                I have had code like this too. I consider this "very hard to test."

                > I now understand that you meant "hard to ensure that test-first
                > alone, as opposed to an awareness of reliability issues and the
                > dangers of ignoring them, will result in reliable code".

                Yes. Exception safety is hard to ensure with tests. Not impossible, but
                hard. I had a fellow who worked with a big application exception safety for
                a month. His strategy was to allow one more memory allocation each time he
                started the smoke test. He didn't get very far during that month.

                On the other hand, proper reviews with statistical testing gives a good idea
                about exception safety. I have our exception raising and HRESULT checking
                calling DebugBreak every time an error occurs. At that point, I go to the
                debugger and walk thru the live exception handling, line by line, focusing
                in code correctness. After this inspection, I remove the DebugBreak call.

                So far 44 calls, 1.2% overall have been inspected without a found defect.
                Simple statistics gives 89% confidentiality level that 95% of the exception
                handling is correct.

                Statistical testing of this kind is easy. Writing automated tests for all
                those 3500 possible exceptions would be an expensive way to find bugs.

                > I had a second "Aha" moment looking at your code; I realized I
                > might have seen this problem before and used essentially the
                > same tactics. Tell me if this looks familiar. Excerpts below, before
                > and after :

                Your code is pretty much a replica of my example. Thanks for giving a real
                world examnple for me! I'll save this discussion for my future lectures.

                This is a typical inconsistent state bug. Inspection is the cheapest tool to
                find these. I think couple of my students had this same feeling of code
                smell: they protested against the code saying it was stupid, but could not
                tell exactly why.


                Kari
              • Laurent Bossavit
                ... Rings true. So we might conclude the following. Some classes of problems are hard to test in the sense that test- first will not completely ensure
                Message 7 of 15 , Mar 2, 2001
                • 0 Attachment
                  > This is a typical inconsistent state bug. Inspection is the
                  > cheapest tool to find these.

                  Rings true. So we might conclude the following.

                  Some classes of problems are "hard to test" in the sense that test-
                  first will not completely ensure reliability.

                  Some classes of problems are "hard to test" in the sense of it
                  being hard to automate the testing.

                  It seems to us that the answer to the first is to perform inspection -
                  in an XP team, pair programming has this role, which can be seen
                  as supplementing the reliability benefits of test-first.

                  It seems to us that the answer to the last is to work hard on the
                  tools that will make automation possible, and eventually less hard -
                  because outside of issues requiring human judgement tests that
                  can be done manually can be automated.


                  ========================================
                  We aim to make simple things simple and
                  complex things possible.
                  ========================================
                  Laurent Bossavit - Software Architect
                  >>> laurent.bossavit@... <<<
                  >>> 06 68 15 11 44 <<<
                  >> ICQ#39281367 <<
                  Agence Bless http://www.agencebless.com/
                  ========================================
                • Robert Sartin
                  ... The challenge with this sort of problem isn t so much that is hard to test as that if you think of the test, it s because you don t have the bug . These
                  Message 8 of 15 , Mar 2, 2001
                  • 0 Attachment
                    --- Laurent Bossavit <laurent.bossavit@...> wrote:
                    > > This is a typical inconsistent state bug. Inspection is the
                    > > cheapest tool to find these.
                    >
                    > Rings true. So we might conclude the following.
                    >
                    > Some classes of problems are "hard to test" in the sense that test-
                    > first will not completely ensure reliability.

                    The challenge with this sort of problem isn't so much that is "hard to
                    test" as that "if you think of the test, it's because you don't have
                    the bug". These are bugs that occur in the blind spot.

                    Regards,

                    Rob


                    __________________________________________________
                    Do You Yahoo!?
                    Get email at your own domain with Yahoo! Mail.
                    http://personal.mail.yahoo.com/
                  Your message has been successfully submitted and would be delivered to recipients shortly.