sdm03/report.tex

\documentclass[a4paper,11pt]{scrartcl}
\topskip=0pt
\parskip=5pt
\parindent=0pt
%\baselineskip=0pt
\usepackage[utf8]{inputenc}
\usepackage{geometry}
\usepackage{enumitem}
\usepackage{lmodern}
\usepackage{multirow}
\usepackage{graphicx}
\usepackage{booktabs}
\usepackage{listings}
\usepackage{float}
\usepackage{hyperref}
\usepackage{cleveref}
\usepackage{listings}
\usepackage[justification=centering]{caption}
\usepackage[bottom]{footmisc}
\usepackage{xcolor}

\definecolor{codegreen}{rgb}{0,0.6,0}
\definecolor{codegray}{rgb}{0.5,0.5,0.5}
\definecolor{codepurple}{rgb}{0.58,0,0.82}
\definecolor{backcolour}{rgb}{0.95,0.95,0.92}

\lstdefinestyle{mystyle}{
    backgroundcolor=\color{backcolour},   
    commentstyle=\color{codegreen},
    keywordstyle=\color{magenta},
    keywordstyle=[2]{\color{olive}},
    numberstyle=\tiny\color{codegray},
    stringstyle=\color{codepurple},
    basicstyle=\ttfamily\footnotesize,
    breakatwhitespace=false,         
    breaklines=true,                 
    captionpos=b,  
    keepspaces=true,                 
    numbers=left,  
    numbersep=5pt,
    showspaces=false,                
    showstringspaces=false,
    showtabs=false,
    tabsize=2,
    aboveskip=0.8em,
    belowcaptionskip=0.8em
}
\lstset{style=mystyle}

\geometry{left=2cm,right=2cm,top=2cm,bottom=3cm} % se serve spazio
\title{
\vspace{-5ex}  % se serve spazio
Assignment 3 -- Software Design and Modelling \\\vspace{0.5cm}
\Large Refactoring the Design of an Existing Project
\vspace{-1ex}  % se serve spazio
}
\author{Claudio Maggioni \and Raffaele Morganti}
\date{\vspace{-3ex}} % se serve spazio

\begin{document}
\maketitle

\section{Project Selection}

The aim of this project is to refactor a part of or a complete application, to achieve good software design without changing the application behaviour. No restrictions are placed on the size or type of project, other than being hosted on GitHub as a public repository.

We choose the project \textbf{\href{https://github.com/dtschust/Zork}{dtschust/Zork}}, which is inspired by the namesake game \textit{Zork}\footnote{\url{https://en.wikipedia.org/wiki/Zork}} released in the early 1980s. 

This project is written in Java and it provides a configurable framework of text-based adventure game mechanics. The program is able to parse and execute a given story, which must be provided in an XML file following a specific format.

We choose this project mainly for two reasons:
\begin{itemize}[itemsep=0pt,topsep=0pt]
    \item It is small enough to be deeply understood and refactored in a couple of weeks. According to \textit{cloc} and as shown in \cref{fig:cloc}, the project has less than 2000 lines of executable code;
    \item The project has a large number of design anti-patterns, and we think we have an opportunity to perform a significant and interesting refactor.
\end{itemize}

\begin{figure}[h]
  \centering
  \begin{tabular}{lrrrr}
    \toprule
    Language & Files & Blank & Comment & Code \\
    \midrule
    Java & 1 & 114 & 77 & 1264 \\
    XML & 10 & 0 & 0 & 530 \\
    Text & 1 & 1 & 0 & 83 \\
    make & 1 & 1 & 0 & 4 \\
    Markdown & 1 & 1 & 0 & 3 \\
    Properties & 1 & 0 & 2 & 3 \\
    \midrule
    Total & 15 & 117 & 79 & 1887 \\
    \bottomrule
  \end{tabular}
  \caption{Output of the \textit{cloc} tool for the \textbf{dtschust/Zork} project before refactoring.}
  \label{fig:cloc}
\end{figure}

\section{The Project Before Refactoring}

A copy of the contents of \textbf{dtschust/Zork} can be found in branch \texttt{main} of the repository 

\begin{center}
{\small\href{https://gitlab.com/usi-si-teaching/msde/2022-2023/software-design-and-modeling/assignment-3-refactoring/group7}{usi-si-teaching/msde/2022-2023/software-design-and-modeling/assignment-3-refactoring/group7}}    
\end{center}

on \textit{gitlab.com}.

As figure \ref{fig:cloc} proves, all the implementation code is contained in a single Java file. A total of 1264 code lines, 77 comments, and 114 blank lines are found. As the project is composed by only 11 classes, we decide to manually inspect the source code to find instances of bad design that we can improve with refactoring.

The \textit{Zork} class is over 1200 lines long (including blanks and comments) and handles almost all the application logic, making it an obvious instance of the god class anti-pattern. The class violates the single choice responsibility principle, as it handles both XML parsing and the actual execution of the given story. Given that the XML story file specification is non-trivial, the parsing logic may be modular. However, the relevant code is all placed in a single method, as shown in \cref{lst:pre-zork}. 
\marginpar[right text]{\color{white}\url{https://youtu.be/icpS6CgfGLo}}

\begin{lstlisting}[caption=An excerpt from the XML parsing logic found in the \textit{Zork} class. All the contents of the files are parsed in this section\, without any delegation to other methods or classes other than the Java DOM API.,language=java,label=lst:pre-zork]
Element rootElement = doc.getDocumentElement();

/* Every single first generation child is a room, container, creature, or item.  So load them in*/
NodeList nodes = rootElement.getChildNodes();
for (k=0;k<nodes.getLength();k++)
{
    Node node = nodes.item(k);
    Element element;
    if (node instanceof Element)
    {
    /* [511 lines omitted] */ 
    }
}
\end{lstlisting}


5 of the other 10 classes, namely \textit{ZorkObject}, \textit{ZorkCreature}, \textit{ZorkItem}, \textit{ZorkContainer} and \textit{ZorkRoom} do not contain any methods, making them data clumps. The sources for \textit{ZorkContainer} are shown in \cref{lst:pre-cont} as an example of this. Attributes in classes are generally public and mutable, allowing other classes to modify their internal state. This indicates low encapsulation and tight coupling between the modules in this application. 

\begin{lstlisting}[caption=Complete source code for the \textit{ZorkContainer} class before refactoring.,language=java,label=lst:pre-cont]
class ZorkContainer extends ZorkObject
{
  public String name;
  public HashMap<String,String> item = new HashMap<String,String>();
  public String description;
  public ArrayList<String> accept = new ArrayList<String>();
  boolean isOpen;
  public ZorkContainer()
  {
  }
}
\end{lstlisting}

The class \textit{ZorkTrigger} encodes an action to be performed in the game state when a given condition is met. The class however, is subject to feature envy by the aforementioned \textit{Zork} class. While  condition evaluation is correctly implemented inside it (specifically in the method \textit{boolean evaluate(Zork)}), the action executing code is instead placed in the god class. Moreover, the condition evaluation is itself envious of \textit{Zork} as it takes the responsibility to fetch the current user input (stored in field \textit{String Zork.userInput}). Listing \ref{lst:pre-cond} shows the code where this takes place.

\begin{lstlisting}[caption=The class \textit{ZorkCommand} (a sub-class of \textit{ZorkCondition}) is envious of how to fetch the current user input by retrieving it from a public field in \textit{Zork} class.,language=java,label=lst:pre-cond]
class ZorkCommand extends ZorkCondition
{
  String command;
  public boolean evaluate(Zork zork)
  {
    if (command.equals(zork.userInput))
      return true;
    else
      return false;
  }
}
\end{lstlisting}


Finally, we execute the \textit{Sonarqube} tool to have a wider overview of the project in terms of metrics. A summary of the \textit{Sonar Scanner} output is shown in \cref{fig:sonar_initial}. Unsurprisingly the results are not stellar. In particular, we want to point out the ``Cognitive Complexity'' metric, which is related to the number of boolean conditions checked in methods. Our refactoring manages to lower its value by a factor of 6, showing how the project originally has a very high density of logic per method.

\begin{figure}[ht]
    \centering
    \includegraphics[width=\textwidth,clip]{sonar_initial.png}
    \caption{Summary of the \textit{Sonar Scanner} detection on the initial project}
    \label{fig:sonar_initial}
\end{figure}

In conclusion, we determine that the code in \textbf{dtschust/Zork} does not follow OOP design best practices, in particular falling short of providing loose coupling and defining an effective separation of concerns between its classes. Our refactor aims to introduce better design to the project to increase modularity, which in turn increases the testability of each class and the readability of the overall code base.

\section{Refactoring}

A copy our refactor of \textbf{dtschust/Zork} can be found in branch \texttt{refactored} of the repository 

\begin{center}
{\small\href{https://gitlab.com/usi-si-teaching/msde/2022-2023/software-design-and-modeling/assignment-3-refactoring/group7/-/tree/refactored}{usi-si-teaching/msde/2022-2023/software-design-and-modeling/assignment-3-refactoring/group7}}    
\end{center}

on \textit{gitlab.com}.

\subsection{Source Code Structure and Build System}

The first change we make on the repository is to introduce \textit{Maven} as a build system and following its standard directory structure mandated for source code and tests. We also split the \textit{Zork.java} file in several files, one per class, and we move each class under the \textit{com.github.dtschust.zork} package as placing classes in the default package (their original position) is considered bad practice in Java. 

These steps are relatively trivial but however crucial to make our work easier with further refactoring efforts. Additionally, this simplified the implementation of (originally missing) test code to check the behaviour of the program remains the same.

\subsection{XML Parsing Logic}

\begin{figure}[ht]
    \centering
    \includegraphics[width=\textwidth,clip]{parser.png}
    \caption{UML class diagram of the \textit{com.github.dtschust.zork.parser} package}
    \label{fig:umlp}
\end{figure}

We then separate the XML parsing logic in the package \textit{com.github.dtschust.zork.parser}, whose UML class diagram is shown in \cref{fig:umlp}. 
Here, the XML-specific logic is separate from the application-specific allocation of the story data structure. 

DOM elements and lists of elements are encapsulated in the \textit{dom.DOMElement} and \textit{dom.DOMElementList} proxy classes. These classes in turn are used to implement an encoding-agnostic interface named \textit{Property}, which abstracts the story file to a collection of property names, property values and sub-properties.

At each nesting level a different parsing strategy is defined as a class under the \textit{strategy} package. Each class implements a method that takes a \textit{Property} and returns an instance of the matching application-specific entity. Each strategy may depend to other strategies through dependency injection, with inversion of control handled by the \textit{ParserIOC} class.

The parser is now a collection of 11 classes and 3 interfaces with a single entry point. With the line of code \texttt{ParserIOC.xmlParser().parse(filename, System.out)} the entire story data structure is parsed from the XML file and instantiated. Our new design increases testability by separating each nesting level into easily mockable classes. Moreover, the new abstractions allow for easy implementation of parsers for other encodings.

\subsection{User Actions}

\begin{lstlisting}[caption=Implementation of the \textit{read} action in the command class \textit{action.ReadAction}.,language=java,label=lst:readaction]
public class ReadAction implements Action {
    @Override
    public boolean matchesInput(List<String> arguments) {
        return arguments.get(0).equals("read");
    }

    @Override
    public int getMinimumArgCount() {
        return 2;
    }

    @Override
    public boolean run(ZorkGame game, List<String> arguments) {
        return game.getItem(arguments.get(1)).map(i -> {
            game.stream.println(i.getWriting());
            return true;
        }).orElse(false);
    }
}
\end{lstlisting}


We decide to implement the execution of user invoked or triggered actions as a variation of the command pattern. All this logic is now in the package \textit{com.github.dtschust.zork.repl}. 

All actions are implemented as a command class under the \textit{action} package, which implement a method \texttt{run} taking as parameters the story data and returning a \textit{boolean} indicating success of failure. An example of command class is shown in \cref{lst:readaction}.

The dispatcher is implemented in the class \textit{ActionDispatcher}. Our variation of the command pattern makes this dispatcher responsible for parsing the action string coming from triggers and the user, choosing the right action to invoke aided by methods in each command class, and dispatching the command providing the parsed action arguments as parameters of the method \textit{run}. This variation is done to cater the domain-specific need of providing a text based REPL (Read-eval-print loop) like interface to the user. 

\subsection{Story Data}

In the overall project we also perform several refactor on the classes modelling the story data and the game state. Fields on these classes are now immutable whenever possible and always \textit{private} to provide better information hiding and to loosen coupling. Additionally, the feature envy anti-patterns are solved by moving the respective methods into the classes they fit in. This required an extensive refactor of the the trigger-related logic from methods in class \textit{Zork} to methods in \textit{GameData} and improved condition evaluation methods in the \textit{ZorkCondition} hierarchy exploiting dynamic dispatch.

\section{Testing}

A problem we found was the absence of tests, so we needed to write them to ensure to don't introduce behavioral changes.

The original GitHub repository includes the two files named \textit{sampleGame.xml} and \textit{RunThroughResults.txt}. The first contains the game configuration (with the definition of all the rooms, items, creatures, etc. in the game). The latter is instead a log-file containing a sequence of inputs and their respective output.

We used these files to build a system test. In particular, our test is starting the game in a thread and mocks the default system input/output interface to automatically send the commands to the game. The implementation logic of this test is shown in \cref{lst:test}. 

\begin{lstlisting}[caption={Code of the system test we implemented.},language=java,label={lst:test}]
@Test
void testSampleGame() {
    String gameConfig = "sampleGame.xml";
    String gameExecution = "RunThroughResults.txt";

    CommandReader run = new CommandReader(gameExecution);
    IOWrapper io = new IOWrapper(true);
    new Thread(() -> {
        try {
            catchSystemExit(() -> Zork.runZork(gameConfig));
        } catch (Exception ignored) {}
    }).start();
    while(true){
        switch(run.getInstructionType()) {
            case SEND:
                io.write(run.getInstruction());
                break;
            case RECV:
                assertEquals(run.getInstruction(), io.read());
                break;
            default:
                io.restore();
                return;
        }
    }
}
\end{lstlisting}

Although it doesn't cover all of the possible outcomes, it was the best option available to ensure the behavioral consistency. Since this game has been realized as an assignment for a university course, a \textit{zorkRequirements.pdf} file with all the specification is available, and we also relied on it. 

To verify the behavior of the initial project, we don't added tests to the original sources, but to the ones already converted to a Maven project for convenience.

A copy of the tests added to the \textbf{dtschust/Zork} can be found in branch \texttt{main-with-test} of the repository 

\begin{center}
{\small\href{https://gitlab.com/usi-si-teaching/msde/2022-2023/software-design-and-modeling/assignment-3-refactoring/group7}{usi-si-teaching/msde/2022-2023/software-design-and-modeling/assignment-3-refactoring/group7}}    
\end{center}

on \textit{gitlab.com}.

\section{Conclusions}

At the end of the refactoring we ran again \textit{Sonar Scanner}, with a summary of the results in \cref{fig:sonar_final}.

\begin{figure}[ht]
    \centering
    \includegraphics[width=\textwidth,clip]{sonar_final.png}
    \caption{Summary of the \textit{Sonar Scanner} detection on the refactored project}
    \label{fig:sonar_final}
\end{figure}

By comparing these with the ones in \cref{fig:sonar_final} described at the start of this report, we can asses there is a huge improvement. The main benefits are the reduced ``cognitive complexity" from a score of 631 to 108 and the removal of code duplication.

In the end the project size is almost identical, however the source deeply changed, with the initial 20 methods and 11 classes extracted in a total of 171 methods and 47 classes. Now the code takes advantage from the basic object-oriented programming principles.

At the first impact the code wasn't really easy to understand, so our first steps in our refactor process were the method extraction of logically separated tasks. After that we started to extract classes and find some design-patterns (like the command pattern described above). Only in the end we cared about the visibility of the attributes and we enforced encapsulation. By working with this step-by-step strategy made our job easier and we didn't encountered any insurmountable barrier. Approaching to refactor in a continuous way allowed us to don't stuck with too hard and too big changes to be done in a single run. If we need to state what was the hardest task, we found the implementation of the command pattern the most challenging overall.

\section{Future work}

As always happens when talking about refactoring, it will be always possible to do something more. However we think our extensive refactor improved all aspects of this project and removed all the anti-patterns.

Still some improvements are possible, as also the last run of \textit{Sonar Scanner} shows in the ``code smells" section. In particular the 3 detected as ``critical" are related to the usage of the generics (\cref{lst:future}) and they can be removed. However since in all the three cases these generics are bounded we don't see to this as a high priority issue and we consider the required effort bigger than any potential benefit of fixing these.

\begin{lstlisting}[caption={Usage of wildcard types in the code.}, language=java,label={lst:future}]
// class ZorkGame
public Optional<? extends ZorkObject> getObject(final String objectName);
//class Property
List<? extends Property> subProperties();
List<? extends Property> subPropertiesByName(String name);
\end{lstlisting}

The 6 ``major" issues instead, are less interesting:

\begin{itemize}
    \item Two of them are about the number of parameters in the constructor for the \textit{ZorkRoom} and \textit{ZorkCreature} classes, but they are instantiated through the respective \textit{StrategyParser.parser} method that is working as a builder.
    
    \item The other four are related to the usage of calls to the \textit{System.out} (all of them are sort of logs printed when  some exceptions are thrown if trying to load of a wrong game configuration file). Making more informative logs of the causes of these exception would certainly be useful, but this improvement is out of the scope of refactoring.
\end{itemize}

In conclusion, although \textit{Sonar Scanner} detected them, we don't think they are really relevant.

\end{document}