Fearless refactoring

Having reliable unit tests not only makes our programs stable, they also give us freedom to look for problems, tweak things here and there, or even make design changes. Why? Well, software development is an iterative process, there’s always something to improve.

In the previous examples, something was wrong with the search() method in the inventory class. I encourage you to look at it and find out what might be wrong with it:

 method search.

    loop at guitars assigning field-symbol(<guitar>).
      if  <guitar>-ref->attributes-builder  = i_guitar_to_search->attributes-builder
      and <guitar>-ref->attributes-model    = i_guitar_to_search->attributes-model
      and <guitar>-ref->attributes-type     = i_guitar_to_search->attributes-type
      and <guitar>-ref->attributes-backwood = i_guitar_to_search->attributes-backwood
      and <guitar>-ref->attributes-topwood  = i_guitar_to_search->attributes-topwood.
		r_found_guitar = <guitar>-ref.
		return.
      endif.
    endloop.

  endmethod.

The method takes as an importing parameter a guitar object, represented by i_guitar_to_search above. What is the issue? If you say out loud what the method actually performs, then it doesn’t make too much sense:

“The customer provides a guitar object and we compare each property with the properties of all guitars in the inventory to find a match”

Do you literally imagine that someone goes to a store to buy a guitar with an existing guitar on their hands? Sure it can happen, but most of the times it does not. You go to a store and tell the salesperson that you’re looking for a certain type of guitar, brand x, within the price range y, you get the idea. In other words, the customer is providing specifications, not real guitar objects. Therefore we need to adjust our design, as it is not reflecting what happens in the real world, and object-oriented programming is about modeling the real world. So next time you find that some piece of code doesn’t make much sense, or there’s a bug somewhere and you can’t find it, try to say it out loud, even better get a rubber duck and talk to it.

I don’t have a rubber duck myself, but the storm trooper is always ready to shoot me when I’m being stupid

So, the solution was to encapsulate those guitar properties into a new object. Encapsulation is good, it allows me to group my application into logical parts, this way I can keep the generic properties of a guitar separate from the actual Guitar object itself.

The second problem I found in the search() method is that it’s not doing just one thing. Methods should do just one thing, they should perform the task you would expect them to do. Nothing more, nothing less. I recommend you to read Robert Martin’s Clean Code book, the examples are written in Java, but there’s so much stuff in there to learn for us ABAP developers, it’s amazing. So this search method is not only searching through the inventory, it is also comparing properties, and this is also wrong. So we need to find a way to encapsulate this comparisson away from search() as well.

Ok, back to the new class, which I’ll call ZCL_GUITAR_SPEC. It has been defined like this:

class zcl_guitar_spec definition
  public
  final
  create public .

  public section.
    types: begin of ty_guitar_attributes,
             builder    type ref to zcl_enum_builder,
             model      type z_guitar_model,
             type       type ref to zcl_enum_guit_type,
             backwood   type ref to zcl_enum_wood,
             topwood    type ref to zcl_enum_wood,
           end of ty_guitar_attributes.

    data: attributes type ty_guitar_attributes read-only.
    methods: constructor importing i_attributes type ty_guitar_attributes,
      matches importing i_otherspec      type ref to zcl_guitar_spec
              returning value(r_matches) type abap_bool.

  protected section.
  private section.
endclass.

the method matches() does what you would expect: compare properties and return true when they match and false otherwise:

method matches.

    if me->attributes-builder <> i_otherspec->attributes-builder.
      r_matches = abap_false.
      return.
    endif.

    if ( i_otherspec->attributes-model is not initial ) and ( me->attributes-model
        <> i_otherspec->attributes-model ).
      r_matches = abap_false.
      return.
    endif.

    if me->attributes-backwood <> i_otherspec->attributes-backwood.
      r_matches = abap_false.
      return.
    endif.

    if me->attributes-topwood <> i_otherspec->attributes-topwood.
      r_matches = abap_false.
      return.
    endif.

    if me->attributes-type <> i_otherspec->attributes-type.
      r_matches = abap_false.
      return.
    endif.

    r_matches = abap_true.
    return.

  endmethod.

Having this, it’s time to update the ZCL_GUITAR, since we abstracted properties into a separate class. The change is small, though, just remove properties like builder, type of wood, type of guitar, and insert the ZCL_GUITAR_SPEC object:

class zcl_guitar definition
  public
  final
  create public .

  public section.
    types: begin of ty_guitar_attributes,
             serialnumber type z_serial_number,
             price        type z_price,
             specs        type ref to zcl_guitar_spec,
           end of ty_guitar_attributes.

    data: attributes type ty_guitar_attributes read-only.
    "! <p class="shorttext synchronized" lang="en"></p>
    "! Creates a guitar object from a guitar structure
    "! @parameter i_guitar_record | <p class="shorttext synchronized" lang="en"> Guitar structure </p>
    methods constructor
      importing
        i_guitar_record type ty_guitar_attributes.

  protected section.
  private section.

endclass.


class zcl_guitar implementation.

  method constructor.

    me->attributes = i_guitar_record.

  endmethod.

endclass.

Now let’s go back to the Inventory class and update the search() method:

First we change the importing parameter, so that it takes a ZCL_GUITAR_SPEC object

 "! <p class="shorttext synchronized" lang="en"></p>
    "! Iterates through the inventory, comparing each property of the i_guitar object with the guitar
    "! inside the inventory. Returns a guitar object only if all of the properties match
    "! @parameter i_guitar_to_search | <p class="shorttext synchronized" lang="en"> Guitar to search</p>
    "! @parameter r_found_guitars | <p class="shorttext synchronized" lang="en">Guitar found in the inventory</p>
    methods search
      importing
        i_guitar_spec          type ref to zcl_guitar_spec
      returning
        value(r_found_guitars) type  guitars_tab.

Then in the implementation part, we change all the weird looking code for something much cleaner like this:

method search.

    loop at guitars assigning field-symbol(<guitar>).
      data(search_specs) = <guitar>-ref->attributes-specs.

      if <guitar>-ref->attributes-specs->matches( search_specs ).

        data(found_guitar) = value ty_guitar( serial_number = <guitar>-ref->attributes-serialnumber
                                              ref           = <guitar>-ref ).
        insert found_guitar into table r_found_guitars.
      endif.

    endloop.

  endmethod.

Isn’t it just nice thing to read? That matches() method is really good looking! You would agree specially if you come from the Java world.

Moment of true, how do we know we didn’t break anything? We made dramatic changes in the original design, created new classes, adapted existing method. In normal situation this is something scary, but not this time, because we have our unit tests, as long as the tests keep passing, we can trust on how the program is behaving. This is pure freedom, it gives the feeling that you can improve anything!

Happy unit testing!

Marco.