Take postcodes and get the Lat long information from them

Question

I had an interview task which was the final stage, however, the feedback I got back for my solution was that it lacked effort and structure. I would be grateful to know how my solution could be improved. I used postcode.io to get the latitude and longitude from the post codes.

Here is the Description:

Brief: Create a server application that allows a user to input one or more postcodes, and then returns location-specific information based on the postcode entered.

Requirements: At least one of the external APIs made available to you should be used to retrieve the information that your API returns. You are not limited to using only one, and are free to use as many of them as you like (inc. any other external resources) in order to provide useful data.

Based on the above I made the following program.

app.js

import * as express from "express";
import * as axios from "axios";

const app = express();
app.use(express.json());

app.get("/", (req, res) => {
  res.send("Server is up and running 🤓");
});

app.get("/postcode/:postcode", async (req, res) => {
  const postcode = req.params.postcode;
  await getLatlonFromPostcode(postcode).then((data) => {
    res.send(data);
  });
});

app.post("/postcodes", async (req, res) => {
  const postCodes = req.body.postcodes;
  const result = await getAdressForMultiplePostCodes(postCodes).then(
    (response) => {
      return returnLatLonObj(response.result);
    }
  );
  res.json(result);
});

//Return the lat and lon for a single postcode
export async function getLatlonFromPostcode(postcode) {
  try {
    const res = await axios
      .get(`https://api.postcodes.io/postcodes/${postcode}`)
      .then((response) => {
        const lat = response.data.result.latitude.toString();
        const lon = response.data.result.longitude.toString();
        return `${lat}, ${lon}`;
      });
    return res;
  } catch (error) {
    console.error(error);
  }
}

//Return array of addresses from Postcode array
export async function getAdressForMultiplePostCodes(postcodes) {
  try {
    const res = await axios
      .post("https://api.postcodes.io/postcodes", {
        postcodes,
      })
      .then((response) => {
        return response.data;
      });
    return res;
  } catch (error) {
    console.error(error);
  }
}

//Grab Latitude and Longitude and convert Array to object
function returnLatLonObj(arr) {
  return arr.reduce((prev, curr) => {
    prev[curr.result.latitude] = curr.result.longitude;
    return prev;
  }, {});
}


export default app;

This is my index.js

import app from "./app";

app.listen(3000, () => {
    console.log("server listening on port 3000");
})

This was my test file, which I used jest for.

import { getLatlonFromPostcode } from "../app";

describe("getLatlonFromPostcode", () => {
  test("it should return latlon for given postcode", async () => {
    const latlon = await getLatlonFromPostcode("ox495nu");
    expect(latlon).toEqual("51.655929, -1.069752");
  });
});

describe("getLatlonFromPostcode in uppercase", () => {
  test("it should return latlon for given postcode", async () => {
    const latlon = await getLatlonFromPostcode("OX49 5NU");
    expect(latlon).toEqual("51.655929, -1.069752");
  });
});

describe("getLatlonFromPostcode with spaces between the postcode", () => {
  test("it should return latlon for given postcode", async () => {
    const latlon = await getLatlonFromPostcode("OX49 5NU");
    expect(latlon).toEqual("51.655929, -1.069752");
  });
});

Answer 1

The code could be improved with a few tweaks, but it's not bad, and the overall structure looks just fine to me.

Error handling is in the wrong place Some of your route handler are async, and inside them you await. Whenever you have a Promise, make sure to handle possible errors properly. Although the functions outside of your router handlers handle errors - they'll never return Promises that reject - it's a bit of a code smell to rely on the called functions to never reject; if you accidentally use a function which does return a Promise that rejects, Express won't be able to handle the rejection inside the route handler, and you'll get an UnhandledRejection, which will either terminate the server process completely, or display a warning message that unhandled rejections are deprecated.

When a Promise rejects, don't just log the error, but also send a response to the user indicating that there was an unexpected error. The error handling should be in the route handlers, not in the functions it's calling.

async shines only with nested Promises On a similar note, async functions and the try { await ... } catch(e) {} they require for error handling is a reasonable amount of boilerplate. If all you have is a single Promise, consider just using .then and .catch instead.

A rule of thumb I like: would an async function use at least two awaits? Then it's probably worth it. Otherwise, it might not be.

For example, /postcode/:postcode could be:

app.get("/postcode/:postcode", (req, res) => {
  // Destructure for conciseness:
  const { postcode } = req.params;
  // Changed name from Latlon to LatLon - "Latlon" is not a single word
  // so use proper CamelCase instead
  getLatLonFromPostcode(postcode)
    .then((data) => {
      res.send(data);
    })
    .catch((error) => {
      console.error(error);
      res.status(500).json({ error: error.message });
    });
});

and getLatLonFromPostcode can be:

//Return the lat and lon for a single postcode
export function getLatLonFromPostcode(postcode) {
  return axios.get(`https://api.postcodes.io/postcodes/${postcode}`)
    .then((response) => {
        const { latitude, longitude } = response.data;
        return `${latitude}, ${longitude}`;
      });
}

letting the caller handle possible errors. Also note the use of destructuring for conciseness, and since string interpolation with ${} will automatically coerce the interpolated expressions to strings, there's no need to call toString explicitly.

You can follow the same pattern for the /postcodes route.

Spelling Misspelled variables and properties, or inconsistent capitalization, are a frequent source of bugs. Change getAdressForMultiplePostCodes to getAddressForMultiplePostcodes (Since postcodes is one word in the request, it should also probably appear as one word here - if you use PostCodes, the inconsistent capitalization could make things harder than they should be)

Groupby into object with Object.fromEntries Your returnLatLonObj can be improved:

• First, .reduce arguably isn't very appropriate when the accumulator is the same object across all iterations. It works, but it can be a bit verbose and confusing. See this video by V8 developers on the subject.
• Second, if you want to turn an array into an object whose keys are properties of the array items, you can do it quite concisely with Object.fromEntries:

function returnLatLonObj(arr) {
  return Object.fromEntries(
    arr.map(({ result: { latitude, longitude }) => [latitude, longitude])
  );
}

Answer 2

Test case structure

It doesn't seem like the code totally "lacked effort and structure" but the structure, at least of the test cases, could be better. Since all three describe() blocks contain a test that calls getLatlonFromPostcode so there could be a single describe() block with three it() blocks to test the various aspects of how the function should handle given postcodes.

Utilizing await

One of the main benefits of using await is to eliminate the need for callbacks to .then(). Some code utilizes this benefit e.g. the test functions but the methods in app.js do not - e.g.

await getLatlonFromPostcode(postcode).then((data) => {
  res.send(data);
});

Ideally, the res.send() call would be on a sequential line instead of in a .then() callback function:

const data = await getLatlonFromPostcode(postcode);
res.send(data);

Or even simpler:

res.send(await getLatlonFromPostcode(postcode));

Error handling

As an interviewer I would be checking to see how well the code handles edge cases. Obviously with promises comes rejection. The first couple methods don't appear to handle rejected promises at all.

Import Star

The two import statements import everything from the modules being imported. In the Airbnb style guide, it recommends no wildcards:

Why? This makes sure you have a single default export.¹

The import for axios could be simplified using destructuring assignment:

import { get, post} from "axios";

That way the code can call get() and post() without needing to reference axios, and the other items exported from axios that are not needed are not imported.