Feuertips #13: My Preconditions for Refactoring

I love how the new role of Steven Feuerstein, one of my personal tech-heroes, allows him to share his experience in new ways with the public. One of these new ways are his “Feuertips” – and the one happening tomorrow caught my interest, because the topic is “Let’s refactor!”

Feuertip #13: Let’s refactor! – Insum

I love refactoring, but I have some preconditions to do it in a self-confident way. One of the most important preconditions are some automated tests so I can be sure the functionality of the code I am going to change stays the same.

Therefore, I wanted to write and share a couple of high-level utPLSQL tests for the code Steven provides on the linked page.

I struggled a bit, however, because the code as it is posted gives a lot of opportunities to refactor, but due to some restrictions (Steven can of course not share internals of how these log tables look like) the code itself doesn’t reveal how the functionality is supposed to work.
It’s not clear what the final content in the files “abcd”, “arty”, etc. should look like, nor is it clear how the input looks like and how the inner and outer cursor loop is connected to each other.

Input and output are the most important things to know when writing example-based tests as I wanted to do, so I reached out to Steven and he gave me the additional information, that the inner loop are child-rows to the outer loop.

With that information and some assumptions how log tables on the DeathStar could look like, I added a couple of tables:

-- Log Table: All the rooms in the deathstar that were accessed at some point
create table log_deathstar_room_access
(
  id integer generated always as identity primary key,
  key_id integer,
  room_name varchar2(4000),
  character_name varchar2(4000),
  log_date timestamp default current_timestamp
);

-- Log Table: All the actions that happened in a room, but we don't have the room name, just some key_id
create table log_deathstar_room_actions
(
  id integer generated always as identity primary key,
  key_id integer,
  action varchar2(4000),
  done_by varchar2(4000),
  log_date timestamp default current_timestamp
);

-- Log Table: Repairs done in a room, but again we don't have the room name
create table log_deathstar_room_repairs
(
  id integer generated always as identity primary key,
  key_id integer,
  action varchar2(4000),
  repair_completion varchar2(100),
  repaired_by varchar2(4000),
  log_date timestamp default current_timestamp
);

Another thing I needed to do in order to actually run the code was to create a directory named “TEMP” and give read and write access to it, for example like that (as SYSDBA):

create or replace directory temp AS '/home/oracle/temp';
grant read, write on directory temp to sithdb;

As a last change, I needed to replace Steven’s select * from dual statements with some selects to the new tables.

I also think that the originally published code example has a flaw, because I guess we want to actually get some data from the crs2 cursors.

So the code I will be working with looks like that (in order to test it properly, we need to put it into a procedure or package, too):

create or replace procedure feuertips_13_poc as
   type T_LINES is
      table of varchar2(4000);
   k_full_date_format  constant varchar2(20) := 'mm/dd/yy';
   k_half_date_format  constant varchar2(20) := 'mm/yyyy';
   k_file1_name        constant varchar2(20) := 'abcd';
   k_file2_name        constant varchar2(20) := 'arty';
   k_file3_name        constant varchar2(20) := 'rett';
   k_file4_name        constant varchar2(20) := 'qsur';
   k_file5_name        constant varchar2(20) := 'wede';
   k_file6_name        constant varchar2(20) := 'cfrt';
   k_file7_name        constant varchar2(20) := 'ored';
   l_file1_lines       T_LINES;
   l_file2_lines       T_LINES;
   l_file3_lines       T_LINES;
   l_file4_lines       T_LINES;
   l_file6_lines       T_LINES;
   l_file5_lines       T_LINES;
   l_file7_lines       T_LINES;
   l_file1_lines_cnt   number := 0;
   l_file2_lines_cnt   number := 0;
   l_file3_lines_cnt   number := 0;
   l_file4_lines_cnt   number := 0;
   l_file6_lines_cnt   number := 0;
   l_file5_lines_cnt   number := 0;
   l_file7_lines_cnt   number := 0;
   l_file1_line        varchar2(4000);
   l_file2_line        varchar2(4000);
   l_file3_line        varchar2(4000);
   l_file4_line        varchar2(4000);
   l_file6_line        varchar2(4000);
   l_file5_line        varchar2(4000);
   l_file7_line        varchar2(4000);
   l_file1_name        varchar2(100) := k_file1_name;
   l_file2_name        varchar2(100) := k_file2_name;
   l_file3_name        varchar2(100) := k_file3_name;
   l_file4_name        varchar2(100) := k_file4_name;
   l_file6_name        varchar2(100) := k_file5_name;
   l_file5_name        varchar2(100) := k_file6_name;
   l_file7_name        varchar2(100) := k_file7_name;
   l_current_key       number default 0;
   l_first_time        number default 0;

   procedure init as
   begin
      l_file1_lines  := T_LINES();
      l_file2_lines  := T_LINES();
      l_file3_lines  := T_LINES();
      l_file4_lines  := T_LINES();
      l_file6_lines  := T_LINES();
      l_file5_lines  := T_LINES();
      l_file7_lines  := T_LINES();
   end init;

   procedure print_lines (
      p_file_name  in  varchar2,
      p_lines      in  T_LINES
   ) as
      l_file utl_file.file_type;
   begin
      l_file := utl_file.fopen('TEMP', p_file_name, 'w');
      for i in p_lines.first..p_lines.last loop
         utl_file.put_line(l_file, p_lines(i));
      end loop;
      utl_file.fclose (l_file);
   end print_lines;

   procedure add_line (
      p_file_name  in  varchar2,
      p_line       in  varchar2
   ) as
   begin
      case p_file_name
         when k_file1_name then
            l_file1_lines.extend();
            l_file1_lines(l_file1_lines.count)      := p_line;
            l_file1_lines_cnt                        := l_file1_lines_cnt + 1;
         when k_file2_name then
            l_file2_lines.extend();
            l_file2_lines(l_file2_lines.count)      := p_line;
            l_file2_lines_cnt                        := l_file2_lines_cnt + 1;
         when k_file3_name then
            l_file3_lines.extend();
            l_file3_lines(l_file3_lines.count)      := p_line;
            l_file3_lines_cnt                        := l_file3_lines_cnt + 1;
         when k_file4_name then
            l_file4_lines.extend();
            l_file4_lines(l_file4_lines.count)      := p_line;
            l_file4_lines_cnt                        := l_file4_lines_cnt + 1;
         when k_file5_name then
            l_file5_lines.extend();
            l_file5_lines(l_file5_lines.count)      := p_line;
            l_file5_lines_cnt                        := l_file5_lines_cnt + 1;
         when k_file6_name then
            l_file6_lines.extend();
            l_file6_lines(l_file6_lines.count)      := p_line;
            l_file6_lines_cnt                        := l_file6_lines_cnt + 1;
         when k_file7_name then
            l_file7_lines.extend();
            l_file7_lines(l_file7_lines.count)      := p_line;
            l_file7_lines_cnt                        := l_file7_lines_cnt + 1;
      end case;
   end add_line;

begin
   init;
   for crs in (
      select *
        from log_deathstar_room_access
   ) loop
      case
         when l_current_key != crs.key_id then
            l_first_time   := 1;
            l_current_key  := crs.key_id;
         else
            l_first_time := 2;
      end case;

      if l_first_time = 1 then
         for crs2 in (
            select *
              from log_deathstar_room_actions
              where key_id = crs.key_id
         ) loop
            l_file1_line := crs.key_id
                            || '|'
                            || crs.room_name
                            || '|'
                            || crs2.done_by
                            || '|'
                            || crs2.action;

            add_line(
               p_file_name  => k_file1_name,
               p_line       => l_file1_line
            );
         end loop;
      end if;

   end loop;

   print_lines(l_file1_name, l_file1_lines);

    /* Next! */

   for crs in (
      select *
        from log_deathstar_room_access) loop
      case
         when l_current_key != crs.key_id then
            l_first_time   := 1;
            l_current_key  := crs.key_id;
         else
            l_first_time := 2;
      end case;

      if l_first_time = 1 then
         for crs2 in (
            select *
              from log_deathstar_room_repairs
              where key_id = crs.key_id) loop
            l_file2_line := crs.key_id
                            || '|'
                            || crs.room_name
                            || '|'
                            || crs2.repair_completion
                            || '|'
                            || crs2.repaired_by
                            || '|'
                            || crs2.action;

            add_line(
               p_file_name  => k_file2_name,
               p_line       => l_file2_line
            );
         end loop;
      end if;

   end loop;

   print_lines(l_file2_name, l_file2_lines);

    /* And so on.... */
end;
/

The Test Package

To give myself enough confidence for refactoring this unfamiliar code, I at least wanted to have two examples how the output should look like given a certain input.

What I would usually do when refactoring, is to start extracting small pieces of the code and test them in isolation. This, however, is a practice and approach which is hard to transport via blog post and also has an immediate effect on the structure of the refactored code.

One of my goals, though, was to give everyone who is interested a very basic automated test, without touching the original code (at least as little as possible).

So please, if you want to participate in the upcoming Feuertips #13 session, feel free to use this little test package to make sure your code still works as I think it was supposed to work 😁

-- In order to use anydata.convertCollection in the utPLSQL 
-- expectation later on, we need t_lines to be a global type.
-- Unfortunately we cannot use package-level types that way
create or replace type t_lines is table of varchar2(4000);

create or replace package ut_feuertips_13 as

  -- %suite(Feuertips 13: LogWriter)

  -- %beforeall
  procedure setup_test_data;

  -- %test(File abcd contains Room Action Log)
  procedure abcd_contains_actionlog;

  -- %test(File arty contains Room Repair Log)
  procedure arty_contains_repairlog;

end;
/

create or replace package body ut_feuertips_13 as

  procedure delete_file(
    p_file_name in varchar2
  ) as
  begin
    utl_file.fremove('TEMP', p_file_name);
  exception when others then
    null; -- File doesnt exist
  end;

  function read_lines (
      p_file_name  in  varchar2
   ) return t_lines as
      l_file utl_file.file_type;
      l_buffer varchar2(4000);
      l_result t_lines := t_lines();
   begin
      l_file := utl_file.fopen('TEMP', p_file_name, 'r');
      begin
        loop
          utl_file.get_line(l_file, l_buffer);
          l_result.extend;
          l_result(l_result.count) := l_buffer;
        end loop;
      exception when no_data_found then
        null; -- we are finished reading
      end;
      utl_file.fclose (l_file);
      return l_result;
   end read_lines;

  procedure setup_test_data as
  begin
    -- Make sure the tables we're using as input are empty
    delete from log_deathstar_room_actions;
    delete from log_deathstar_room_access;
    delete from log_deathstar_room_repairs;

    -- Add some test data for our tests
    insert into log_deathstar_room_access ( key_id, room_name, character_name ) values (1, 'Bridge', 'Darth Vader');
    insert into log_deathstar_room_access ( key_id, room_name, character_name ) values (1, 'Bridge', 'Mace Windu');
    insert into log_deathstar_room_access ( key_id, room_name, character_name ) values (2, 'Engine Room 1', 'R2D2');

    insert into log_deathstar_room_actions ( key_id, action, done_by ) values ( 1, 'Enter', 'Darth Vader');
    insert into log_deathstar_room_actions ( key_id, action, done_by ) values ( 1, 'Sit down', 'Darth Vader');
    insert into log_deathstar_room_actions ( key_id, action, done_by ) values ( 2, 'Enter', 'R2D2');
    insert into log_deathstar_room_actions ( key_id, action, done_by ) values ( 1, 'Enter', 'Mace Windu');
    insert into log_deathstar_room_actions ( key_id, action, done_by ) values ( 1, 'Activate Lightsaber', 'Mace Windu');
    insert into log_deathstar_room_actions ( key_id, action, done_by ) values ( 1, 'Jump up', 'Darth Vader');
    insert into log_deathstar_room_actions ( key_id, action, done_by ) values ( 2, 'Hack Console', 'R2D2');
    insert into log_deathstar_room_actions ( key_id, action, done_by ) values ( 1, 'Activate Lightsaber', 'Darth Vader');
    insert into log_deathstar_room_actions ( key_id, action, done_by ) values ( 1, 'Attack', 'Mace Windu');
    insert into log_deathstar_room_actions ( key_id, action, done_by ) values ( 2, 'Beep', 'R2D2');

    insert into log_deathstar_room_repairs ( key_id, action, repair_completion, repaired_by) values ( 1, 'Inspect', '50%', 'The Repairman');
    insert into log_deathstar_room_repairs ( key_id, action, repair_completion, repaired_by) values ( 1, 'Analyze', '53%', 'The Repairman');
    insert into log_deathstar_room_repairs ( key_id, action, repair_completion, repaired_by) values ( 1, 'Investigate', '57%', 'The Repairman');
    insert into log_deathstar_room_repairs ( key_id, action, repair_completion, repaired_by) values ( 2, 'Analyze', '25%', 'The Repairman');
    insert into log_deathstar_room_repairs ( key_id, action, repair_completion, repaired_by) values ( 1, 'Fix it', '95%', 'Lady Skillful');
    insert into log_deathstar_room_repairs ( key_id, action, repair_completion, repaired_by) values ( 2, 'Fix it', '100%', 'Lady Skillful');
  end;

  procedure abcd_contains_actionlog as
    l_actual t_lines;
    l_expect t_lines := t_lines();
  begin
    -- Arrange
    delete_file('abcd'); -- Make sure the file we want to check is empty at first

    -- Act
    feuertips_13_poc();

    l_actual := read_lines('abcd');
    l_expect.extend(10);
    l_expect(1) := '1|Bridge|Darth Vader|Enter';
    l_expect(2) := '1|Bridge|Darth Vader|Sit down';
    l_expect(3) := '1|Bridge|Mace Windu|Enter';
    l_expect(4) := '1|Bridge|Mace Windu|Activate Lightsaber';
    l_expect(5) := '1|Bridge|Darth Vader|Jump up';
    l_expect(6) := '1|Bridge|Darth Vader|Activate Lightsaber';
    l_expect(7) := '1|Bridge|Mace Windu|Attack';
    l_expect(8) := '2|Engine Room 1|R2D2|Enter';
    l_expect(9) := '2|Engine Room 1|R2D2|Hack Console';
    l_expect(10) := '2|Engine Room 1|R2D2|Beep';

    ut.expect(anydata.convertCollection(l_actual)).to_equal(anydata.convertCollection(l_expect));
  end;

  procedure arty_contains_repairlog as
    l_actual t_lines;
    l_expect t_lines := t_lines();
  begin
    -- Arrange
    delete_file('arty'); -- Make sure the file we want to check is empty at first

    -- Act
    feuertips_13_poc();

    l_actual := read_lines('arty');
    l_expect.extend(6);
    l_expect(1) := '1|Bridge|50%|The Repairman|Inspect';
    l_expect(2) := '1|Bridge|53%|The Repairman|Analyze';
    l_expect(3) := '1|Bridge|57%|The Repairman|Investigate';
    l_expect(4) := '1|Bridge|95%|Lady Skillful|Fix it';
    l_expect(5) := '2|Engine Room 1|25%|The Repairman|Analyze';
    l_expect(6) := '2|Engine Room 1|100%|Lady Skillful|Fix it';

    ut.expect(anydata.convertCollection(l_actual)).to_equal(anydata.convertCollection(l_expect));
  end;

end;
/

select * from (ut.run('ut_feuertips_13'));