melissaappolis / command-line-node-app Goto Github PK

Hey Melissa,

Firstly thanks for completing the assessment!

I'd like to comment on it, and then ask you to please make some changes. This type of code-feedback-review is pretty much how it goes in the real-world too :)

Regarding structure - I think there was a baseline assumption that was misunderstood.

Firstly, the remote API is the gateway to the database where all the user and invoice records are stored.

The first assumption is that the script (locally on the users machine) does not have context of what users and data is available before requesting (since the data is not stored locally on the users machine).
Consequently hardcoding the email address and the invoice ID break this paradigm a bit.

The downside to doing so, is that what if we had to search a different user's email? It's not practical to add another -if statement to do so. We have 35000 active customers, so the script should be more generic in that it only accepts an email, and then will query the API for the result and/or validity of that user.

Also - the user_id (used to fetch the invoice) will not be known to the client or the agent using the script - this information has to (and can only) come from the first API request.

Please refactor to:

1. accept only an email address (thanks for being creative and adding user_id support too!)
2. call the /client API for the client details
3. user the user_id returned from step 2 to then call the /invoices/:user_id API
4. using the data returned from 3, print the output for the profile.
5. print out a line for each invoice returned in the invoices payload.

Some javascript-stylistic points:
1.
let argv = yargs.argv; - please use const over let as a default. To the next developer reading your code, if they let, it suggests that this will be reassigned a value at a later stage, and add more to the plate to think about.

let email = argv.email;
let id = argv.id;

Highly recommend you look into javascript's object and array destructuring.
The above can be written as:

const { email, id } = argv

let id = argv.id; - variable names go a really long way and are a subtle art, but it helps to frequently ask whether a variable name accurately reflects what it represents.
id is fairly generic. invoice id? account id? user id? When using generic terms like id, or data, it's good practice to give context to it in its name. userId is appropriate here. (though the script should only accept email for now :) )

axios.all([
        axios.get(`https://whmcstest.proxy.beeceptor.com/invoices/${id}`),
        axios.get(`https://whmcstest.proxy.beeceptor.com/client/[email protected]`)
    ])

Similar to the const vs let comment above, it better to use native javascript constructs unless you have a specific reason not to. In this instance, you can replace axios.all with Javascript's Promise.all method. It has the same syntax and does the same thing, however by using axios.all - the next developer has to also figure out why you used this, and what the specific reason was.

Promise.all([
        axios.get(`https://whmcstest.proxy.beeceptor.com/invoices/${id}`),
        axios.get(`https://whmcstest.proxy.beeceptor.com/client/[email protected]`)
    ])

Array destructuring lets us rename specific items of the array as variables.
Your axios.all returns the two promises in the first argument as the array [firstRequest, secondRequest]. We generally don't want to continuously reference res[0] and res[1] all the time, or long object chains like object.data repeatedly. We should take this opportunity to destucture commonly referenced items with a meaningful variable name.

e.g res[0].data doesn't mean much, but it's actually the userProfile
res[1].data also doesn't mean much, but it's actually userInvoices

This would be used like so:

.then(res => {
        const [userInvoices, userProfile] = res

        console.log("=========================================================="
                    +'\n'+userProfile.fullname + " ("+ userProfile.email +")"
                    +'\n'+"ID: " + userProfile.id
                    +'\n'+"Status: " + userProfile.status
                    +'\n'+"Credit: " + userProfile.currency_code + " " + userProfile.credit
                    +'\n'+"Last Login:" + userProfile.lastlogin.replace(/Date:|<br>/g, ""));

        console.log('\n'+"Orders"
                    +'\n'+"# - InvoiceId - Status - Date - Due_Date - Total"
                    +'\n'+"1. "  + userInvoices.invoices.invoice[0].id 
                     + " | " + userInvoices.invoices.invoice[0].status
                     + " | " + userInvoices.invoices.invoice[0].date
                     + " | " + userInvoices.invoices.invoice[0].duedate
                     + " | " + userInvoices.invoices.invoice[0].currencycode + " " + userInvoices.invoices.invoice[0].total);
    });

Loop over invoices

You've harded coded references to the first item in the invoices array. Please refactor this to print out a line for each invoice in the invoices array.

7. Template literals
   Another great javascript feature for printing long strings in specific formats is to use template literal strings.
   You simply start and end the string with backticks (), and interpolate javascript values in with the ${ someVariable }` syntax.
   It will respect all formatting and line breaks. This helps get away from the + "" pattern that makes things challenging to read.

For example:

const userProfileOutput = 
`================================
${fullName} (${email})
ID: ${id}
Status: ${status}
`

Regex for last login

Firstly good job using the regex. Exactly what I was looking for. They're always fun to play with :)
Just a note on long-term maintenance, instead of removing the parts of the string that we don't want, it may have been better to instead use the regex to select the format/pattern that we want instead.

If the string format changes and suddenly has other weird characters, we would have to update our regex every time.
If we're only looking for a date in a specific format, then if we find that format, we can match it and use it, regardless of any other weird characters in the future.

e.g if we know a valid date will always look like this:
02/02/2021 09:25

We should then write a regex to match that, so we'll only ever have to update our code if the date-format changes.

String.match will then return a match value (or none if no match):
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/match

You don't have to refactor this point if you don't want. Regex's can be a pain :)