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

Re: "Hard to test" (Was RE: [XP] [refactoring] More on: Introduce Parameter Object )

Expand Messages
  • Ron Jeffries
    ... Outstanding! Way to go! Ronald E Jeffries http://www.XProgramming.com http://www.objectmentor.com
    Message 1 of 15 , Mar 1, 2001
    • 0 Attachment
      At 01:50 PM 3/1/2001 +0100, it seemed like Laurent Bossavit wrote:
      >(N.B. The above code has been compiled and run with the
      >expected results.)

      Outstanding! Way to go!

      Ronald E Jeffries
      http://www.XProgramming.com
      http://www.objectmentor.com
    • Laurent Bossavit
      ... Damn, wrong again. I suppose the reason I guessed wrong is the usual suspect - eagerness to talk - and show off - overriding willingness to listen. (I do
      Message 2 of 15 , Mar 1, 2001
      • 0 Attachment
        > Concurrency is a good guess, but this time it's not what I have in
        > my mind.

        Damn, wrong again.

        I suppose the reason I guessed wrong is the usual suspect -
        eagerness to talk - and show off - overriding willingness to listen. (I
        do appreciate the kudos very much.)

        If I'd paid more attention to your words I probably would have
        focused on "mutable member variables". Let's see - I think I know
        for sure that String objects are all immutable; you can't even have a
        mutable subclass as String is final. So you must be talking about
        RecordSet.

        Is the bug related to the contract of whatever class "db"
        implements ? Specifically, does the "db" instance retain a
        reference to the RecordSet that they return, and maybe alter that
        RecordSet in subsequent calls to Execute ?

        If I'm correct *this* time, my next question would be "Isn't this hard
        to test only because the bug is in the class of db, not in your own
        code ?". This would be a rather restricted meaning of "hard to test",
        but there might be appropriate solutions to this specific kind of
        problem too.


        ========================================
        The best way to predict the future is to
        invent it.
        ========================================
        Laurent Bossavit - Software Architect
        >>> laurent.bossavit@... <<<
        >>> 06 68 15 11 44 <<<
        >> ICQ#39281367 <<
        Agence Bless http://www.agencebless.com/
        ========================================
      • dmitry@yahoo.com
        ... Java and ... 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
        Message 3 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...

          Dmitry
        • 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 4 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 5 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 6 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 7 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 8 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 9 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 10 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 11 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.