Past Self Code Review!

A short while ago, I finished reading this very cool book called Clean Code: A Handbook of Agile Software Craftsmanship, by Robert C. Martin, or as he's known in the industry, "Uncle Bob". The book gives you lots of advice on how to improve code readability and structure. I've already seen the lessons mentioned in this book applied in a professional code base, and I can say that clean code is much easier to debug!

Up next, I'll put some of Uncle Bob's advice to the test in some code from my old samples.

Let's now talk about some old code!!

A few years ago I wrote this short program(in C++) to show how I solve a string problem:

Given a null-terminated string that contains a hexadecimal number, write a function that parses the string and returns the numerical int value.

Assumptions:

The string will be in a valid hexadecimal format:"0xEA877". In this example, the decimal value would be 960631

Here's my solution from roughly 6 years ago:

#include <stdio.h>

int ParseHexadecimalString(const char* hexString)
{
  //General assumption:
  //The string is properly formatted like 0x....

  if (!hexString) return 0; ///additional nullptr check
  if (hexString[1] != 'x' || hexString[0] != '0') return 0;

  //move a reader const char* to the end of the string
  const char* reader = (hexString += 2);
  while (*(reader + 1))
    ++reader;

  //declare result value
  int value = 0; 


  int increment = 1;
  for (; reader >= hexString; --reader)
  {
    char hexDigit = *reader;

    //if digit is within 0 & 9, consider it normally
    if (hexDigit >= '0' && hexDigit < '9')
    {
      hexDigit -= '0';
    }
    //it was wrong, it did not use an else if
    else if (hexDigit >= 'A' && hexDigit <= 'F')
    {
      //get the digit in range of 10 to 15
      hexDigit = 10 + (hexDigit - 'A');
    }

    else
    {
      return 0;
    }
    value += increment * hexDigit;//value += hexDigit; wrong…
    increment *= 16;

  }
  return value;
}

int main() {
    printf("%x", ParseHexadecimalString("0x5567"));
}

As you can see it's not that complicated, and I was able to remember fairly well what I'm doing. But really, this is not such a tough problem to solve. If the complexity would've been much higher, I would've taken much longer to remember what the heck I did.

Being my own critic, I made a mental note of some stuff I didn't like:

I'm inconsistent in my own formatting! I use...
```
function() {
...
}
```
and ...
```
function()
{
...
}
```
... in the same code.
I leave unused code with unnecessary comments:
```
value += increment * hexDigit;//value += hexDigit; wrong…
```
If it's wrong, then just remove it! 🤣
Too much comment and being redundant at the same time:
```
//declare result value
int value = 0;
```
You have the option to use descriptive variable names!😅

Checking for nullptr is good, but it's written in the assumptions that I'll receive a good format("0x..."). No need to check the input!

//General assumption(s):
//The string is properly formatted like 0x....

if (!hexString) return 0; ///additional nullptr check
if (hexString[1] != 'x' || hexString[0] != '0') return 0;

I even say it on the comments! 🙄

Just ONE test and it's all good?! 😬

int main() {
  printf("%x", ParseHexadecimalString("0x5567"));
}

📣Note: Please feel free to point out other stuff I did not mention in my short list.

Let's now re-work some old code!!

I'll now apply some of the first lessons that you learn from Uncle Bob's book. The 2nd and 3rd chapters talk respectively about meaningful names and proper function structure and usage.

Some of the advice discussed in these chapters:

When naming a variable or a function...

Make sure that you use descriptive names that mention the purpose.
Use searchable names
Don't use "internal jokes", or words without proper context

When writing a function...

Functions should do one thing, and they should do it well.
There's almost no need for a long list of parameters.

Follow The Stepdown Rule. Use the To... paragraphs:

/*
To parse a hexadecimal string one must process each character, starting from the last one, to one by one, updating the return value.
    To process each character one must determine if it's numerical (0 to 9) or a letter (A to F)
          To determine if it's numerical or a letter...
*/

It even looks like a recipe!

Here's my attempt! 💪🏽

#include <stdio.h>

void ParseHexadecimalStringTest(const char* hexString);

int ParseHexadecimalString(const char* hexString);
const char* GetPointerToLastCharacterOfHexString(
    const char** hexStringReference);
char GetHexCharacterNumericalValue(char hexCharacter);
bool IsHexCharacterWithinZeroAndNine(char hexCharacter);
bool IsHexCharacterWithinAandFupperCase(char hexCharacter);

void PrintHexadecimalStringTestData(
    const char* inputHexString,
    int numericalValue);

int main()
{
    ParseHexadecimalStringTest("0x5567");
    ParseHexadecimalStringTest("0x1");
    ParseHexadecimalStringTest("0x2");
    ParseHexadecimalStringTest("0x10");
    ParseHexadecimalStringTest("0xFF");
    ParseHexadecimalStringTest("0xff");
    ParseHexadecimalStringTest("0xAFFFDE");
    ParseHexadecimalStringTest("0xafFfDe");
}

void ParseHexadecimalStringTest(const char* hexString)
{
    int numericalValue = ParseHexadecimalString(hexString);

    PrintHexadecimalStringTestData(hexString, numericalValue);
}

int ParseHexadecimalString(const char* hexString)
{
  const char* reader = GetPointerToLastCharacterOfHexString(&hexString);

  int numericalResult = 0;

  int hexadecimalOrderOfMagnitude = 1;

  for (; reader >= hexString; --reader)
  {
    char hexDigit = GetHexCharacterNumericalValue(*reader);

    numericalResult += hexadecimalOrderOfMagnitude * hexDigit;

    hexadecimalOrderOfMagnitude *= 16;
  }

  return numericalResult;
}

const char* GetPointerToLastCharacterOfHexString(
    const char** hexStringReference)
{
  // jumping ahead of the first two "0x" characters
  const char* readerPointer = (*hexStringReference) += 2;

  while (*(readerPointer + 1) != 0)
    ++readerPointer;

  return readerPointer;
}

char GetHexCharacterNumericalValue(char hexCharacter)
{
    if (IsHexCharacterWithinZeroAndNine(hexCharacter))
    {
      hexCharacter -= '0';
    }
    else
    {
      char subtractionModifier = 
        IsHexCharacterWithinAandFupperCase(hexCharacter) ? 'A' : 'a';

      //get the digit in range of 10 to 15
      hexCharacter = 10 + (hexCharacter - subtractionModifier);
    }

    return hexCharacter;
}

bool IsHexCharacterWithinZeroAndNine(char hexCharacter)
{
  return hexCharacter >= '0' & hexCharacter < '9';
}

bool IsHexCharacterWithinAandFupperCase(char hexCharacter)
{
  return hexCharacter >= 'A' && hexCharacter <= 'F';
}

void PrintHexadecimalStringTestData(
    const char* inputHexString,
    int numericalValueReceived)
{
    printf("input string is \"%s\".\nOutput is %x in hexadecimal, and %d in decimal.\n\n",
        inputHexString,
        numericalValueReceived,
        numericalValueReceived);
}

Woah! What's all this?! 😰😱

As you can see, the program is indeed longer, but when you compare them side by side, the newer version is more readable. BTW, I have to give a proper shout out to this awesome online compiler I used to test the re-worked code. Feel free to try it out!

Some notable changes I think are helping here:

An actual order of execution in the function declarations that also can give you a high-level description of the solution:

void ParseHexadecimalStringTest(const char* hexString);

int ParseHexadecimalString(const char* hexString);
const char* GetPointerToLastCharacterOfHexString(
    const char** hexStringReference);
char GetHexCharacterNumericalValue(char hexCharacter);
bool IsHexCharacterWithinZeroAndNine(char hexCharacter);
bool IsHexCharacterWithinAandFupperCase(char hexCharacter);

void PrintHexadecimalStringTestData(
    const char* inputHexString,
    int numericalValue);

int main()
{
   ...
}

See how the functions tell a story of what needs to happen in order to parse a string containing a hexadecimal number.

Better variable names. They're longer names, but they actually speak to what they represent:

int numericalResult = 0;  // used to be int value = 0;

int hexadecimalOrderOfMagnitude = 1; // used to be int increment = 1;

const char* readerPointer = (*hexStringReference) += 2;
// used to be const char* reader = (hexString += 2);

Yes more functions, but we broke down a larger function into smaller ones which are easier to debug:

bool IsHexCharacterWithinZeroAndNine(char hexCharacter)
{
  return hexCharacter >= '0' && hexCharacter < '9';
}

bool IsHexCharacterWithinAandFupperCase(char hexCharacter)
{
  return hexCharacter >= 'A' && hexCharacter <= 'F';
}

Well, what now? 🤔

I'm not an expert at all when it comes to writing code like it's suggested in this book, but I will keep practicing! I truly recommend to anyone interested in coding to at least read this just once, because it provides a lot of good advice. I'll definitely check out his other book called CleanArchitecture, and see what I learn there.

Please let me know what you think! 📝❔

Do you feel the 2nd version is actually more readable?
Any modifications you'd add to this to clean it even more?
Have you read this book? If so, what do you think?
Do you apply some or all the book's rules to your own code?

🎉🎉🎉Happy coding!!🎉🎉🎉

Esteban's Game Development Blog