Chatp GPT, help me refactor my class

[ennowulff]

During a workshop, I asked the contestants to refactor the sources of the Ski Lift Price Kata by dpanzer. We made three rounds with about 45 minutes per round and 15 skilled and experienced developers tried to identify, what the code does, write sensible unit tests and refactor the code to make it better to read and maintain.

This morning I chatted with ChatGPT to see, how it can help me to solve this task. I asked questions, gave instructions and tried to use the output in Eclipse.

This is my journey.

Note: I chatted with ChatGPT in german and translated the conversation to english using DeepL, because I am too lazy to do the same thing in english again. I did not do everything in the order that I describe it, but the generated code and the answers are a real output of ChatGPT. Also I didn't restart the process when adding the results to eclipse in order to fix errors. I fixed them myself.

Ski Lift Pricing Kata

The Ski Lift Pricing Kata originally is a refactoring kata by Johan Martinsson for different programming languages. The code is awful, buggy and not very good to read.

The code calculates the price for a ski lift pass depending on some parameters.

The task is, to refactor the code using unit tests. To solve this task you will have to identify what the code does in the first place. After that you can decide which unit tests are sensible for this class. If you have written enough unit tests to be sure to cover every combination, you can refactor the code in order to make it better to read and better to maintain.

Writing unit tests

First I asked ChatGPT to show a list of senseful combinations to test. This is the answer:

• 
  

  Time of day "Day":
  

  
  
  • Age < 6: Expected result: 0
  
  
  • Age < 15: Expected result: Base price with 30% discount
  
  
  • Age > 64: Expected result: Base price with 25% discount
  
  
  • No age specified: Expected result: Base price
  
  

  
  

  
  


• 
  

  Time of Day "Night":
  

  
  
  • Age >= 6: Expected result: Base price with 40% discount
  
  
  • Age < 6: Expected result: 0
  
  

  
  

  
  

It may be useful to add additional age groups (e.g., 15-64) and different base prices for different lift types to expand the tests and check the accuracy of the results.

That answer helps a lot to know which unit tests I should define. But because I am lazy, I asked ChatGPT to code the unit tests.

It firstly only showed me the code for unit test without any method names or class definition like this:

* Test case for normal lift pass for adult

  DATA(age) = 30.

  DATA(type) = 'normal'.

  DATA(expected_cost) = 100.

  cl_zprices_1=>calculate_lift_pass_price( EXPORTING age = age

                                           type = type

                                           IMPORTING cost = DATA(actual_cost) ).

  cl_abap_unit_assert=>assert_equals( expected_cost, actual_cost ).

This is not bad, as ChatGPT uses the correct class and method CL_ABAP_UNIT_ASSERT=>ASSERT_EQUALS. It also uses inline declaration for defining the variables. It is not necessary to explicitly define the variables, but for me the solution is okay. It corresponds to the common GIVEN - WHEN -THEN logic of unit test:

GIVEN - the input data

WHEN - calculating the price

THEN - I expect the following (ASSERT_EQUALS)

The syntax of the ASSERT_EQUALS method is not correct. In ABAP we have to name the parameters we want to use. Therefore I asked ChatGPT to correct it.

It then corrected it but still not ABAP-correct:

ASSERT_EQUALS( expected = 0 actual = cost message = 'Test for age less than 6 failed' ).

ChatGPT forgot to put the class name in the ASSERT_EQUALS call and used wrong parameter names. So I again asked for correction ("The parameter names are incorrect"). And now something strange happened: ChatGPT suddenly printed a complete test class with test class definition, method names and method implementation. It still uses the wrong parameters and forgot the key word RISK LEVEL and the mothd names are too long, but hey, it really helps. There is also an artefact of something I asked before: create a Data Access Object to replace the SELECT command. (But later more about that). ChatGPT mixed up the DAO-class and the main class. But that is easy to correct. oh, and also the addition FOR TESTING was missing at the methods.

ChatGPT didn't show me the complete code. I think that is because of the text limitation. But I just learned that I could have typed CONTINUE to complete the answer.

CLASS lcl_zprices_1 DEFINITION FOR TESTING DURATION SHORT.

  PRIVATE SECTION.

    DATA: lv_dao TYPE REF TO zprices_1_dao.



    METHODS setup.

    METHODS teardown.

    METHODS test_calculate_lift_pass_price_day_under_6.

    METHODS test_calculate_lift_pass_price_day_6_to_15.

    METHODS test_calculate_lift_pass_price_day_over_15.

    METHODS test_calculate_lift_pass_price_night_under_6.

    METHODS test_calculate_lift_pass_price_night_6_to_64.

    METHODS test_calculate_lift_pass_price_night_over_64.

ENDCLASS.



CLASS lcl_zprices_1 IMPLEMENTATION.



  METHOD setup.

    CREATE OBJECT lv_dao TYPE zprices_1_dao.

    lv_dao->set_base_price( 'day', 100 ).

    lv_dao->set_base_price( 'night', 80 ).

  ENDMETHOD.



  METHOD teardown.

  ENDMETHOD.



  METHOD test_calculate_lift_pass_price_day_under_6.

    DATA(lv_cost) = lv_dao->calculate_lift_pass_price( age = 5, type = 'day' ).

    cl_abap_unit_assert=>assert_equals( expected = 0, actual = lv_cost msg = 'Price for day ticket under 6 years should be 0' ).

  ENDMETHOD.



  METHOD test_calculate_lift_pass_price_day_6_to_15.

    DATA(lv_cost) = lv_dao->calculate_lift_pass_price( age = 10, type = 'day' ).

    cl_abap_unit_assert=>assert_equals( expected = 70, actual = lv_cost msg = 'Price for day ticket between 6 and 15 years should be 30% off' ).

  ENDMETHOD.



  METHOD test_calculate_lift_pass_price_day_over_15.

    DATA(lv_cost) = lv_dao->calculate_lift_pass_price( age = 20, type = 'day' ).

    cl_abap_unit_assert=>assert_equals( expected = 100, actual = lv_cost msg = 'Price for day ticket over 15 years should be full price' ).



    DATA(lv_cost) = lv_dao->calculate_lift_pass_price( age = 65, type = 'day' ).

    cl_abap_unit_assert=>assert_equals( expected = 75, actual = lv_cost msg = 'Price for day ticket over 64 years should be 25% off' ).

  ENDMETHOD.

...

I copied this code to eclipse and did some corrections to make it work.

These are the things I corrected:

• added RISK LEVEL HARMLESS to class definition


• Changed the method names
  
  
  
  • from TEST_CALCULATE_LIFT_PASS_PRICE_*
  
  
  • to CALCULATE_PRICE_*
  
  
  
  


• Added FOR TESTING to test methods


• changed LV_DAO stuff to CUT with the name of my current class.


• added the test case Calculate_Price_Day_Over_64 which was put into the method Calculate_Price_Day_over_15


• added the test cases which were defined but implementation missing

oh... Wait!!!

I just realize that I requested something from ChatGPT that it couldn't deliver: The base price of the lift pass is hidden in the database. Therefore it really makes sense that ChatGPT uses a DAO attribute to set the assumed base prices for day and night:

CREATE OBJECT lv_dao TYPE zprices_1_dao.

lv_dao->set_base_price( 'day', 100 ).

lv_dao->set_base_price( 'night', 80 ).

Neve the less, the data access object was not used correctly. Also I didn't explain that I started with asking ChatGPT to refactor the code and extract the database selection into a data access object. So I will just add, what ChatGPT and I did there before.

Data access object

A DAO - Data Access Object is a class that solely cares about providing and saving data. It is a common technique to match the design principle Separaction of Concerns (SoC) and to use fake data for unit tests (Dependency Injection).

The process is:

• define an interface for the DAO that has methods to set and get data


• create a class that implements the DAO interface and uses a select to access the desired data


• Create a (local) class that implements the DAO interface and uses fake data for testing


• Use the DAO interface as attribute of the main class


• Add a CONSTRUCTOR that has the DAO interface as importing parameter


• in the CONSTRUCTOR the DAO attribute must be created with the DAO-class that does the normal data selection


• in unit tests the main class can be instantiated by providing a DAO class that uses fake data

With this technique you can get rid of the dependency "data selection".

I asked ChatGPT to do this and it did in three attempts that all were not consistent. But at least it did the following:

1. add a class attribute to the main class:
   DATA dobj TYPE REF TO if_price_dao.


2. delivered the code for the DAO:

INTERFACE if_price_dao.

  METHODS: get_base_price IMPORTING type TYPE char10

                              RETURNING value(base_price) TYPE int4.

ENDINTERFACE.



CLASS lcl_price_dao_impl DEFINITION IMPLEMENTING if_price_dao.

  METHOD get_base_price.

    SELECT SINGLE cost INTO base_price FROM zbase_prices WHERE type = type.

  ENDMETHOD.

ENDCLASS.

So ChatGPT used the DAO concept in the generated test class. It did not use it correct, because the created DAO reference should be passed to the CONSTRUCTOR of the main class. ChatGPT uses the (yet not generated method) SET_BASE_PRICE to define the assumption of the calculation.

It is not consistent at all, but I know what I wanted to achieve and ChatGPT did a lot so that I could use it.

At next I will try to add the DAO interface in the Class-relevant local types section of my class and implement the code for selecting data. There were some adaptions I had to do:

• correct the syntax


• add method SET_BASE_PRICE


• add class definition and implementation for DAO-test

INTERFACE if_price_dao.

  METHODS get_base_price

    IMPORTING

      type              TYPE char10

    RETURNING

      VALUE(base_price) TYPE int4.

  METHODS set_base_price

    IMPORTING

      type       TYPE char10

      base_price TYPE int4.

ENDINTERFACE.



CLASS lcl_price_dao_impl DEFINITION.

  PUBLIC SECTION.

    INTERFACES zif_price_dao.

ENDCLASS.



CLASS lcl_price_dao_test DEFINITION.

  PUBLIC SECTION.

    INTERFACES zif_price_dao.

  PRIVATE SECTION.

    TYPES: BEGIN OF _rate,

             type  TYPE char10,

             price TYPE i,

           END OF _rate,

           _rates TYPE SORTED TABLE OF _rate WITH UNIQUE KEY type.

    DATA rates TYPE _rates.

ENDCLASS.

The implementation must be placed in the Local types section of the class:

CLASS lcl_price_dao_impl IMPLEMENTATION.

  METHOD if_price_dao~get_base_price.

    SELECT SINGLE cost INTO base_price FROM zbase_prices WHERE type = type.

  ENDMETHOD.



  METHOD if_price_dao~set_base_price.

    "not needed here

  ENDMETHOD.

ENDCLASS.



CLASS lcl_price_dao_test IMPLEMENTATION.

  METHOD zif_price_dao~get_base_price.

    base_price = VALUE #( rates[ type = type ]-price DEFAULT 0 ).

  ENDMETHOD.



  METHOD zif_price_dao~set_base_price.

    ASSIGN rates[ type = type ]-price TO FIELD-SYMBOL(<price>).

    IF sy-subrc = 0.

      <price> = base_price.

    ELSE.

      INSERT VALUE #( type = type price = base_price ) INTO TABLE rates.

    ENDIF.

  ENDMETHOD.

ENDCLASS.

Normally I wouldn't use a SET method to define the test values, but I like the idea and will try it.

For the test DAO I had to do some more implementation to hold the rates for day and night.

At least I need a possibility to set the DAO for the main class. Therefore I add the DAO attribute and create a constructor:

  PUBLIC SECTION.

    METHODS constructor

      IMPORTING

        dao TYPE REF TO if_price_dao OPTIONAL.

  PRIVATE SECTION.

    DATA dao TYPE REF TO if_price_dao.

  METHOD constructor.

    IF dao IS SUPPLIED.

      me->dao = dao.

    ELSE.

      me->dao = NEW lcl_price_dao_impl( ).

    ENDIF.

  ENDMETHOD.

Unfortunatly I get the following error:

Type "IF_PRICE_DAO" is unknown

I remember that there was some issue and that there was a solution, but I don't remember anymore. To overcome this error, I created the DAO interface globally: ZIF_PRICE_DAO

Back to testing

Now that I have implemented all the DAO stuff, I added the call to the SETUP method of my testclass:

  METHOD setup.

    DATA(dao) = CAST zif_price_dao( NEW lcl_price_dao_test( ) ).

    dao->set_base_price( type = 'day' base_price = 100 ).

    dao->set_base_price( type = 'night' base_price = 80 ).

    cut = NEW #( dao ).

  ENDMETHOD.

I also had to replace the direct select by using the Data Access Object:

   DATA(ld_baseprice) = dao->get_base_price( type ).

Start the engines

Okay. It's time to run the (partly) generated unit tests (CTRL-SHIFT-F10).

This is the result:

I will not try to find out why this one test case is wrong. Instead I assume that ChatGPT did not find out the correct values and just adapt the test case.

Now there are still two test cases missing which I will fill out manually and use the values that I receive:

  METHOD calculate_price_night_over_64.

    DATA(lv_cost) = cut->calculate_lift_pass_price( age = 65 type = 'night' ).

    cl_abap_unit_assert=>assert_equals( exp = 32 act = lv_cost msg = 'Price for night ticket should be reduced' ).

  ENDMETHOD.



  METHOD calculate_price_night_under_6.

    DATA(lv_cost) = cut->calculate_lift_pass_price( age = 4 type = 'night' ).

    cl_abap_unit_assert=>assert_equals( exp = 0 act = lv_cost msg = 'Price for child night ticket should be free' ).

  ENDMETHOD.

Refactoring

Now we come to the main task: Refactoring the code. I asked ChatGPT to

now change the code so that the logic is easier to understand and read

The first attempt had the result that only the useless CONV statements were dismissed. The second try was also not very good, so I said:

the logic is still hard to read. try again.

And this is the output:

CLASS zprices_1 DEFINITION

  PUBLIC

  FINAL

  CREATE PUBLIC .

  PUBLIC SECTION.



    METHODS calculate_lift_pass_price IMPORTING

      i_type TYPE char10

      i_age  TYPE int4 OPTIONAL

    RETURNING VALUE(r_cost) TYPE int4.

  PRIVATE SECTION.

    CLASS-METHODS calculate_discount

      IMPORTING

        i_age  TYPE int4

      RETURNING VALUE(r_discount) TYPE f.

    CLASS-DATA: 

      c_discount_night TYPE f VALUE '0.4',

      c_discount_kids TYPE f VALUE '0.3',

      c_discount_senior TYPE f VALUE '0.25'.

    INTERFACES if_pricing_dao.

    DATA:

      m_dao TYPE REF TO if_pricing_dao.

ENDCLASS.



CLASS zprices_1 IMPLEMENTATION.



  METHOD calculate_lift_pass_price.

    DATA:

      l_base_price TYPE int4.

    m_dao->get_base_price( EXPORTING i_type = i_type IMPORTING e_base_price = l_base_price ).

    IF l_base_price IS INITIAL.

      r_cost = 0.

    ELSE.

      r_cost = CEIL( l_base_price * ( 1 - calculate_discount( i_age ) ) ).

    ENDIF.

  ENDMETHOD.



  CLASS-METHOD calculate_discount.

    IF i_age < 6.

      r_discount = 0.

    ELSEIF i_age < 15.

      r_discount = c_discount_kids.

    ELSEIF i_age > 64.

      r_discount = c_discount_senior.

    ELSEIF i_type = 'night'.

      r_discount = c_discount_night.

    ELSE.

      r_discount = 0.

    ENDIF.

  ENDMETHOD.

ENDCLASS.



INTERFACE if_pricing_dao.

  METHODS get_base_price

    IMPORTING

      i_type TYPE char10

    EXPORTING

      e_base_price TYPE int4.

ENDINTERFACE.



CLASS zbase_prices_dao DEFINITION IMPLEMENTING if_pricing_dao.

  METHOD if_pricing_dao~get_base_price.

    SELECT SINGLE cost INTO e_base_price

    FROM zbase_prices

    WHERE type = i_type.

  ENDMETHOD.

ENDCLASS.

I had to do some small corrections:

• the parameter names started now with i_


• the DAO method GET_BASE_PRICE had an importing parameter instead of a returning parameter


• the TYPE was not available in method CALCULATE_DISCOUNT.
  
  
  
  • I had the following two options:
    
    
    
    • add TYPE as input parameter
    
    
    • set TYPE as global attribute
    
    
    
    
  
  
  • For whatever reason I decided to use a global attribute.
  
  
  
  

Now I had the following refactored code:

CLASS zprices_1_chatgpt DEFINITION

  PUBLIC

  FINAL

  CREATE PUBLIC .



  PUBLIC SECTION.



    METHODS constructor

      IMPORTING

        dao TYPE REF TO zif_price_dao OPTIONAL.



    METHODS calculate_lift_pass_price IMPORTING

                                                type          TYPE char10

                                                age           TYPE int4 OPTIONAL

                                      RETURNING VALUE(r_cost) TYPE int4.



  PRIVATE SECTION.

    METHODS calculate_discount

      IMPORTING

                age               TYPE int4

      RETURNING VALUE(r_discount) TYPE f.

    DATA:

      c_discount_night  TYPE f VALUE '0.4',

      c_discount_kids   TYPE f VALUE '0.3',

      c_discount_senior TYPE f VALUE '0.25'.

    DATA:

      type  TYPE char10,

      m_dao TYPE REF TO zif_price_dao.



ENDCLASS.







CLASS zprices_1_chatgpt IMPLEMENTATION.



  METHOD constructor.

    IF dao IS SUPPLIED.

      m_dao = dao.

    ELSE.

      m_dao = NEW lcl_price_dao_impl( ).

    ENDIF.

  ENDMETHOD.



  METHOD calculate_lift_pass_price.

    DATA:

      l_base_price TYPE int4.

    l_base_price = m_dao->get_base_price( EXPORTING type = type ).



    IF l_base_price IS INITIAL.

      r_cost = 0.

    ELSE.

      r_cost = ceil( l_base_price * ( 1 - calculate_discount( age ) ) ).

    ENDIF.



    me->type = type.

  ENDMETHOD.



  METHOD calculate_discount.

    IF age < 6.

      r_discount = 0.

    ELSEIF age < 15.

      r_discount = c_discount_kids.

    ELSEIF age > 64.

      r_discount = c_discount_senior.

    ELSEIF type = 'night'.

      r_discount = c_discount_night.

    ELSE.

      r_discount = 0.

    ENDIF.



  ENDMETHOD.

ENDCLASS.

I also asked ChatGPT, what it did. It answered:

I have tried to improve the logic in the code by restructuring the if-then-else queries and using variables with meaningful names. This includes removing unnecessary branches and using loops to simplify repeated calculations. I have also tried to remove unnecessary data type conversions and improve the use of constants instead of hard coding. In addition, I tried to modularize the logic by creating a "DAO" (Data Access Object) class and using dependency injection to improve the maintainability of the code. I also tried to create the interface for it and remove unused variables.

Do you want to know the result?? 😉 Here it is

We - ChatGPT and I - have two errors. Not two bad for the beginning, but also room for improvement.

This is the code coverage. Also not perfect, but okay.

Conclusion

I do not want to dig any deeper now, because AI and I produced enough code and text for today. When I tried ChatGPT for refactoring, I was stunned about the results. After having checked them I am still impressed, but I also see that there is a lot of manual work. Never the less, AI can help to get ideas or a strating point even for programming tasks.

This is what I found was strange:

• even though ChatGPT reacts on former input and referrs to this, it produces inconsistent results like the parameter changes from TYPE to I_TYPE and AGE to I_AGE.


• It seems to know what is code and what is text, but sometimes...
  


• At the end I asked ChatGPT to comment the original code and it stated that it can't remember the original code so I had to post it again. 😅

In my opinion it is a really great achievement what AI is able to do.

I am very curios about the next evolution steps of ChatGPT.

So far we can see AI as a tool and not (yet) as a menace.

 

Have a nice weekend!

~Enno