Lab 05, or how to refactor code
Tags: opensource
Published: 2020-10-26 02:09:27

This week was all about tightening up our code. For me, that meant looking at what I wrote that added bloat, and what I wrote that broke modularity rules. Oh, I also cleared up some variable names.

This stuff is really important! Having messy code is something that's totally possible and honestly, pretty likely given how much time you can spend working on one project.

Let's start with my first and easier improvement, clearing up variables names.

```
fs.readFile(`${argv[2]}`, (err, fileContents) => {
    try {
        var linkList = generateLinkList(fileContents);
        linkList = Array.from(linkList);
    } catch (err) {
        console.log("The app has recieved a wrong filename.")
        console.log("Please enter a correct filename.")
        exit(1);
    }
    validateLinks(linkList)
})
```

This is an excerpt from my code after I changed a few variable names. For one, all my ``linkList`` variable names were one work ``linklist`` which made 1. it harder to read and 2. not in line with best variable naming practices. So I fixed that up. Secondly, the ``fs.readFile(`${argv[2]}`, (err, fileContents)`` was originally ``fs.readFile(`${argv[2]}`, (err, data)`` which I found to be quiet uninformative. I changed it because I frankly want to be able to know what I'm referencing in the future. There were a few other changes like this that I made, again just to make the code more readable and follow best practices.

 Second, I decided that I was doing 2 things in 1 function that I thought ought to be relegated to their own.

```
const isValid = (link) => {

    return new Promise((resolve) => {
        req.head(link, {
            timeout: 1500
        }, function (_, res) {
            if (!res) {
                if (!options.b && !options.g) {
                    console.log(chalk.gray(`[TIMEOUT] ${link}`));
                    process.exitCode = 2;
                }
                return resolve();
            }

            const status = res.statusCode;
            
            displayStatusCode(status, link);

            resolve();
        });
    })

}

const displayStatusCode = (code, link) => {
    if (code === 200) {
        if (!options.b) {
            console.log(chalk.green(`[200] GOOD ${link}`));
            if (process.exitCode != 1 && process.exitCode != 2) {
                process.exitCode = 0;
            }
        }
    } else if (code === 400 || code === 404) {
        if (!options.g) {
            console.log(chalk.red(`[${code}] BAD ${link}`));
            if (process.exitCode != 2)
                process.exitCode = 1;
        }
    } else {
        if (!options.b && !options.g) {
            console.log(chalk.gray(`[${code}] UNKNOWN ${link}`));
            process.exitCode = 2;
        }
    }
}
```

These 2 functions used to be 1 but after thinking about it, I felt that it was pretty important to separate the act of finding the status code and displaying the good or bad message into 2 different functions. It just makes sense. The code is more modular, another person can now edit the display function without messing with the ``isValid()`` function and vice versa. This change was common sense and was obvious from the moment I read about refactoring our code.

I think there's a possibility of further restructuring like taking the status code returned from the promise and sending _that_ to the ``displayStatusCode`` function instead of calling the function within the promise, but I'll think about that over the study week.

Finally, I decided to just cut down on some unnecessary lines of code.

```
fs.readFile(`${argv[2]}`, (err, fileContents) => {
    try {
        var linkList = generateLinkList(fileContents);
        linkList = Array.from(linkList);
    } catch (err) {
        console.log("The app has recieved a wrong filename.")
        console.log("Please enter a correct filename.")
        exit(1);
    }
    validateLinks(linkList)
})



const generateLinkList = (fileContents) => {
    let list;
    if (options.i) {
        list = getUrls(fileContents.toString(), {
            stripWWW: false,
            exclude: iFileArr
        });
    } else {
        list = getUrls(fileContents.toString(), {
            stripWWW: false
        }); 
    }
    return list;
}

const validateLinks = async (links) => {
    
    for (const link of links) {
        await isValid(link)
    }
    console.log("=========================");
    console.log("Exiting with exit code: " + process.exitCode);
}
```

So here we have 3 functions. the ``fs.readFile()`` goes through the file and sends the block of text to ``generateLinkList()`` to separate the links into a ``linkList`` of links which it then returns. Finally, the ``fs.readFile()`` sends that list to ``validateLinks()`` to validate them by calling ``isValid()`` which we looked at previously and finally, end the program.

Before this, there were actually 4 functions. The 4th was called ``separateLinks()`` and all it did was call ``generateLinkList()``. Pretty superfluous, no? So I got rid of it.

I think I may also want to find a way to remove global variables and separate these things into separate files. Again, more changes I can make over study week.

The interactive rebase for this code went well! I only wrote 3 commits, one for each change, and I didn't have anything extra to add at the last second. I simply squashed the [commits](https://github.com/fluentinstroll/Link-Checker/commit/8cff8712fb29eee9370a046e39eeed7f60ac3ed9) together and we on my merry way. I'm concerned that my ignore function isn't working properly - or at least when I change it to check for the ``-i`` argument in a different function it seems to break. Something to definitely take a look at because I think that portion of the app can use an overhaul. Changing my project's history was... odd. It felt wrong in a certain way. I understand changing commits to reflect actual changes (and not "...commit" like so many commits are) but I'm concerned that this can lead to exploitation. However, the maintainers or anyone else looking at a commit can clearly see what was changed on any given commit so I don't think there's much to worry about.

Anyway, that was my experience for Lab 5 and in general, refactoring my code. I think this is a process I should undergo more often in my projects if only to keep myself honest when a project gets even a few features larger. I'm going to definitely come back to this project and do some more refactoring over the next week.