Function to find user's region based on GPS coordinates

Question

I've made an algorithm/ function to get a user's region based on their GPS coordinates. It primarily looks for big cites and counties for the region and the state for the outer region eg. orange county, Texas.

This wasn't simple as GPS / maps isn't a perfect science. Sometimes I might only get a town and a country to work with. Other times I have 8 levels of their location from house number to country.

I've managed to do this really well, only returning unknown when they are in the ocean. But there are a lot of ifs and else ifs etc. and it would be nice to tidy it up a bit.

function getRegion(latitude, longitude){
    return new Promise(function(resolve, reject){   
        geocoder.geocode({'latLng': new google.maps.LatLng(latitude, longitude)}, function (results, status) {

            var region = 'Unknown'; //Set default reigion to unknown
            var outterRegion; //Declare an outta reigion as some 'inner' regions have the same name

            if (status == google.maps.GeocoderStatus.OK) {

                var details = results[0].address_components; //Returns the users location

                var gotOptimal = false; //If the optimal reigion name is there, dont search for it anymore
                var backupRegion = null; //Look for a backup reigion name incase of eg. Utah, Utah. Changes to xcounty, Utah

                for (var i = 0; i< details.length; i++) { //Each level of the users address from smallest (street), to largest (country)

                    var invalidAreaType = false; //I dont want someones reigion to be their country, postcode, or a route.
                    for (var j=0; j<details[i].types.length;j++) { //Each level has a type eg country, neibourhood etc
                        invalidAreaType = ((details[i].types[j] == 'country') || (details[i].types[j] == 'postal_code') || (details[i].types[j] == 'route'));
                        break;
                    }

                    if (!invalidAreaType){ //If the current level of the users location is a type we dont want, skip it
                        for (var j=0; j<details[i].types.length;j++) {
                            if (details[i].types[j] == 'administrative_area_level_2' && (!parseInt(details[i].long_name))) {
                                region = details[i].long_name; //This is the prime reigion, Big citys and countys
                                gotOptimal = true;
                            } else if (details[i].types[j] == 'administrative_area_level_1' && (!parseInt(details[i].long_name))) {
                                outterRegion = details[i].long_name; //Level 1 up from reigion, eg state
                                region = (!gotOptimal) ? details[i].long_name : region; //If optimal isnt found yet, save it for now
                            } else if (details[i].types[j] == 'political' && (!parseInt(details[i].long_name))) {
                                region = (!gotOptimal) ? details[i].long_name : region; //big else if statment from most optimal, to least
                            } else if (details[i].types[j] == 'postal_town' && (!parseInt(details[i].long_name))) {
                                region = (!gotOptimal) ? details[i].long_name : region; 
                                backupRegion = (!gotOptimal) ? details[i].long_name : backupRegion; 
                            } else if (details[i].types[j] == 'locality' && (!parseInt(details[i].long_name))) {
                                region = (!gotOptimal) ? details[i].long_name : region; 
                                backupRegion = (!gotOptimal) ? details[i].long_name : backupRegion; 
                            } else if (details[i].types[j] == 'sublocality' && (!parseInt(details[i].long_name))) {
                                region = (!gotOptimal) ? details[i].long_name : region; 
                                backupRegion = (!gotOptimal) ? details[i].long_name : backupRegion; 
                            } else if (details[i].types[j] == 'neighborhood' && (!parseInt(details[i].long_name))) {
                                region = (!gotOptimal) ? details[i].long_name : region; 
                                backupRegion = (!gotOptimal) ? details[i].long_name : backupRegion; 
                            }
                        }
                    }
                }

                //This is used for cases when very little info is returned or a number region eg. '3'
                if (region == 'Unknown' && details.length > 1 && !invalidAreaType) {
                    region = details[Math.floor(details.length / 2)].long_name;
                    region = (parseInt(region)) ? details[Math.floor(details.length / 2) - 1].long_name : region;
                    region = (parseInt(region)) ? details[Math.floor(details.length / 2) + 1].long_name : region;
                    region = (parseInt(region)) ? 'Unknown' : region;
                    //Ive had number regions returned before, this is to rule it out as a reigion called '3' is too broad
                }

                //eg Utah, Utah  --> change the region to a county or town
                region = (region == outterRegion) ? backupRegion : region;

                //Finalise and return
                userLocation.region = region;
                userLocation.outterRegion = outterRegion;

                region = (outterRegion && (!parseInt(outterRegion))) ? region + ', ' + outterRegion : region;
            }   
            resolve(region);
        });

    })
}

I've commented a lot of it to give you an idea of why its there. You can see why I would ideally like this to be a bit shorter. This uses the google maps API also.

Answer 1

The callback supplied to geocode has a lot of code in it. This becomes difficult to test independently of testing the rest of getRegion but the more immediate pressing issue is that it makes things harder to visually parse.

A nice fix to kick things off would be to extract the call to geocode to a separate function that yields a promise and then pass the result of that promise to the function that handles the business logic of your code, like this:

function getLatitudeAndLongitude(latitude, longitude) {
  return new Promise((resolve, reject) => {
    geocoder.geocode({
      latLng: new google.maps.LatLng(latitude, longitude)
    }, (results, status) => {
      if (status === google.maps.GeocoderStatus.OK) {
        resolve(results);
      }

      // Otherwise you'll want to reject in a way that makes sense to you.
      reject();
    }
  })
}

I also changed the following about the above code:

• Changed function expressions to arrow functions. This is mostly for terseness' sake.
• Removed the quotation marks around latLng - It's not necessary to do this in object literals unless there is a symbol in the object literal that would cause a syntax error (like a space).
• Changed the returned Promise to only resolve with the results if the status is OK and fail otherwise.

Now let's look at the remainder of your code. The first thing we should do is place this in a function:

function getRegion(results) {
  const details = results[0].address_components; //Returns the users location
  const invalidAreaTypes = ["country", "postal_code", "route"];
  const typesWithBackupRegion = [
    "postal_town",
    "locality",
    "sublocality",
    "neighborhood"
  ];
  const initialState = {
    region: null,
    gotOptimal: false,
    backupRegion: null,
    outerRegion: null
  };

  let {
    region,
    outerRegion,
    backupRegion
  } = details.reduce((accumulator, entry) => {
    return entry.types.reduce((accumulator, areaType) => {
      const longName = entry.long_name;
      if (invalidAreaTypes.includes(areaType) || parseInt(longName)) {
        return { ...accumulator, invalidAreaType: true };
      }

      const { gotOptimal } = accumulator;
      const useBackupRegion =
        typesWithBackupRegion.includes(areaType) && gotOptimal;
      const isOptimalName = areaType === "administrative_area_level_2";
      const isOuterRegion = areaType === "administrative_area_level_1";

      return {
        region: gotOptimal === false ? longName : accumulator.region,
        outerRegion: isOuterRegion ? longName : accumulator.outerRegion,
        gotOptimal: isOptimalName || accumulator.gotOptimal,
        backupRegion: useBackupRegion ? longName : accumulator.backupRegion
      };
    }, accumulator);
  }, initialState);

  //This is used for cases when very little info is returned or a number region eg. '3'
  if (region === null && details.length > 1) {
    region = details[Math.floor(details.length / 2)].long_name;
    region = parseInt(region)
      ? details[Math.floor(details.length / 2) - 1].long_name
      : region;
    region = parseInt(region)
      ? details[Math.floor(details.length / 2) + 1].long_name
      : region;
    region = parseInt(region) ? null : region;
    //Ive had number regions returned before, this is to rule it out as a reigion called '3' is too broad
  }

  //eg Utah, Utah  --> change the region to a county or town
  region = region === outerRegion ? backupRegion : region;

  region =
    outerRegion && !parseInt(outerRegion)
      ? region + ", " + outerRegion
      : region;

  return region;
}

Above:

• I changed any uses of var to let or const in case they were not mutated. It's good practice to use these keywords as they communicate your intent more clearly than var.
• I moved your comparison of invalidAreaType to within the loop you had, which prevents iterating the area types multiple times.
• Every branch of your if statements when checking area types requires that parseInt(entry.long_name) is truthy, so I put that at the front of the loop.
• Your large numbers of if statements can be simplified to be a reducer function. This has the added benefit of not having to track so much state, which can be confusing.
• Several of your if statements regarding sublocality, neighbourhood etc can be condensed into one, since they all do the same thing.
• I changed your outer numeric for loop into a reduce function, since that entire segment of code is just determining which region to use. As far as I can tell my reduce function is faithful to the original code since your original code iterates through all the details and types and then returns only a single region.
• I changed instances of == to ===. == is loose equality and is almost never what you want.
• I changed "Unknown" to null. Use null to signify absence of a value. You should let your display layer display "Unknown" when it encounters a null response from getRegion.
• I'm not sure where userLocation came from, but it's not defined in your function so I've removed it. It looks like it used two regions from the function but only one was returned. You should consider returning those two regions instead and having the caller set userLocation.

I'm not sure how to refactor the rest of your code, particularly the bit that handles when there's a lack of data, but this is what I came up with. This doesn't reduce the LOC very much (although you would score additional savings by simplifying the logic after the bit I refactored), however it definitely has a readability improvement - plus, the newly extracted getRegion function is more testable and can be tested in isolation now.

It would be used like this:

getLatitudeAndLongitude(lat, long)
  .then(getRegion);

Key takeaways

Embrace the S.R.P. Your function should do one thing and one thing well.

You could argue that your original getRegion function does only one thing - getting the region. That's true, but your function can be broken down into several logical units that each do 'one thing':

• Get information about a particular long/lat from Google Maps
• Find the most relevant Region information to display to the user
• Format the region information for display

Use ES6. It's been out for a long time now; every evergreen browser at least supports const, let and arrow functions. If your browser supports Promise, it supports these things. They communicate intent and increase terseness.

If you need to reduce an Array into a single value, instead of using nested for loops with multiple 'tracking' variables, use reduce. It's a lot easier to read and reason about.

If you make multiple calls to a function with the same value, extract it to a common variable unless that value changes (like your region code).

Answer 2

I agree with the great feedback in Dan's answer. I like using functional programming wherever possible (and recommend these exercises for anyone who wants to work with FP in JS more), though one should bear in mind that there will be performance impacts, since each iteration has an extra function added to the callstack.

One thing I noticed with the original code is that there are many calls to parseInt(), like this:

} else if (details[i].types[j] == 'political' && (!parseInt(details[i].long_name))) {

And I don't see any radix passed to those calls. Note the documentation from MDN about that parameter:

Always specify this parameter to eliminate reader confusion and to guarantee predictable behavior. Different implementations produce different results when a radix is not specified, usually defaulting the value to 10.¹

It is wise to pass the radix- perhaps defaulting to 10:

} else if (details[i].types[j] == 'political' && (!parseInt(details[i].long_name, 10))) {

Perhaps it would never be the case that the Google Maps API would return a value with a leading zero (e.g. 022), but if there was, then it wouldn't be interpreted as the octal value (I.e. Decimal number 18).

Another thing I noticed is that a few for loops are used. Consider using for...of loops to simplify the syntax and logic - e.g. don't need to increment a counter variable like i just to use it to dereference an object/array.

---

¹_{https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/parseInt#Parameters}