Skip to content

FIX-1: Slight refactor to make the error handling more robust #3

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Apr 25, 2024
Merged

FIX-1: Slight refactor to make the error handling more robust #3

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
51fe840
fix: slight refactor to make the error handling more robust on index.js
NickAnderegg Apr 19, 2024
8c306a6
fix: add callbacks to render function calls
NickAnderegg Apr 19, 2024
8eb8294
fix: slight refactor to handle data processing better
NickAnderegg Apr 19, 2024
35aaeb4
fix: remove unused code
NickAnderegg Apr 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 45 additions & 26 deletions index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
const express = require('express')
const weather = require('./src/weatherAPI.js')
const {requestCityFromIP, requestCoordsFromIP} = require('./src/geoipAPI.js')
const {geolocateFromIP} = require('./src/geoipAPI.js')
const anonymizeip = require('./src/anonymizeip.js')
var path = require('path');

Expand Down Expand Up @@ -28,31 +28,47 @@ api.get('/', (req, res) => {
ipAddr: anonymizeip(clientIP),
}

requestCityFromIP(clientIP).then((coords) => {
console.log('Coords obj:', coords)
const weatherResp = geolocateFromIP(clientIP).then((coords) => {
// If the geolocation is successful, format the name of the returned location,
// then call the weather API with the coordinates and timezone.

if ("regionName" in coords && "city" in coords && "country" in coords) {
renderValues.locationName = `${coords.city}, ${coords.regionName}, ${coords.country}`
} else if ("country" in coords) {
if ("city" in coords) {
renderValues.locationName = `${coords.city}, ${coords.country}`
} else if ("region" in coords) {
renderValues.locationName = `${coords.regionName}, ${coords.country}`
renderValues.locationName = coords.locationName
return weather(coords.lat, coords.lon, coords.timezone)
}).catch(() => {
// If the geolocation fails, default to Toronto, Ontario, Canada, then call
// the weather API with the coordinates and timezone.

renderValues.locationName = "Toronto, Ontario, Canada"
return weather("43.7", "-79.42", "America/Toronto")
})

weatherResp.then((weatherData) => {
// Once the weather API call is successful, render the index page with the
// template values specified in `renderValues`.

renderValues.forecast = weatherData
return res.render('index', renderValues, function (err, html) {
if (err) {
console.error("Error rendering index page:", err)
return res.status(500).send('An error occurred while rendering the page.')
} else {
renderValues.locationName = coords.country
return res.send(html)
}
} else if ("city" in coords) {
renderValues.locationName = coords.city
} else {
renderValues.locationName = coords.regionName
}

// By default, display the weather in Toronto.
return weather("43.7001", "-79.4163", "America/Toronto")
}).then((weatherData) => {
// renderValues.forecast = JSON.stringify(weatherData)
renderValues.forecast = weatherData
res.render('index', renderValues)
})
}).catch((e) => {
// If the weather API call fails, render the index page with the template
// and the limited values that are available.

console.error("Error in main route:", e)
res.render('index', renderValues, function (err, html) {
if (err) {
console.error("Error rendering index page:", err)
return res.status(500).send('An error occurred while rendering the page.')
} else {
return res.send(html)
}

})
})
})

Expand All @@ -61,19 +77,22 @@ api.get('/geolocate', (req, res) => {
// will send a request to `/geolocate` to get the estimated coordinates
// of the client's IP address. This will then return the coordinates to the
// client, which will use them to call the weather API as it normally would.
geoip(req.ip).then((coords) => {
res.json(coords)
})
geolocateFromIP(req.ip)
.then(coords => res.json(coords))
.catch(e => res.json({status: 'error', code: 500, message: e.message}))
})

api.get('/weather', (req, res) => {
const queryParams = req.query
if (!queryParams.lat || !queryParams.lon || !queryParams.timezone) {
res.status(400).send('Missing query parameters. All of the following are required: lat, lon, timezone')
return
}

weather(queryParams.lat, queryParams.lon, queryParams.timezone).then((weatherData) => {
res.json(weatherData)
}).catch((e) => {
res.json({status: 'error', code: 500, message: e.message})
})
})

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This pull request generally is well-structured, with clear comments. However, there are a few potential issues that could be improved.

  1. It seems that requestCityFromIP and requestCoordsFromIP from ./src/geoipAPI.js have been replaced by single function geolocateFromIP. If those functions were doing different things, this might cause problems. Review please if the geolocateFromIP function fully replaces both previous methods.

  2. The weather API call in api.get('/') uses coords.lat, coords.lon, and coords.timezone. If these properties don't exist or if the geolocation completely fails (maybe due to an unsuccessful geolocation because of no internet etc.), you would potentially execute erroneous weather API call, even though there is catch block below.

  3. Misleading comments: if ("city" in coords) and so on imply that these properties can be missing. Yet your code further down assumes they always exist during the weather API call.

  4. Ensure correct error handling: There's mixed use of Promise .then and .catch chains and try-catch blocks.

  5. In api.get('/weather'), there is a condition verifying query parameters. However, there isn't any form of validation for them. Latitudes and longitudes should fall within specific ranges, and timezone should typically match a certain format or be part of a list of standard time zones.

  6. Make sure the front-end implements appropriate behavior when 'status' comes back as 'error'. This back-end design expects the client to be able handle responses of different structures.

Expand Down
54 changes: 38 additions & 16 deletions src/geoipAPI.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,16 @@ function buildAPIRequest(ipAddr, fieldsVal) {

}

const defaultCoords = {
status: 'success',
country: 'Canada',
regionName: 'Ontario',
city: 'Toronto',
lat: '43.7',
lon: '-79.42',
timezone: 'America/Toronto',
}

function apiRequest(url) {

return new Promise((resolve, reject) => {
Expand All @@ -49,6 +59,26 @@ function apiRequest(url) {
res.on('end', () => {
try {
coordsResponse = JSON.parse(responseData)
// console.log("coordsResponse: ", coordsResponse)

let locationName = ''
if ("regionName" in coordsResponse && "city" in coordsResponse && "country" in coordsResponse) {
locationName = `${coordsResponse.city}, ${coordsResponse.regionName}, ${coordsResponse.country}`
} else if ("country" in coordsResponse) {
if ("city" in coordsResponse) {
locationName = `${coordsResponse.city}, ${coordsResponse.country}`
} else if ("regionName" in coordsResponse) {
locationName = `${coordsResponse.regionName}, ${coordsResponse.country}`
} else {
locationName = coordsResponse.country
}
} else if ("city" in coordsResponse) {
locationName = coordsResponse.city
} else {
locationName = coordsResponse.regionName
}

coordsResponse.locationName = locationName

// Because this sample app is used in live demos, we want to
// anonymize the coordinates returned to the client side.
Expand All @@ -58,13 +88,17 @@ function apiRequest(url) {
}

if (coordsResponse.status !== 'success') {
throw new Error('API request to `ip-api.com` failed.')
// If our query to the geolocation API endpoint fails,
// return the default coordinates object, which has the
// coordinates and timezone for Toronto, Ontario, Canada.
console.error('API request to `ip-api.com` did not succeed. Rejecting with default coordinates object (Toronto).')
reject(defaultCoords)
} else {
resolve(coordsResponse)
}
} catch (e) {
console.error(e.message)
reject(e)
reject(defaultCoords)
}
})
})
Expand All @@ -78,7 +112,7 @@ function apiRequest(url) {
})
}

function requestCoordsFromIP(ipAddr) {
function geolocateFromIP(ipAddr) {

// Validate input and throw an error if the input string isn't a valid IP address.
if (!ipAddrRegex.test(ipAddr)) {
Expand All @@ -89,20 +123,8 @@ function requestCoordsFromIP(ipAddr) {
return apiRequest(url)
}

function requestCityFromIP(ipAddr) {

// Validate input and throw an error if the input string isn't a valid IP address.
if (!ipAddrRegex.test(ipAddr)) {
throw new Error('Invalid IP address format')
}

const url = buildAPIRequest(ipAddr, 'status,country,regionName,city')
return apiRequest(url)
}



module.exports = {
requestCityFromIP,
requestCoordsFromIP,
geolocateFromIP,
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here are the potential issues I've identified in this patch:

  1. Typos/Missing Variables: There's no declaration or definition of ipAddrRegex, which is used to validate the IP address. If this variable is not defined elsewhere in the program, it will raise an error.

  2. Function Overhaul: The function requestCoordsFromIP has been commented out and replaced with geolocateFromIP. If there are other parts of the code that depend on calling requestCoordsFromIP, these calls will fail as the function is no longer exported from this module. Ensure to update these calls if necessary.

  3. Breaking change in Error Handling: The original code was rejecting the promise with the error when API request failed, or response parsing failed, thereby allowing the caller function to catch and handle these failures appropriately. However, the new code is rejecting the promise with defaultCoords object, which is a success scenario object. Therefore, the caller function might interpret this as a successful API call and proceed with incorrect coordinates. If you intend to provide defaults as a fallback mechanism for failure scenarios, you should resolve rather than reject the promise with default values.

  4. Input validation: Even though commenting out the requestCityFromIP function does not directly pose problems, it slightly reduces input validation granularity. Previously separate functions validated the IP addresses and handled different types of requests. Now, they are combined which reduces modularity.

  5. Lat and Lon values in defaultCoords: These values are strings ('43.7', '-79.42') instead of numbers (43.7, -79.42). If the rest of your program expects these to be numbers, this may cause inconsistencies or errors downstream.

Ensure you test these changes well, especially considering situations where things don't go to plan such as network errors, API down times, invalid responses etc.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a few potential issues with the patch:

  1. Rename of requestCoordsFromIP: The function requestCoordsFromIP has been renamed to geolocateFromIP, this will break any other parts of the code that still uses the original name, unless they are changed as well.

  2. Unused constant defaultCoords: The defaultCoords object is created, but it is never used when the API call fails. When an exception is thrown or rejected in the promise callback for the request, it rejects with this default coordinates object. This might not be what you want if the goal is to handle errors effectively. An error object containing helpful debug information should usually be returned.

  3. Changing error handling behavior: The previous version was rejecting the promise with the caught error, which allows the caller to take the appropriate actions based on the specifics of the error. The new version rejects with the defaultCoords object indiscriminately for all types of errors, which could mask a wide variety of problems and make debugging difficult, especially for callers that expect to receive an Error object upon rejection.

  4. Loss of functionality: The requestCityFromIP function was removed. If there are places in the existing codebase where this function is invoked, those will break after applying this patch.

  5. Latitude and Longitude data type inconsistency: In defaultCoords, lat and lon values are strings, while the API likely returns them as numbers. This difference can lead to unexpected behaviors down the line wherever these values are consumed in calculations.

Overall, it would be important to consider these potential issues before merging the pull request, possibly leaving comments requesting changes or further clarification.

6 changes: 3 additions & 3 deletions src/weatherAPI.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ function generateForecast(weatherData) {
dailyForecasts[day] = singleDayForecast
})

console.log(dailyForecasts)
// console.log(dailyForecasts)
return dailyForecasts
}

Expand Down Expand Up @@ -182,8 +182,8 @@ function getWeather(lat, lon, timezone) {
timezone: timezone
})

console.log('------------------------------\nQuery parameters:')
console.log(queryParams)
// console.log('------------------------------\nQuery parameters:')
// console.log(queryParams)

const queryString = Object.keys(queryParams).map(key => key + '=' + queryParams[key]).join('&')

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The provided patch primarily includes commenting out console.log lines. What this would achieve is the reduction of console output when these functions are called. If those logs were only there for debugging purposes and aren't needed in production code, it's legitimate to remove them.

However, this will not produce any logging output from those points in the codebase anymore. This might be a problem if the logging information was providing valuable insights about the application's state during runtime. For instance, in case of bugs or other issues in production, these logs could be very helpful for troubleshooting, even more so when no other robust monitoring or error tracking system is in place.

In terms of functionality, assuming the other parts of these functions don't have issues, this shouldn't introduce any bugs or exceptions as all it’s doing is commenting out log output. There aren't any typos, incorrect usage of variables or unhandled scenarios in the changed lines according to your description.

However, please note that without having access to the whole codebase, as well as knowledge of the specific context, requirements, and environment where the software will be deployed, pinpointing every possible risk or issue introduced by a patch will be somewhat limited.

Expand Down
2 changes: 1 addition & 1 deletion views/currentConditions.pug
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ h3 Weather Forecast
button(id="geolocate") Get Local Forecast
p(id="coords")
#forecast
if forecast.current
if forecast && forecast.current
if forecast.current.temperature_2m
#currentTemperature
span(id="currentTempVal") #{forecast.current.temperature_2m[0]}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The provided patch appears to be correct, as it adds a nil check for forecast before attempting to access its current attribute. This should help prevent any potential NoMethodError exceptions from occurring if forecast is nil.

However, based on the diff provided, there's no change in handling when forecast.current.temperature_2m is nil or absent. If temperature_2m is undefined or nil, then #{forecast.current.temperature_2m[0]} might throw an error similar to "undefined method `[0]' for nil:NilClass". It might be good to add a condition to handle this case.

There's also no check for forecast.current.temperature_2m being an empty array before trying to get its first element [0]. If temperature_2m is an empty array, #{forecast.current.temperature_2m[0]} would give us nil. Depending how you handle nil in your output, this could be a problem or not.

Remember that this review is limited to the diff provided and cannot account for issues elsewhere in the codebase.

Expand Down