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

[XP] Re: 100 line function

Expand Messages
  • Mats Henricson
    ... The function I have in mind *can* be split into functions, but almost all of them would have many in and out parameters, so they would be quite contrived.
    Message 1 of 14 , Mar 29, 2000
    • 0 Attachment
      > > >Some functions are more than 100 lines long, and
      > > >I can't see any way to refactor it to split it up into several
      > > >functions.
      >
      > Try using a Method Object (http://www.c2.com/cgi/wiki?MethodObject).
      > There is no function that will not yield to this technique.

      The function I have in mind *can* be split into functions,
      but almost all of them would have many in and out parameters,
      so they would be quite contrived.

      > Also take a peek at Martin Fowlers book on refactoring.

      Well, thanks for the tip, but I've already read it and
      contributed two patterns of my own to www.refactoring.com.

      So, just to expose my poor programming skills, here is the
      function I had in mind when I wrote about a 100 line function.
      I'm sorry, I will not spend all of this day explaining the
      context for you. We're not an XP project, yet, and we're in
      QA bug fixing mode, and it is a bit hectic.

      And, make sure you don't read the describing comments! ;-)

      Mats
      ------------------
      /**
      * Returns a page of principals of an organization. PLEASE NOTE that
      this function
      * will only work if principals are returned in this order:
      * First organizations, then groups, then aliases and last users
      * The implementation below is rather complex, but my guess is that
      the way I have
      * done it is reasonable.
      *
      * This function works together with its twin function
      getPrincipalsWithPages().
      * This is the usual way they work together:
      *
      * 1. getPrincipals() gets called, and if the parameter is a simple
      UserID it calls
      * getPrincipalsWithPages().
      * 2. This function either returns all principals of this
      organization, or finds
      * out that there are far too many principals of this
      organization to return
      * it back in one vector.
      * 3. If there are too many principals, getPrincipalsWithPages()
      creates a vector
      * of all of them in the following order: First all organizations
      sorted by name,
      * then all groups sorted by name, then all aliases sorted by
      name and finally
      * all users sorted by name.
      * 4. Then it divides this vector into chunks (pages) of
      approximately SIZE_LIMIT
      * size, and returns a new vector of OrganizationPage objects
      that represents
      * these pages. These OrganizationPage objects store the first
      and last UserID
      * of each page. It is clear that the first UserID of the first
      page probably is
      * an organization, since the organizations are first in the
      vector, but it is not
      * as obvious what the last UserID is. If there was more than
      SIZE_LIMIT number
      * of organization members, then also last will be an
      organization UserID. If
      * there was less than SIZE_LIMIT organization members, then the
      last UserID of
      * the first page can be a group, alias or user, depending on how
      many members
      * of these types this organization has.
      * 5. The client gets this vector of OrganizationPage objects,
      realizes that to get
      * all principals of this organization, it must call
      getPrincipals() several
      * times with the OrganizationPageID objects (stored inside the
      OrganizationPage
      * objects) as parameters.
      * 6. If getPrincipals() is called with an OrganizationPageID object
      as parameter,
      * then the below function will be called.
      * 7. getPrincipalPage() will ask its parameter for the first and
      last UserID, and
      * the UserID of the parent organization.
      * 8. This data is enough for it to start loading principals from
      the database, if
      * first and last really represents pages in accordance to the
      scheme described
      * above.
      * 9. If first is an organization with name "Accounting" and last is
      an alias with
      * name "Best tester in town", then the function below will load
      all organizations
      * with names >= "Accounting". Then it will load all groups. Then
      it will load
      * all aliases with names <= "Best tester in town".
      *10. Then all principals found will be passed back to the client.
      *
      * @param pageID An object holding all data
      necessary to know
      * exactly what page is asked for
      * @return A vector containing the page of
      principals of an
      * organization
      * @throws UsersServiceException Thrown if the state of the pageID
      object was not
      * usable for creating the page, or
      if the page had
      * too many or too few principals,
      or if there was
      * a general database problem.
      * @throws UserSizeLimitException If there were significantly more
      or significantly
      * less than SIZE_LIMIT entries in
      this page, which
      * would indicate that the page is
      very old. This
      * exception class extends
      UsersServiceException.
      * @throws NoResponseException Thrown if there was no response
      from the database
      * @throws SQLException Thrown if there was a problem
      while querying the
      * database
      */

      private CVector getPrincipalPage(ConnectionHolder coho,
      OrganizationPageID pageID)
      throws UsersServiceException, NoResponseException,
      SQLException
      {
      String parent = pageID.getParentID().identifier();
      UserID firstID = pageID.getFirstID();
      UserID lastID = pageID.getLastID();
      CPrincipal first = createPrincipalFromUserID(coho,
      firstID);
      CPrincipal last = createPrincipalFromUserID(coho,
      lastID);
      String firstName =
      DBUtil.quote(first.name().toUpperCase());
      String lastName =
      DBUtil.quote(last.name().toUpperCase());

      boolean fallthrough = false;
      CVector userIDs = new CVector();
      String sql;

      switch (firstID.getType())
      {
      case UserID.ORGANIZATION:
      {
      sql = "SELECT organization_id FROM Organization WHERE
      parent_org_id=" + parent +
      " AND is_available='T'";

      if (lastID.getType() == UserID.ORGANIZATION)
      {
      // This means that both first and last are
      organizations
      // This means we should grab organizations with
      names between first and last
      sql = sql + " AND UPPER(organization_name) BETWEEN "
      + firstName +
      " AND " + lastName;
      fallthrough = false; // No more fallthrough
      }
      else
      {
      // This means that first is an organization while
      last is a group, alias or user.
      // This means that we should grab organizations with
      names >= first
      sql = sql + " AND UPPER(organization_name)>=" +
      firstName;
      fallthrough = true; // We must also check
      next type of principal
      }

      sql = sql + " ORDER BY UPPER(organization_name)";
      userIDs.addElements(getUserIDs(coho, sql,
      UserID.ORGANIZATION));

      if (!fallthrough)
      {
      break;
      }
      }
      case UserID.GROUP:
      {
      sql = "SELECT group_id FROM NCSGroup WHERE
      organization_id=" + parent +
      " AND is_available='T'";

      if (fallthrough)
      {
      // This means that first was an organization
      if (lastID.getType() == UserID.GROUP)
      {
      // This means that first was an organization
      while last is a group.
      // This means that we should grab groups with
      names <= last
      sql = sql + " AND UPPER(group_name)<=" +
      lastName;
      fallthrough = false; // No more fallthrough
      }
      else
      {
      // This means that first was an organization
      while last is an alias or user.
      // This means that we should grab all groups.
      // We don't need to qualify the default SQL
      above
      fallthrough = true; // We must also check
      next type of principal
      }
      }
      else
      {
      // This means that first is a group
      if (lastID.getType() == UserID.GROUP)
      {
      // This means that both first and last are
      groups
      // This means we should grab groups with names
      between first and last
      sql = sql + " AND UPPER(group_name) BETWEEN " +
      firstName +
      " AND " + lastName;
      fallthrough = false; // No more fallthrough
      }
      else
      {
      // This means that first is a group while last
      is an alias or user.
      // This means that we should grab groups with
      names >= first
      sql = sql + " AND UPPER(group_name)>=" +
      firstName;
      fallthrough = true; // We must also check
      next type of principal
      }
      }

      sql = sql + " ORDER BY UPPER(group_name)";
      userIDs.addElements(getUserIDs(coho, sql,
      UserID.GROUP));

      if (!fallthrough)
      {
      break;
      }
      }
      case UserID.ALIAS:
      {
      String list = "SELECT member_alias_id FROM
      OrganizationMember" +
      " WHERE organization_id=" + parent + " AND
      member_alias_id IS NOT NULL";
      sql = "SELECT alias_id FROM Alias WHERE alias_id
      IN (" + list + ")" +
      " AND is_available='T'";

      if (fallthrough)
      {
      // This means that first was an organization or a
      group
      if (lastID.getType() == UserID.ALIAS)
      {
      // This means that first was an organization or
      a group
      // while last is an alias.
      // This means that we should grab aliases with
      names <= last
      sql = sql + " AND UPPER(alias_name)<=" +
      lastName;
      fallthrough = false; // No more fallthrough
      }
      else
      {
      // This means that first was an organization or
      a group
      // while last is a user.
      // This means that we should grab all aliases
      // We don't need to qualify the default SQL
      above
      fallthrough = true; // We must also check
      next type of principal
      }
      }
      else
      {
      // This means that first is an alias
      if (lastID.getType() == UserID.ALIAS)
      {
      // This means that both first and last are
      aliases.
      // This means we should grab aliases with names
      between first and last
      sql = sql + " AND UPPER(alias_name) BETWEEN " +
      firstName +
      " AND " + lastName;
      fallthrough = false; // No more fallthrough
      }
      else
      {
      // This means that first is an alias while last
      is a user
      // This means that we should grab aliases with
      names >= first
      sql = sql + " AND UPPER(alias_name)>=" +
      firstName;
      fallthrough = true; // We must also check
      next type of principal
      }
      }

      sql = sql + " ORDER BY UPPER(alias_name)";
      userIDs.addElements(getUserIDs(coho, sql,
      UserID.ALIAS));

      if (!fallthrough)
      {
      break;
      }
      }
      case UserID.PERSON:
      {
      String list = "SELECT member_user_id FROM
      OrganizationMember" +
      " WHERE organization_id=" + parent + " AND
      member_user_id IS NOT NULL";
      sql = "SELECT user_id FROM NCSUser WHERE user_id
      IN (" + list + ")" +
      " AND is_available='T'";

      if (fallthrough)
      {
      // This means that first was an organization, group
      or alias
      if (lastID.getType() == UserID.PERSON)
      {
      // This means that first was an organization,
      group or alias
      // while last is a user.
      // This means that we should grab users with
      names <= last
      sql = sql + " AND UPPER(user_name)<=" +
      lastName;
      }
      else
      {
      // This means that first was an organization,
      group or alias
      // while last is none of that, and not a user
      --> faulty code !!
      throw new UsersServiceException("Page not
      correctly created: " + pageID);
      }
      }
      else
      {
      // This means that first is a user
      if (lastID.getType() == UserID.PERSON)
      {
      // This means that both first and last are
      users.
      // This means we should grab users with names
      between first and last
      sql = sql + " AND UPPER(user_name) BETWEEN " +
      firstName +
      " AND " + lastName;
      }
      else
      {
      // This means that first was a user,
      // while last is not a user --> faulty code !!
      throw new UsersServiceException("Page not
      correctly created: " + pageID);
      }
      }

      sql = sql + " ORDER BY UPPER(user_name)";
      userIDs.addElements(getUserIDs(coho, sql,
      UserID.PERSON));

      break;
      }
      default:
      {
      throw new UsersServiceException("Unknown principal type:
      " + firstID.getType());
      }
      }

      // Do we have more than 10% too many UserIDs ???
      if (userIDs.size() > UserStoreAdaptor.SIZE_LIMIT +
      UserStoreAdaptor.SIZE_LIMIT / 10)
      {
      throw new UserSizeLimitException("Page boundaries changed",
      null);
      }

      CVector allPrincipals = getPrincipalsImpl(coho, userIDs);

      // Do we have more than 10% too few UserIDs ???
      if (allPrincipals.size() < UserStoreAdaptor.SIZE_LIMIT -
      UserStoreAdaptor.SIZE_LIMIT / 10)
      {
      throw new UserSizeLimitException("Too few hits",
      allPrincipals);
      }

      return allPrincipals;
      } // getPrincipalPage()
    • Phlip
      ... Replace Switch-on-Type with Polymorphic Dispatch..?
      Message 2 of 14 , Mar 29, 2000
      • 0 Attachment
        > switch (firstID.getType())

        Replace Switch-on-Type with Polymorphic Dispatch..?
      • Mats Henricson
        ... I think that is hard because there is fall through between the case clauses. Mats
        Message 3 of 14 , Mar 29, 2000
        • 0 Attachment
          > > switch (firstID.getType())
          >
          > Replace Switch-on-Type with Polymorphic Dispatch..?

          I think that is hard because there is fall through between
          the case clauses.

          Mats
        • Joshua Kerievsky
          ... Thanks for sending the code Mats. I ll be sending a rewritten version to this list in a day or two (maybe less, if I keep procrastinating on my client
          Message 4 of 14 , Mar 29, 2000
          • 0 Attachment
            >So, just to expose my poor programming skills, here is the
            >function I had in mind when I wrote about a 100 line function.
            >I'm sorry, I will not spend all of this day explaining the
            >context for you. We're not an XP project, yet, and we're in
            >QA bug fixing mode, and it is a bit hectic.
            >
            >And, make sure you don't read the describing comments! ;-)

            Thanks for sending the code Mats. I'll be sending a rewritten version to this
            list in a day or two (maybe less, if I keep procrastinating on my client work).
            --jk

            _______________________________
            Industrial Logic, Inc.
            Joshua Kerievsky, founder
            mailto:joshua@...
            http://industriallogic.com
            415-292-6266
            415-292-6267 (fax)
          • Eric Hodges
            I looked over this. First, I d break out each case into its own function, and have that function return a boolean for fallsthrough. This would make the
            Message 5 of 14 , Mar 29, 2000
            • 0 Attachment
              I looked over this. First, I'd break out each case into its own function,
              and have that function return a boolean for fallsthrough. This would make
              the method shorter, and naming the case methods would provide a clue about
              what they do (which I couldn't figure out from the code alone.) Doing that
              makes the method about 50 lines long. Still pretty big, but much more
              readable.

              The switch statement becomes something like this, with better names for the
              case methods:

              switch (firstID.getType()) {
              case UserID.ORGANIZATION:
              if (!doOrganizationStuff(userIDs, ...))
              break;
              case UserID.GROUP:
              if (!doGroupStuff(userIDs, ...))
              break;
              case UserID.ALIAS:
              if (!doAliasStuff(userIDs, ...))
              break;
              case UserID.PERSON:
              doPersonStuff(userIDs, ...);
              break;
              default:
              throw new UsersServiceException("Unknown principal type: " +
              firstID.getType());
              }
            • Joshua Kerievsky
              Mats, After spending 20 minutes looking over your code, my first thought is there is way too much embedded SQL in this code - it is hard to read, hard to
              Message 6 of 14 , Mar 29, 2000
              • 0 Attachment
                Mats,

                After spending 20 minutes looking over your code, my first thought is "there is
                way too much embedded SQL in this code - it is hard to read, hard to maintain
                and I want to get rid of it." Next thought: "parameterized SQL" For example,
                here are the first two queries from case UserID.ORGANIZATION - I've
                parameterized them with ^xyz^ variables.

                // Possible Query Name: AllOrganizations
                SELECT organization_id
                FROM Organization
                WHERE parent_org_id= ^parent^
                AND is_available='T'
                AND UPPER(organization_name) BETWEEN ^firstName^ AND ^lastName^
                ORDER BY UPPER(organization_name)

                // Possible Query Name: OrgsAndOthers
                SELECT organization_id
                FROM Organization
                WHERE parent_org_id= ^parent^
                AND is_available='T'
                AND UPPER(organization_name) >= ^firstName^
                ORDER BY UPPER(organization_name)

                You store these paramaterized SQL string wherever you like - in a database, in
                objects, whatever. The point is, you have the SQL strings which need to be
                filled in with values. You place your variables in a hashtable. This means the
                variable "parent, firstname, lastname, etc" go into a hashtable, keyed by their
                names. Then you write a small routine to substitute ^ variables (in the SQL)
                for the real values, which are in the hashtable. This is a generic routine,
                which can be used throughout your system.

                Once you do this, you can remove tons of code from the Switch statement. You
                still have a lot more to simplify -- esp. the fallthrough logic -- but now you
                have a much smaller function, which will make it easier to do the next
                refactorings. In addition, having done this, you can write better tests to see
                that the proper UserIDs are returned from each query.

                I'll have more tomorrow.

                --jk
              • Mats Henricson
                Yes, this is probably possible (I haven t looked at the details). My only objections are that the do*Stuff() functions would have quite contrieved names. I
                Message 7 of 14 , Mar 29, 2000
                • 0 Attachment
                  Yes, this is probably possible (I haven't looked at the details).
                  My only objections are that the do*Stuff() functions would have
                  quite contrieved names.

                  I think what I have realized over the last few days, and after
                  reading the refactoring book is that it makes sense to extract
                  methods even if they aren't natural in the usual sense; good
                  and simple name of the function, reasonable number of parameters,
                  only one return value, etc. Just getting the method down to a
                  reasonable length can be reason enough.

                  But maybe the extracted functions *do* have good names, and are
                  natural in the sense mentioned. If so, thanks. I'll rewrite it
                  if I get the time.

                  Mats

                  > I looked over this. First, I'd break out each case into its own function,
                  > and have that function return a boolean for fallsthrough. This would make
                  > the method shorter, and naming the case methods would provide a clue about
                  > what they do (which I couldn't figure out from the code alone.) Doing that
                  > makes the method about 50 lines long. Still pretty big, but much more
                  > readable.
                  >
                  > The switch statement becomes something like this, with better names for the
                  > case methods:
                  >
                  > switch (firstID.getType()) {
                  > case UserID.ORGANIZATION:
                  > if (!doOrganizationStuff(userIDs, ...))
                  > break;
                  > case UserID.GROUP:
                  > if (!doGroupStuff(userIDs, ...))
                  > break;
                  > case UserID.ALIAS:
                  > if (!doAliasStuff(userIDs, ...))
                  > break;
                  > case UserID.PERSON:
                  > doPersonStuff(userIDs, ...);
                  > break;
                  > default:
                  > throw new UsersServiceException("Unknown principal type: " +
                  > firstID.getType());
                  > }
                  >
                  > ------------------------------------------------------------------------
                  > To Post a message, send it to: extremeprogramming@...
                  > To Unsubscribe, send a blank message to: extremeprogramming-unsubscribe@...
                  > Ad-free courtesy of objectmentor.com
                  >
                  > ------------------------------------------------------------------------
                  > -- Talk to your group with your own voice!
                  > -- http://www.egroups.com/VoiceChatPage?listName=extremeprogramming&m=1
                • Andy Glew
                  ... Not a problem. Have one of the objects invoke the other s method. Start off by factoring the case that is fallen through to as a class.
                  Message 8 of 14 , Mar 29, 2000
                  • 0 Attachment
                    > > > switch (firstID.getType())
                    > >
                    > > Replace Switch-on-Type with Polymorphic Dispatch..?
                    >
                    > I think that is hard because there is fall through between
                    > the case clauses.

                    Not a problem.

                    Have one of the objects invoke the other's method.

                    Start off by factoring the case that is fallen through to
                    as a class.
                  • Martin Fowler
                    Do you have any tests for this? ... From: extremeprogramming-return-3767-martin_fowler=compuserve.com@returns.egro ups.com
                    Message 9 of 14 , Mar 29, 2000
                    • 0 Attachment
                      Do you have any tests for this?

                      -----Original Message-----
                      From:
                      extremeprogramming-return-3767-martin_fowler=compuserve.com@...
                      ups.com
                      [mailto:extremeprogramming-return-3767-martin_fowler=compuserve.com@retu
                      rns.egroups.com]On Behalf Of Mats Henricson
                      Sent: Wednesday, March 29, 2000 5:21 PM
                      To: extremeprogramming@...
                      Subject: [XP] Re: 100 line function



                      > > >Some functions are more than 100 lines long, and
                      > > >I can't see any way to refactor it to split it up into several
                      > > >functions.
                      >
                      > Try using a Method Object (http://www.c2.com/cgi/wiki?MethodObject).
                      > There is no function that will not yield to this technique.

                      The function I have in mind *can* be split into functions,
                      but almost all of them would have many in and out parameters,
                      so they would be quite contrived.

                      > Also take a peek at Martin Fowlers book on refactoring.

                      Well, thanks for the tip, but I've already read it and
                      contributed two patterns of my own to www.refactoring.com.

                      So, just to expose my poor programming skills, here is the
                      function I had in mind when I wrote about a 100 line function.
                      I'm sorry, I will not spend all of this day explaining the
                      context for you. We're not an XP project, yet, and we're in
                      QA bug fixing mode, and it is a bit hectic.
                    • Mats Henricson
                      No. Mats
                      Message 10 of 14 , Mar 29, 2000
                      • 0 Attachment
                        No.

                        Mats

                        > Do you have any tests for this?
                        >
                        > -----Original Message-----
                        > From:
                        > extremeprogramming-return-3767-martin_fowler=compuserve.com@...
                        > ups.com
                        > [mailto:extremeprogramming-return-3767-martin_fowler=compuserve.com@retu
                        > rns.egroups.com]On Behalf Of Mats Henricson
                        > Sent: Wednesday, March 29, 2000 5:21 PM
                        > To: extremeprogramming@...
                        > Subject: [XP] Re: 100 line function
                        >
                        >
                        > > > >Some functions are more than 100 lines long, and
                        > > > >I can't see any way to refactor it to split it up into several
                        > > > >functions.
                        > >
                        > > Try using a Method Object (http://www.c2.com/cgi/wiki?MethodObject).
                        > > There is no function that will not yield to this technique.
                        >
                        > The function I have in mind *can* be split into functions,
                        > but almost all of them would have many in and out parameters,
                        > so they would be quite contrived.
                        >
                        > > Also take a peek at Martin Fowlers book on refactoring.
                        >
                        > Well, thanks for the tip, but I've already read it and
                        > contributed two patterns of my own to www.refactoring.com.
                        >
                        > So, just to expose my poor programming skills, here is the
                        > function I had in mind when I wrote about a 100 line function.
                        > I'm sorry, I will not spend all of this day explaining the
                        > context for you. We're not an XP project, yet, and we're in
                        > QA bug fixing mode, and it is a bit hectic.
                        >
                        > ------------------------------------------------------------------------
                        > To Post a message, send it to: extremeprogramming@...
                        > To Unsubscribe, send a blank message to: extremeprogramming-unsubscribe@...
                        > Ad-free courtesy of objectmentor.com
                        >
                        > ------------------------------------------------------------------------
                        > -- Check out your group's private Chat room
                        > -- http://www.egroups.com/ChatPage?listName=extremeprogramming&m=1
                      • Phil Goodwin
                        ... That s just what Method Object takes care of. All those temporaries that would have to be shuttled between methods become class members. Check it out. --
                        Message 11 of 14 , Mar 29, 2000
                        • 0 Attachment
                          Mats Henricson wrote:
                          >
                          > > > >Some functions are more than 100 lines long, and
                          > > > >I can't see any way to refactor it to split it up into several
                          > > > >functions.
                          > >
                          > > Try using a Method Object (http://www.c2.com/cgi/wiki?MethodObject).
                          > > There is no function that will not yield to this technique.
                          >
                          > The function I have in mind *can* be split into functions,
                          > but almost all of them would have many in and out parameters,
                          > so they would be quite contrived.

                          That's just what Method Object takes care of. All those temporaries that
                          would have to be shuttled between methods become class members. Check it
                          out.

                          --
                          Phil Goodwin, Java Software, Sun Microsystems, 408-517-6951, or x66951

                          "There's only so much you can do, and you have to do that much, even if
                          you don't know how much it is." --Garrison Keillor
                        • Eric Hodges
                          ... From: Mats Henricson To: Sent: Wednesday, March 29, 2000 7:01 PM Subject: [XP] Re: 100 line
                          Message 12 of 14 , Mar 29, 2000
                          • 0 Attachment
                            ----- Original Message -----
                            From: "Mats Henricson" <mats@...>
                            To: <extremeprogramming@egroups.com>
                            Sent: Wednesday, March 29, 2000 7:01 PM
                            Subject: [XP] Re: 100 line function


                            > Yes, this is probably possible (I haven't looked at the details).
                            > My only objections are that the do*Stuff() functions would have
                            > quite contrieved names.
                            >
                            > I think what I have realized over the last few days, and after
                            > reading the refactoring book is that it makes sense to extract
                            > methods even if they aren't natural in the usual sense; good
                            > and simple name of the function, reasonable number of parameters,
                            > only one return value, etc. Just getting the method down to a
                            > reasonable length can be reason enough.
                            >
                            > But maybe the extracted functions *do* have good names, and are
                            > natural in the sense mentioned. If so, thanks. I'll rewrite it
                            > if I get the time.

                            Cool. My hunch is that the method names will be significant. Anytime I do
                            this they always are. And I often find that I can apply some pattern like
                            Visitor once I see those names. The extra abstraction seems to help. And
                            if they are contrived names, don't sweat it. They are small, easy to
                            understand methods. And once you've read the first one you can see that the
                            others are similar. Someone should thank you later for that.
                          • Ron Jeffries
                            ... Yes, they almost always do, because chunks of code that occur together are usually accomplishing some specific thing. So you can usually find a descriptive
                            Message 13 of 14 , Mar 30, 2000
                            • 0 Attachment
                              At 05:01 PM 3/29/2000 -0800, you wrote:
                              >I think what I have realized over the last few days, and after
                              >reading the refactoring book is that it makes sense to extract
                              >methods even if they aren't natural in the usual sense; good
                              >and simple name of the function, reasonable number of parameters,
                              >only one return value, etc. Just getting the method down to a
                              >reasonable length can be reason enough.
                              >
                              >But maybe the extracted functions *do* have good names, and are
                              >natural in the sense mentioned. If so, thanks. I'll rewrite it
                              >if I get the time.

                              Yes, they almost always do, because chunks of code that occur together are
                              usually accomplishing some specific thing. So you can usually find a
                              descriptive name, which actually helps make the code readable as well as
                              short. And often leads to the NEXT improvement.



                              Ron Jeffries
                              www.XProgramming.com
                            • Joshua Kerievsky
                              ... Extract Method is important, but when I study Mats code, the first smell I encounter is the violation of Once And Only Once. That is, there is a ton of
                              Message 14 of 14 , Mar 31, 2000
                              • 0 Attachment
                                Mats writes:
                                >>But maybe the extracted functions *do* have good names, and are
                                >>natural in the sense mentioned. If so, thanks. I'll rewrite it
                                >>if I get the time.

                                Martin writes:
                                >And if you don't extract the right way first time, you can always inline and
                                >re-extract later.

                                Extract Method is important, but when I study Mats code, the first smell I
                                encounter is the violation of Once And Only Once. That is, there is a ton of
                                code which is simply devoted to building up the right SQL queries. This is
                                duplication and my first refactoring will be to remove this duplication. But I
                                wonder if others saw all that SQL code as duplication? It is not the same code
                                repeated - but it is the same *activity* repeated in a way that clutters and
                                bloats the code. I think this is an important distinction that we sometimes
                                overlook. In this case, removing the duplication will greatly simplify the
                                code, making it easier to continue to refactor.

                                regards
                                jk
                                _______________________________
                                Industrial Logic, Inc.
                                Joshua Kerievsky, founder
                                mailto:joshua@...
                                http://industriallogic.com
                                415-292-6266
                                415-292-6267 (fax)
                              Your message has been successfully submitted and would be delivered to recipients shortly.