# Chatp GPT, help me refactor my class

During a workshop, I asked the contestants to refactor the sources of the Ski Lift Price Kata by Dominik Panzer. 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 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 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

### Assigned Tags

You must be Logged on to comment or reply to a post.

Thanks for sharing! It's an interesting use case. Keeping in mind that this is just a demo program, as a human developer, I'd have these questions on the resulting code:

1. The method is calculate...price but it returns the field named r_cost. Cost and price are different terms. Sure, price of the ticket becomes the buyer's cost but the code is for us, not for the buyer. π Not sure if it's in Clean ABAP but for RETURNING parameters I avoid such dilemmas by using generic R_RETURN or similar because there can be only one anyway.
2. I'm quite confused why the field with "price" in name (line 46) has INT4 type. That's unusual.
3. In calculate_discount, I'd put "age between 6 and 15" and then let "age < 6" drop into ELSE part. But this is not hugely important and having it written as is can have benefits too.

I think it's very cool that AI can help someone to improve the code but maybe "make code easier to read" is not always "make it easier to understand". π

Enno Wulff
Blog Post Author

Jelena Perfiljeva since I know ChatGPT I am too lazy to think by myself. π

This is what ChatGPT answers you:

Yes, I can answer these questions.

1. The method name "calculate_lift_pass_price" and the variable name "r_cost" can be confusing, as you mentioned. It would be better to use a more generic name such as "calculate_ticket_cost" and "r_ticket_cost" to avoid this confusion.

2. The field "l_base_price" is of type INT4, which is unusual for a price field. It would be more appropriate to use a type such as DECIMAL or CURR.

3. In the method "calculate_discount", you can use the age range "age BETWEEN 6 AND 15" and then let "age < 6" drop into the ELSE part as you suggest. This would make the code more readable and maintainable.

Let me add the following: This was not a productive SAP class. It is an exercise for refactoring. Therefore, I think that it is okay to have an integer price.

Plus: ChatGPT can correct a lot of things when speaking to it. Of course you must point the errors. Maybe I will start another try in full dialog and directly point out every little errors that occurs. π

How,long did refactoring take using the AI?

My main concern would be that by using an AI, the refactorer may not gain a good understanding of the code, which could allow errors to creep in.

I've played with it quite a bit. It's very good and chatting and keeping context.It can't be trusted - it will readily admit that, and I'm not convinced of it as a problem solver - which is fine, because it's not designed to do that.

Enno Wulff
Blog Post Author

I am not sure anymore how long the refactoring actually took. But it took a lot more time than I expected, because some things that looked quite ok, were not okay and I had to fix them. I would say about 2-3 hours including writing the results to the blog post.

My main concern would be that by using an AI, the refactorer may not gain a good understanding of the code, which could allow errors to creep in.

That's totally right. If you do not understand what AI does to your code, it's just a more comfortable copy and paste from stackoverflow solutions.

You are also right if you say: "it can't be trsusted". Never the less, it can be a good help if you are stuck or if you are missing ideas in how to continue.

Hi Enno Wulff, great blog!

I had similar experiences with ChatGPT. Did some tests with it by writing a simple OData client in ABAP. First, I was really impressed by the results. After some time dealing with the different outputs and proposals from ChatGPT, I realized that at least in my case, it was very similar to the results you could get when you "google" my test topic.

You have very good starting points with the help of ChatGPT, but you need a lot of knowledge around HTTP(S), JSON, OData, object oriented ABAP coding and many topics more.

So I totally agree with you and your statement: "So far we can see AI as a tool and not (yet) as a menace."

I always have to think of all the science fiction films and series in which the on-board computer is asked something and then supports the person. I can imagine something similar for our developer future.

But now we come to my social problem with this topic. I think ChatGPT was developed by very smart people. However, it is also used by less smart people who want to make facts and profit from the ChatGPT outputs. It could lead many "poor" developers on this planet into a dependency on artificial intelligence. Whoever leads and directs artificial intelligence directs many developers...

ChatGPT can make a decent "Google assistant" (although I still don't understand why some people are not able to use Google straight up, you can also ask "how to..." questions there). The main danger in placing too much faith in ChatGPT is that you need to have certain level of knowledge to understand when it's proposing nonsense. It's kind of a catch-22 and "you don't know what you don't know" situation. If we approach it with healthy skepticism, it can be helpful, just like many AI tools.

I'm not too worried though since more ChatGPT written programs will just mean extra income for me after retirement to fix all that stuff. π

You are a real businesswoman with a good instinct π But it feels like we're all really still working after retirement. At least in Germany, this could become necessary if you look at the topic of "pensions".

Just noticed the blog. Very cool Enno Wulff . Thanks for demonstrating it on my kata.