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 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.
Top comments (0)