[Archive] Refactoring my first ever program

This article is an archive from my old blog. The information presented may be outdated and no longer reflect my current views.

As I write this article, it has been 10 14 years since I first wrote code.

I still remember the joy I felt when, after hours of research, the little program I had just written finally worked as I had imagined.

Whether it’s launching a website, developing a script, or setting up cloud architecture, I feel like I rediscover that immense satisfaction every time my work functions as intended.

After this initial phase, there comes a time when the first version of a solution starts to show its weaknesses. Bugs appear, the addition of pieces of code degrades maintainability, and it’s time to refactor the program.

Having found the source code of the aforementioned program, I will indulge in this exercise today with a touch of nostalgia.

colors.bat

When I first got interested in coding, installing anything on the family computer wasn’t an option. I therefore turned to command-line application programming, which I wrote using the Windows XP Notepad.

The resulting program is called colors.bat. It has a single function: to ask the user to choose a color and then change the console color to reflect that choice.

So, are you ready to see this monstrosity? 🙃

@echo off
:debut
cls
set /p lol=Quel est ta couleur prefere ?
if "%lol%"=="noir" goto noir
if "%lol%"=="bleu" goto bleu
if "%lol%"=="vert" goto vert
if "%lol%"=="marron" goto marron
if "%lol%"=="pourpre" goto pourpre
if "%lol%"=="kaki" goto kaki
if "%lol%"=="gris" goto gris
if "%lol%"=="rouge" goto rouge
if "%lol%"=="rose" goto rose
if "%lol%"=="jaune" goto jaune
if "%lol%"=="blanc" goto blanc
:noir
color 0f
pause
goto fin
:bleu
set /p choi=Quel bleu ?(fonce,gris,clair ou cyan)
if "%choi%"=="fonce" goto fonce
if "%choi%"=="gris" goto gris2
if "%choi%"=="clair" goto clair
if "%choi%"=="cyan" goto cyan
:vert
set /p lki=Quel vert ?(vert ou clair)
if "%lki%"=="vert" goto vertvert
if "%lki%"=="clair" goto clair2
:marron
color 40
pause
goto fin
:pourpre
color 50
pause
goto fin
:kaki
color 60
pause
goto fin
:gris
set /p lkj=Quel gris ?(gris ou gris clair)
if "%lkj%"=="gris" goto gris3
if "%lkj%"=="clair" goto clair3
:rouge
color c0
pause
goto fin
:rose
color d0
pause
goto fin
:jaune
color e0
pause
goto fin
:blanc
color f0
pause
goto fin
:fonce
color 10
pause
goto fin
:gris2
color 30
pause
goto fin
:clair
color 90
pause
goto fin
:cyan
color b0
pause
goto fin
:vertvert
color 20
pause
goto fin
:clair2
color a0
pause
goto fin
:gris3
color 80
pause
goto fin
:clair3
color 70
pause
goto fin
:fin
cls
echo FIN !!!
pause
set /p encore=Une autre couleur ?(oui ou non)
if "%encore%"=="oui" goto debut
if "%encore%"=="non" goto dommage
:dommage
echo Dommage ! Une prochaine fois peut etre ...
pause

Analysis

Where to begin?

Let’s ignore the spelling mistakes for a moment and take a line-by-line look at what this piece of code does.

@echo off
:debut
cls
set /p lol=Quel est ta couleur prefere ?

The first line @echo off is a Windows script specific instruction. It simply tells the console not to display the commands being executed, only their results.

The second instruction :debut defines a section of the program that can be accessed later with the goto debut instruction, thus modifying the flow of execution from top to bottom.

The third instruction clears the console content.

The fourth highlighted instruction defines a variable lol that contains the result of a user input after displaying the prompt “What is your favorite color?”. If only I had known I would be reviewing this someday...

Not to mention the haphazard naming of variables, we see that several elements pose issues.

Indeed, from a user experience perspective, there is no indication of the expected format of the user input. Without any guidance, the user might think they could enter anything from “blue” to “light blue with a hint of green” or even “#0000ff”.

if "%lol%"=="noir" goto noir
if "%lol%"=="bleu" goto bleu
if "%lol%"=="vert" goto vert
if "%lol%"=="marron" goto marron
if "%lol%"=="pourpre" goto pourpre
if "%lol%"=="kaki" goto kaki
if "%lol%"=="gris" goto gris
if "%lol%"=="rouge" goto rouge
if "%lol%"=="rose" goto rose
if "%lol%"=="jaune" goto jaune
if "%lol%"=="blanc" goto blanc

Once the variable lol is defined, these instructions change the flow of execution to access the correct section based on the user input.

:bleu
set /p choi=Quel bleu ?(fonce,gris,clair ou cyan)
if "%choi%"=="fonce" goto fonce
if "%choi%"=="gris" goto gris2
if "%choi%"=="clair" goto clair
if "%choi%"=="cyan" goto cyan
:vert
set /p lki=Quel vert ?(vert ou clair)
if "%lki%"=="vert" goto vertvert
if "%lki%"=="clair" goto clair2

For some colors, the program asks for details on the shade of the color and assigns it to a new variable choi for blue and lki for green in this example.

Again, we’ll discuss the naming of these variables in the next section of this article.

:blanc
color f0
pause
goto fin
:fonce
color 10
pause
goto fin
:gris2
color 30
pause
goto fin
:clair
color 90
pause
goto fin

Each section defined by a label (e.g., :white) does three things:

1. Changes the display color according to the user’s choice.
2. Puts the program on pause, i.e., waits for the user to press a key before continuing execution.
3. Jumps to the fin block.

:fin
cls
echo FIN !!!
pause
set /p encore=Une autre couleur ?(oui ou non)
if "%encore%"=="oui" goto debut
if "%encore%"=="non" goto dommage
:dommage
echo Dommage ! Une prochaine fois peut etre ...
pause

After clearing the screen and displaying the end message, a new variable another allows the user to return to the beginning of the program or terminate it based on whether they entered “yes” or “no”.

Areas for Improvement

As seen, even though the program works, it presents numerous areas for improvement.

User Experience

As previously mentioned, the instructions given to the user are not clear.

In the new version, I suggest guiding the user by clearly displaying the type of data they need to provide.

Instead of the current “What is your favorite color?”, I propose displaying a list of all acceptable options:

ECHO What's your favorite color in this list?
ECHO:
ECHO - Black - Gray
ECHO - Blue - Light Blue
ECHO - Green - Light Green
ECHO - Aqua - Light Aqua
ECHO - Red - Light Red
ECHO - Purple - Light Purple
ECHO - Yellow - Light Yellow
ECHO - White - Bright White

Code Readability

The first thing that jumps out at me when reading the code is the lack of distinction between system instructions, variables, and string literals.

Even though the code embedded in this article is neatly formatted, it should be noted that a developer might work on this file using a text editor like vim or nano, which might not have proper syntax highlighting for this file format.

My proposal is as follows: all system instructions should be written in uppercase, and all variables in lowercase.

I also arbitrarily decided that variables should be written in camel case for uniformity.

Since the script is divided into several stages (color selection, displaying the color, and end screen), it is appropriate to reflect this organization in the code by adding blank lines when necessary. This way, at a glance, we can see that each block performs a distinct operation:

IF /I "%color%"=="bright white" SET colorCode=f0
)
IF "%colorCode%"=="" (
ECHO This program can't handle this color.
) ELSE (
COLOR %colorCode%
ECHO Here's a nice %color% for you!
)
ECHO:
ECHO Do you want to try another one? (y/N)

Variable Naming

Let’s list the variables used in this first version: lol, choi, lki, lkj, and encore.

If I asked you to guess what they contain based solely on their names, I’m sure no one would guess that lol contains the color name or that lki contains a shade.

In my final version, I reduced the number of variables to the strict minimum with each having a name that clearly reflects its content:

DRY - Dont Repeat Yourself

The second issue that stands out is the following:

:rouge
color c0
pause
goto fin
:rose
color d0
pause
goto fin
:jaune
color e0
pause
goto fin
:blanc
color f0
pause
goto fin
:fonce
color 10
pause
goto fin

As described earlier, these lines mostly do the same thing except for the color instruction, which changes based on the user’s choice.

A better solution is to handle this logic later in the program after setting the color code in a variable higher up:

IF /I "%color%"=="black" SET colorCode=0f
IF /I "%color%"=="blue" SET colorCode=1f
IF /I "%color%"=="green" SET colorCode=2f
IF /I "%color%"=="aqua" SET colorCode=3f
IF /I "%color%"=="red" SET colorCode=4f

IF "%colorCode%"=="" (
ECHO This program can't handle this color.
) ELSE (
COLOR %colorCode%
ECHO Here's a nice %color% for you!
)

From 20 lines in the previous example, we now have only 6 lines once the repeated logic is avoided. The readability of the program is greatly improved, and a developer can easily add a color in the future without having to touch the core logic of the program.

Final Version

With these improvements, the program becomes significantly easier to read, understand, and use.

If a developer wants to add a color, they only need to add it to the list of possibilities and append its code to the existing ones.

@ECHO OFF

:defineColor
CLS
COLOR 0f

ECHO What's your favorite color in this list?
ECHO:
ECHO - Black        - Gray
ECHO - Blue         - Light Blue
ECHO - Green        - Light Green
ECHO - Aqua         - Light Aqua
ECHO - Red          - Light Red
ECHO - Purple       - Light Purple
ECHO - Yellow       - Light Yellow
ECHO - White        - Bright White
ECHO:
SET /P color=^>

IF "%color%"=="" (
  GOTO defineColor
) ELSE (
  IF /I "%color%"=="black" SET colorCode=0f
  IF /I "%color%"=="blue" SET colorCode=1f
  IF /I "%color%"=="green" SET colorCode=2f
  IF /I "%color%"=="aqua" SET colorCode=3f
  IF /I "%color%"=="red" SET colorCode=4f
  IF /I "%color%"=="purple" SET colorCode=5f
  IF /I "%color%"=="yellow" SET colorCode=60
  IF /I "%color%"=="white" SET colorCode=70
  IF /I "%color%"=="gray" SET colorCode=80
  IF /I "%color%"=="light blue" SET colorCode=9f
  IF /I "%color%"=="light green" SET colorCode=a0
  IF /I "%color%"=="light aqua" SET colorCode=b0
  IF /I "%color%"=="light red" SET colorCode=cf
  IF /I "%color%"=="light purple" SET colorCode=d0
  IF /I "%color%"=="light yellow" SET colorCode=e0
  IF /I "%color%"=="bright white" SET colorCode=f0
)

IF "%colorCode%"=="" (
  ECHO This program can't handle this color.
) ELSE (
  COLOR %colorCode%
  ECHO Here's a nice %color% for you!
)

ECHO:
ECHO Do you want to try another one? (y/N)
SET /P retry=^>
IF /I "%retry%"=="y" GOTO defineColor
IF /I "%retry%"=="yes" GOTO defineColor

EXIT /B %ERRORLEVEL%