Skip to content
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

Jump to bottom

REFACTOR-1: Improve flow and add explanatory comments #10

Merged
merged 2 commits into from
Apr 25, 2024
Merged

REFACTOR-1: Improve flow and add explanatory comments #10

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
08948d3
fix: make suggested improvements to `client.js` and add comments
NickAnderegg Apr 23, 2024
d42bac1
refactor: make flow cleaner and add explanatory comments
NickAnderegg Apr 23, 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
6 changes: 6 additions & 0 deletions index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,12 @@ api.get('/', (req, res) => {
ipAddr: anonymizeip(clientIP),
}

// For the initial page load on the main route, the server will attempt to
// geolocate the user based on their IP address. If the geolocation is
// successful, the server will then call the weather API with the coordinates
// and timezone obtained from the geolocation API. If the geolocation fails,
// the server will default to Toronto, Ontario, Canada, and call the weather
// API with the coordinates and timezone for Toronto.
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.
Copy link

Choose a reason for hiding this comment

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

The main difficulty with this pull request is that it includes comments without any corresponding code implementing the described functionality. While the comment suggests changes to system behavior in case of geolocation success or failure, there's no actual code provided to carry out those actions.

Here are a few potential issues from this promise-based implementation:

  • There is no error handling for the promise returned by geolocateFromIP(clientIP). What happens if this promise is rejected? An error could occur due to network issues or other unforeseen circumstances. To handle such scenarios, consider adding a .catch(err) clause.
  • The server defaults to Toronto according to the comment, but there's no actual code provided that does so.
  • There are no variables or values representing "coordinates" or "timezone", despite these terms being used in the comments.
  • Finally, the weather data fetched doesn't seem to be sent back to the client or used anywhere else in the given snippet which would mean that the API call is unnecessary.

Please note: without complete input including surrounding (preceding/succeeding) lines of code and use case context, detailed feedback is challenging. If precursory steps like calls to APIs, error handling mechanisms, variable declarations, etc., happen prior to this code segment, the concerns might be already handled. However, based on given information, above points outline potential issues to address.

Expand Down
101 changes: 91 additions & 10 deletions public/js/client.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,52 +1,133 @@
/** Take a decimal coordinate (representing either latitude or longitude) and
* return an anonymized version of it, as this app exists to be used for demos,
* rather than for production purposes.
* @returns {String} The anonymized coordinate, truncated to two decimal places.
* @param {Number} coord The coordinate value to anonymize.
*/
function anonymizeCoordinate(coord) {

// We want to randomly add or substract a small value to the coordinate to
// anonymize it. To decide whether to add or subtract, we generate a random
// number, and if it is greater than 0.5, we add; otherwise, we subtract.
const addOrSubtract = Math.random() > 0.5 ? 1 : -1

// The actual random variation is a value between 0 and 0.5, which will be
// multiplied by the `addOrSubtract` value to determine the direction of the
// variation.
const randomVariation = Math.random() * 0.5 * addOrSubtract
return (parseFloat(coord) + randomVariation).toFixed(2)

// Return the anonymized coordinate as a string with two decimal places,
// which reduces the specificity of the coordinate and adds to the
// anonymization effect.
const anonymizedCoord = (parseFloat(coord) + randomVariation).toFixed(2)
return anonymizedCoord
}

/** Utility function to get the local timezone of the user's browser.
* @returns {String} The local timezone of the user's browser.
* */
function getLocalTimezone() {
return new Intl.DateTimeFormat().resolvedOptions().timeZone
}

/**
* Fetch the user's geolocation based on their IP address.
*
* This function calls the `/geolocate` endpoint on the server, which will
* call a third-party API to get the user's geolocation based on their IP address.
* @returns {Object} An object containing the user's geolocation data.
*/
async function getGeolocationFromIP() {
const response = await fetch('/geolocate')
return response.json()
try {
const response = await fetch('/geolocate')
return response.json()
} catch (e) {
console.error('Error getting geolocation from IP:', e)
}
}

/** Update the text on the page to show the user's location in a human-readable
* format.
*
* This is done by updating the inner HTML of the `#coords` element on the page.
* @param {string} lat - The latitude of the user's location.
* @param {string} lon - The longitude of the user's location.
* */
function updateLocationText(lat, lon) {
const latString = `${Math.abs(lat)}° ${lat > 0 ? 'N' : 'S'}`;
const lonString = `${Math.abs(lon)}° ${lon > 0 ? 'E' : 'W'}`;

// Format the latitude and longitude values to two decimal places, and add
// the appropriate compass direction (N, S, E, W) based on the sign of the
// coordinate.
const latString = `${parseFloat(Math.abs(lat)).toFixed(2)}° ${lat > 0 ? 'N' : 'S'}`;
const lonString = `${parseFloat(Math.abs(lon)).toFixed(2)}° ${lon > 0 ? 'E' : 'W'}`;

// Update the text on the page to show the user's location in a human-readable
// format, with compass directions and degree symbols.
document.querySelector('#coords').innerHTML = `Location: ${latString}, ${lonString}`
}

// function updateWeatherData

/**
* Attempt to geolocate the user using the browser's geolocation API after they
* click the "Geolocate" button on the page. If the browser does not support
* geolocation, or if the user denies the request, the function will fall back
* to getting the location from the user's IP address.
*/
function geolocate() {

console.log(`Timezone: ${localTimezone}`)
// Create a constant to reference the `#coords` element on the page, which
// will be updated to display the user's location.
const coordsOutput = document.querySelector('#coords')

/**
* Callback function for when the browser successfully retrieves the user's
* geolocation.
* @param {GeolocationPosition} position - The geolocation data returned by the browser.
*/
function success(position) {
console.log('Device geolocation successful')
console.log('Device geolocation successful!')
const lat = anonymizeCoordinate(position.coords.latitude)
const lon = anonymizeCoordinate(position.coords.longitude)
updateLocationText(lat, lon)
}

/**
* Callback function for when the browser fails to retrieve the user's
* geolocation. This function will attempt to get the user's location from
* their IP address instead.
*/
function deviceGeolocationUnavailable() {
console.log('Device geolocation failed')
console.log('Device geolocation failed; getting location from IP address.')
getGeolocationFromIP().then((coords) => {
console.log('Geolocation from IP address successful!')
updateLocationText(anonymizeCoordinate(coords.lat), anonymizeCoordinate(coords.lon))
})
}

// First, attempt to get the user's location using the browser's geolocation
// API. If the browser does not support geolocation, or if the user denies
// the request, the `deviceGeolocationUnavailable` function will be called
// instead to retrieve an approximate location based on the user's IP address.
if (navigator.geolocation) {
coordsOutput.innerHTML = 'Retrieving your location...'

// If device geolocation is successful, the `success` function will be
// called with the `GeolocationPosition` object returned by
// `getCurrentPosition`. If the device geolocation fails, then call
// `deviceGeolocationUnavailable` instead, which takes no arguments.
navigator.geolocation.getCurrentPosition(success, deviceGeolocationUnavailable);
} else {
// If the browser simply does not support geolocation capabilities, don't
// even attempt to use it. Instead, get the location from the IP address.
deviceGeolocationUnavailable()
}
}

// Get the local timezone of the user's browser, and store it as a global variable
// for use in later calls to the weather API, which require a timezone for accurately
// dating the forecast data.
const localTimezone = getLocalTimezone()

// The script will run when the page is loaded, and will set up the event
// listener for the geolocation button, starting here. The geolocate button
// is a default button in the index.pug template.
document.querySelector('#geolocate').addEventListener('click', geolocate)
Copy link

Choose a reason for hiding this comment

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

The code looks generally correct and adheres to good practices, but there are a couple of potential issues that may need attention. Here they are:

  1. Error Suppression: The getGeolocationFromIP() function catches errors with a try/catch block, where it logs the error to the console. However, it doesn't provide any return statements or propagate the error so that calling functions can handle the error gracefully. When a network error occurs, the function's output will be undefined. This could cause downstream issues if attempts are made to destruct properties from this function.

  2. Geolocation Exception Handling: If the geolocation fails in the deviceGeolocationUnavailable function in the geolocate method, while calling getGeolocationFromIP(). It's not clear if this function properly handles a failure scenario. As per the current implementation, it might fail silently.

  3. Function return types: In the JSDoc for anonymizeCoordinate, it mentions that it returns a string, whereas the coordinate is actually a number. Since after using toFixed(2), the output becomes a string, ensure all pieces of code that interact with the return value of this function are designed to work with strings. Similarly, in the getGeolocationFromIP() function documentation, confirm that the object returned matches the expected structure across all execution paths.

  4. User approval for device geolocation: Most browsers require explicit user consent to access device geolocation data due to privacy concerns. Ensure this permission request is handled correctly as per best practices outside this script.

  5. Updating Element InnerHTML: In the updateLocationText() function, it directly updates the inner HTML of an element. This approach can potentially expose your application to cross-site scripting (XSS) attacks. Instead, consider using textContent property or safe methods that create sanitized text nodes.

  6. Dependency on Third-Party API: The /geolocate endpoint relies on a third-party API to geolocate the IP address. Any changes in that API or any outages could affect your function.

Remember to always test these patches thoroughly before pushing them into the production environment, covering as many edge cases as possible.

74 changes: 55 additions & 19 deletions src/weatherAPI.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,53 @@ function generateCurrentConditions(weatherData) {
return currentConditions
}

/**
* Parse the raw weather data retrieved from the Open-Meteo API into a format
* that can be used more easily by the client-side code.
* @param {Object} rawData - The raw weather data retrieved from the Open-Meteo API.
* @param {Number} rawData.latitude - The latitude of the location.
* @param {Number} rawData.longitude - The longitude of the location.
* @param {String} rawData.timezone - The timezone of the location.
* @param {Object} rawData.current - The current weather conditions.
* @param {Object} rawData.current_units - The units for the current weather conditions.
* @param {Object} rawData.daily - The daily weather forecast.
* @param {Object} rawData.daily_units - The units for the daily weather forecast.
* @param {string[]} rawData.daily.time - The timestamps for the daily forecast.
* @param {number[]} rawData.daily.weather_code - The weather codes for the daily forecast.
* @param {number[]} rawData.daily.temperature_2m_max - The maximum daily temperature.
* @param {number[]} rawData.daily.temperature_2m_min - The minimum daily temperature.
* @param {number[]} rawData.daily.precipitation_sum - The daily precipitation amount.
* @param {number[]} rawData.daily.precipitation_probability_max - The maximum daily precipitation probability.
*/
function parseWeatherResponse(rawData) {
console.log("Weather API raw response: ", rawData)

// TODO: Throw an error if the `rawData` object
// is missing the expected keys (i.e. `current`, `daily`)

rawData.current.weather = weatherCodes[rawData.current['weather_code']]
rawData.daily.weather = []
rawData.daily.weather_code.forEach(day => {
rawData.daily.weather.push(weatherCodes[day])
})

const parsedWeatherData = {
current: generateCurrentConditions(rawData),
daily_forecast: generateForecast(rawData)
}

return parsedWeatherData
}

/**
* Fetch weather data from the Open-Meteo API based on the provided latitude,
* longitude, and timezone. Returns a Promise that resolves with the parsed
* weather data.
* @returns {Promise<http.ClientRequest>} A Promise that resolves with the parsed weather data.
* @param {string} lat - The latitude of the location.
* @param {string} lon - The longitude of the location.
* @param {string} timezone - The timezone of the location.
*/
function getWeather(lat, lon, timezone) {

const queryParams = Object.assign({}, defaultQueryParams, {
Expand All @@ -169,46 +216,35 @@ function getWeather(lat, lon, timezone) {
timezone: timezone
})

// Construct the query string from the `queryParams` object, after merging
// the submitted values with the default values.
const queryString = Object.keys(queryParams).map(key => key + '=' + queryParams[key]).join('&')

const url = `http://api.open-meteo.com/v1/forecast?${queryString}`

return new Promise((resolve, reject) => {
let weatherResponse = {}

// Make the request to the Open-Meteo API to get the weather data.
const weatherReq = http.get(url, (res) => {
res.setEncoding('utf8')


// If the response status code is not 200, reject the Promise with an error.
if (res.statusCode !== 200) {
reject(new Error(`HTTP ${res.statusCode} ${res.statusMessage}`))
}

// Create a variable to store the response data chunks as they come in.
let responseData = ''
res.on('data', (chunk) => {
// console.log(`BODY: ${JSON.stringify(chunk)}`)
responseData += chunk
})

// When the response has finished, parse the JSON data and resolve the Promise.
res.on('end', () => {
try {
weatherResponse = JSON.parse(responseData)
console.log("Weather API raw response: ", weatherResponse)

// TODO: Throw an error if the `weatherResponse` object
// is missing the expected keys (i.e. `current`, `daily`)

weatherResponse.current.weather = weatherCodes[weatherResponse.current['weather_code']]
weatherResponse.daily.weather = []
weatherResponse.daily.weather_code.forEach(day => {
weatherResponse.daily.weather.push(weatherCodes[day])
})

const parsedWeatherData = {
current: generateCurrentConditions(weatherResponse),
daily_forecast: generateForecast(weatherResponse)
}

resolve(parsedWeatherData)
resolve(parseWeatherResponse(JSON.parse(responseData)))
} catch (e) {
console.error(e.message)
reject(e)
Copy link

Choose a reason for hiding this comment

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

  1. Missing error handling: Inside the parseWeatherResponse function, there are "TODO" comments indicating that error checks (for missing keys in the rawData object) need to be implemented but they are not present in this patch. This could lead to undefined references if expected keys are not present.

  2. Mutating input parameters: The parseWeatherResponse function is directly mutating the rawData parameter by adding new keys(rawData.current.weather and rawData.daily.weather) on it. It's generally a good practice to treat input parameters as immutable to avoid side effects.

  3. Lack of null/array length checks: In the loop where we generate rawData.daily.weather, there's no check for rawData.daily.weather_code being an array or being non-empty. If rawData.daily.weather_code is not defined or empty, the forEach call could raise exceptions.

  4. Misleading JSDoc: For the function getWeather the JSDoc indicates that lat and lon parameters should be string types. However, latitudes and longitudes are usually represented as numbers.

  5. Missing dependency declaration: The variable weatherCodes has been used but not declared or imported anywhere in the visible code. If it's not globally available, this will result in a ReferenceError exception.

  6. No HTTP Error Handling: While there’s a check for the status code 200, other HTTP/client errors like 404, 400, 500, network issues etc., are not explicitly handled.

  7. No check for Content-Type: Before calling JSON.parse(responseData), it would be good to confirm that the response's Content-Type is indeed JSON to avoid exceptions.

  8. Logging sensitive data: As per best practices, it is considered unsafe to log raw response data due to potential exposure of sensitive information. So console.log("Weather API raw response: ", rawData) might pose security risks.

I would suggest addressing these issues before merging this patch with the codebase.

Expand Down