So for our first lab we were to partner up with someone and go through the code review and testing process in GitHub by using Issues. As I said in my previous post, I wanted to practice my C++ to get back into it, so of course I wanted to see if any of my classmates were doing their SSG in C++ as well. I dropped a quick message in our class Slack channel to see who I could find. Luckily for me, my classmate Gus was also doing his project in C++!
This was interesting to do a code review and testing of someone else's code. Seeing how someone else do the same project but with different lines of thinking or logic is really cool. Even though this is a simple project there were certain features or methods that I didn't really think of doing like he did.
For example, in Gus' code he made a validation block that would check if there were arguments, but if there wasn't his program would prompt the user to re-enter the arguments instead of exiting with a message warning the user or something of that like. What I did was just display my help information again if there were no arguments. But I like his method much more.
It was also very nice to have someone else take a look at your code, I made a few silly mistakes that I would probably never have caught because it made sense in my head, such as my versioning being wrong. (Issue).
When going through Gus' code I found a few errors, albeit he was still in progress of finishing the program. As I went through his code and tested using the same text file I tested mine with, I found an issue where entering: -i "Silver Blaze.txt" as the arguments caused an error. [Link to issue].(https://github.com/gusmccallum/GAS-ssg/issues/6)
When I looked through his code I realized that he had code that would concatenate a text file that had spaces in the name. So if the argument was -i Silver Blaze.txt it would work fine. So he was missing a handler for text files where there were no spaces or were encapsulated in quotes.
Another issue I found was with his validation block that I mentioned earlier. While it was nice that it would prompt the user again, it was declared after he declared a std::string argString = argv[1]; variable. Which means if the user didn't input an argument the program would crash before getting to the validation block since argv[1] wouldn't exist. Link
Gus was able to find a few issues with my code that were simple to fix such as the versioning I mentioned earlier. However, he also filed an issue with my variable declarations for the filename:
//Creating Output file and inital html
string base_filename = path.substr(path.find_last_of("/\\") + 1);
string::size_type const p(base_filename.find_last_of('.'));
string file_without_extension = base_filename.substr(0, p);
string newHTML = "./dist/" + file_without_extension + ".html";
He said that this had extraneous variable declarations, which I honestly agreed with. But this one wasn't really as easy to "fix" because of how the logic worked. I needed multiple variables because substr() needs the length of the file name which is harder to get from the full file path instead of the file name + extension. I could have put all the functions into one line but that felt too messy and confusing to me, so I decided to leave this one as is because it made more sense to me and felt more readable. You can find the issue here and more of my reasoning if you are interested. Also if you can think of a cleaner way to do this please feel free to share or add to the issue!
All in all, this was a interesting learning experience. Even though this is a fairly simple program there are many approaches different people take, and there are many things some people can catch while others might not. This really shows the power of open source, with just two people we got very different perspectives and approaches to how we write a program/review code. Just imagining this but multiplied by hundreds of thousands is showing me how incredible open source is.
If you are interested in checking out either of mine or Gus' repos feel free to take a look!
GAS-ssg by Gus McCallum
LENNAH by Joshua Li
Take care!
Top comments (0)