Yesterday I had my code reviewed by someone else for the first time. As one of our assignments for the open source development course I'm taking, we had to review each other's code. For this exercise I paired up with Vinh, a friend of mine who also happens to be a pretty good programmer. We were tasked with testing and filing issues on each other's work for the command line tool each we've been working on in the course.

Vinh NhanFollow

/vinhyan

Asynchronous vs Synchronous Approach

We did our code reviews both synchronously over text and asynchronously over GitHub issues. I found the synchronous approach led to faster results as I could consult the author of the code on why they took a certain approach when writing their code, and immediately get a response. However, the asynchronous approach removes the need to find a fixed time in both peoples' schedules to get the work done.

Testing Vinh's program

Vinh created a command-line tool called barrierless that uses AI to translate text phrases into other languages, which I thought was a cool idea. When I began testing Vinh's program, it was still in early development, and so it didn't have a README yet (it does now, go check it out!).

bl-bot <input files> [-l <target language>]

A feature I really liked is the colorful output text which makes the user experience a little bit more pleasant - something I neglected in my own program in trying to model it after CLI tools like git.

I read the package.json file to find out how the program should be run, and when it immediately crashed I realized I forgot to add the API key as an environment variable. After adding my API key, the program ran without errors, although I did find an interesting quirk - the program defaults the output language to English, so if you didn't specify one, and the input was in English, it seemed to choose a language to translate to on its own - either randomly, or based on context from the input.

I opened a few other issues, mostly to do with improving code quality:

• A missing try/catch block around an async function call

Uncaught exception in index.js #7

uday-rana posted on Sep 12, 2024

index.js contains the following async function calls which are not wrapped in a try/catch block and may lead to an uncaught exception:

export async function main(text, targetLang) {
  const chatCompletion = await getGroqChatCompletion(text, targetLang);
  console.log(chatCompletion.choices[0]?.message?.content || '');
}

...

program
  ...
  .action(async (text, options) => {
    console.log(chalk.blue(`Translating  ${text}...`));

    await main(text, options.language);
  });

View on GitHub

• Some suggestions to make code easier to understand

Could simplify code #8

uday-rana posted on Sep 12, 2024

Some changes may be made to to the project make it easier to understand and work on:

• [x] Move Groq configuration above program initialization with commander
• [x] main() seems unnecessary since it contains two lines of code and there are more lines of code involved in creating and invoking the function than if it was omitted
• [ ] prompt.js seems unnecessary since it just contains a single function which places arguments into a template literal and returns them
• [x] Exporting main() and getGroqChatCompletion() seems unnecessary

View on GitHub

• Adding a comment to explain the use of both import and require statements

Add comments explaining mixed import/require #9

uday-rana posted on Sep 12, 2024

The project uses both ES6 import and CommonJS require due to the chalk module requiring the use of import and using import for package.json leading to an error. It would be helpful to add a comment explaining this.

View on GitHub

My turn

Next it was my turn to be reviewed. I wasn't sure what kind of issues would turn up, but Vinh ended up finding a bunch of issues I hadn't paid attention to:

• Adding npm link as another option for not having to prefix the tool with node in the instructions in the README

README.md does not include instruction to run `npm link` #2

vinhyan posted on Sep 12, 2024

The README.md file is missing instructions to run npm link, which is necessary for local development and testing of the CLI tool

View on GitHub

• Unnecessary command assignment using commander.js

`program.command("run")` is unnecessary since the CLI does not have subcommands #3

vinhyan posted on Sep 12, 2024

View on GitHub

• A variable name typo

Variable name typo #4

vinhyan posted on Sep 12, 2024

index.js line 31: typo in variable name: reponseStream

View on GitHub

Conclusion

I thought I'd done a pretty good job but goes to show there's always a bug that might've been missed or a feature that could be improved. It was great having a fresh pair of eyes scrutinize the code I wrote. For now, I fixed the typo and updated the README, but the other issues require testing and I plan on getting to those before I release version 0.1.