3
\$\begingroup\$

Was told this code was 'weird' and didn't pass the coding test. I tried to split up to show the different levels of abstraction but maybe I went overboard? I told them this in the walk through of the code as things like console logging would be something more like save to database or something more complex so it's its own abstraction.

const https = require('https');
/*
 * Complete the function below.
 * Use console.log to print the result, you should not return from the function.
 * Base url: https://jsonmock.hackerrank.com/api/movies/search/?Title=
 */
async function getMovieTitles(substr) {

    // get first page results
    const results = await fetchMoviePage(substr, 1);
    let matches = results.data.map(extractMovieTitle);

    // base case - if we only have one page of results then we done here
    if (results.total_pages === 1) {
        return sortAndPrintResults(matches);
    }

    // fill an incremented integer array from page 2 to last page
    const pages = fillRange(2, results.total_pages);

    // for each page fetch data and add to results list
    for await (const page of pages) {
        const { data } = await fetchMoviePage(substr, page);
        matches = matches.concat( data.map(extractMovieTitle) );
    }

    sortAndPrintResults(matches);
}

/**
 * send sorted results to stdout
 */
function sortAndPrintResults(results){
    console.log(results.sort().join('\n'));
}

/**
 * fetches a page of movie results.
 */
async function fetchMoviePage(substr, page) {
    try {
        return await fetchMatchingMovies(substr, page);

    } catch (error) {
        console.log(`Error retrieving movies for ${substr} on page ${page}: ${error}`);
        return {data:[]};
    }
}

/**
 * Fills an array of incremented integers from start to end
 */
function fillRange(start, end) {
    return Array(end - start + 1).fill().map((item, index) => start + index);
};

/**
 * returns the movie title from a movie object
 */
function extractMovieTitle(movie) {
    return movie.Title;
}

/**
 * fetch a list of movies matching a search string
 */
function fetchMatchingMovies(searchString, page) {
    const queryString = `https://jsonmock.hackerrank.com/api/movies/search/?Title=${searchString}&page=${page}`;

    return new Promise((resolve, reject) => {

        https.get(queryString, res => {
            let data = '';
            res.on('data', chunk => data += chunk);
            res.on('end', () => resolve(JSON.parse(data)));

        }).on('error', e => reject(e));
    });
}
\$\endgroup\$
  • \$\begingroup\$ Is this the entirety of the code? It is neither a module nor does it actually execute anything like a standalone script. It's just a set of functions. \$\endgroup\$ – Mike Brant Mar 22 at 0:09
  • \$\begingroup\$ Were you really asked to fetch all results? What API endpoints were you provided? Fetching all results from a paginated API is weird. I would expect an endpoint to return me all results than to loop over and do multiple calls to a paginated API. This is probably part of the test, and just saying "ok, I'll do it" without question is probably the trick. But then this is just an assumption. We need more info. \$\endgroup\$ – Joseph Mar 22 at 17:15
1
\$\begingroup\$

I think it's cool that you fetch the pages in parallel.

A minor miss of the spec

The description says to not return from getMovieTitles, but the implementation does that.

I agree the early return makes sense when there is a single page of results. I would have extracted to a function the code that fetches results. Then the caller could simply call sortAndPrintResults on the returned array.

Things I find weird

return sortAndPrintResults(matches) is weird because sortAndPrintResults is void.

sortAndPrintResults joins the results before printing to console, which doubles the memory used by the results. It would have been better to call console.log in a loop.

I find the name fillRange misleading. Basically it creates a sequence of numbers. That's not what I would guess from the name.

It's a pity to construct an array of numbers just to loop over them.

These two issues could be fixed by implementing a range function as an async generator.

It's not 100% clear if the functions were given or only the first one. Many of them would be better inlined.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.