-
Notifications
You must be signed in to change notification settings - Fork 5
Popescu Alexandru Interview #4
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,33 +1,47 @@ | ||
# Backend Interview Starting Point | ||
<details> | ||
<summary><i>Local setup</i></summary> | ||
|
||
This repo will serve as a starting point for your code challenge. Feel free to change anything in order to complete it: Change framework, other tests, new gems etc. | ||
1. Install ruby: `$ rvm install 3.4.1` | ||
2. `$ cd .` or `$ cd <path_to_project>` to auto-create the rvm gemset | ||
3. Install bundler: `$ gem install bundler` | ||
4. Install the dependencies with bundler: `$ bundle install` | ||
</details> | ||
|
||
## Get this repo | ||
### Code Structure and Purpose | ||
|
||
- Fork this repo | ||
- Clone your fork | ||
- **Controller to Service**: When a request is received, the controller delegates the task to the appropriate service. **Sinatra** is used to handle HTTP requests and responses in a lightweight manner. | ||
- **Service to External API**: The `CoffeeShopFinderService` interacts with an external API to retrieve data about coffee shops. It processes this data to find the closest coffee shops based on the provided coordinates. | ||
- **Response Handling**: After processing the request, the service returns the result to the controller, which then formats the response and sends it back to the client. | ||
|
||
## Prerequisites | ||
- Have RVM installed: https://letmegooglethat.com/?q=install+rvm+on+ubuntu | ||
### Testing | ||
|
||
## Local setup | ||
1. Install ruby: `$ rvm install 3.4.1` | ||
2. `$ cd .` or `$ cd <path_to_project>` to auto-create the rvm gemset | ||
3. Install bundler: `$ gem install bundler` | ||
4. Install the dependencies with bundler: `$ bundle install` | ||
- **Unit Tests**: The project is thoroughly tested using the `rspec` gem. | ||
- Run tests ➡️ `$ bundle exec rspec` | ||
|
||
## Run sample CLI command | ||
`$ bin/ruby-interview` | ||
|
||
## Run tests | ||
`$ bundle exec rspec` | ||
|
||
## Tools | ||
|
||
- Write HTTP APIs [rails](https://rubyonrails.org/) or [roda](https://roda.jeremyevans.net/documentation.html) or others | ||
- Write CLI tools [thor](http://whatisthor.com/) or [tty](https://ttytoolkit.org/) or others (including [rake](https://github.com/ruby/rake)) | ||
- Test your code with [rspec](https://rspec.info/) | ||
# Usage | ||
#### Run CLI command to start the server | ||
`$ bin/start` | ||
|
||
--- | ||
#### Choose a provider to call the endpoind, I choose [Thunder Client](https://www.thunderclient.com/) inside VS Code. (Postman, Insomnia) | ||
`http://localhost:9292/api/closest_shops?lat=47.6&lon=-122.4` | ||
#### Should see a reponse similar to: | ||
 | ||
|
||
Good luck! | ||
|
||
#### It can be also tested using the terminal | ||
```curl "http://localhost:9292/api/closest_shops?lat=47.6&lon=-122.4"``` | ||
``` | ||
Coffee shops nearest (47.6, -122.4) by distance: | ||
|
||
0.0645 <--> Starbucks Seattle2 | ||
0.0861 <--> Starbucks Seattle | ||
10.0793 <--> Starbucks SF' | ||
``` | ||
|
||
### Disclaimers | ||
|
||
- I thought about setting up a DB, querying the endpoint to seed it and then seeding it periodically. | ||
- But taking into the consideration the size of the csv file, the minimal number of requests I choose to prioritize always having the latest data and calling the endpoint through my CoffeeShopFinderService on each request." |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
# frozen_string_literal: true | ||
|
||
require_relative 'config/environment' |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,79 @@ | ||
# frozen_string_literal: true | ||
|
||
require 'sinatra/base' | ||
require 'json' | ||
require_relative '../services/coffee_shop_finder_service' | ||
|
||
class CoffeeShopController < Sinatra::Base | ||
configure do | ||
set :protection, except: [:host_authorization] | ||
set :show_exceptions, false | ||
set :raise_errors, true | ||
enable :logging | ||
end | ||
|
||
before do | ||
content_type :json | ||
end | ||
|
||
get '/closest_shops' do | ||
content_type 'text/plain' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why did you choose this content type? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
lat, lon = validate_coordinates!(params) | ||
|
||
shops_with_distances = coffee_shop_finder_service.closest_shops(lat, lon) | ||
format_response(shops_with_distances, lat, lon) | ||
rescue StandardError => e | ||
handle_error(e) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why does success returns a plain/text and failure returns a json? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hey! I'll answer here for 2 other comments as well to keep the conversation in one place since they're all related. Related comments: Why set it here as json, and override it on line 20? , Why did you choose this content type? |
||
end | ||
|
||
private | ||
|
||
# Format shops into "Name --> distance <-- (user-lat, user_lon)" strings | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why this format? What's the intended audience? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The format from that comment is outdated as well, it actually returns:
I meant to match the |
||
def format_response(shops_with_distances, user_lat, user_lon) | ||
header = "Coffee shops nearest (#{user_lat}, #{user_lon}) by distance:\n\n" | ||
|
||
header + shops_with_distances.map do |shops_with_distance| | ||
shop = shops_with_distance[:shop] | ||
distance = shops_with_distance[:distance] | ||
|
||
"#{distance} <--> #{shop.name}" | ||
end.join("\n") | ||
end | ||
|
||
def validate_coordinates!(parameters) | ||
error!(400, 'Invalid coordinates') unless parameters[:lat] && parameters[:lon] | ||
error!(400, 'Coordinates must be numeric') unless numeric?(parameters[:lat]) && numeric?(parameters[:lon]) | ||
|
||
lat = parameters[:lat].to_f | ||
lon = parameters[:lon].to_f | ||
error!(400, 'Invalid coordinates') unless lat.between?(-90, 90) && lon.between?(-180, 180) | ||
|
||
[lat, lon] | ||
end | ||
|
||
def numeric?(str) | ||
return false unless str =~ /\A[-+]?[0-9]*\.?[0-9]+\Z/ | ||
|
||
Float(str) | ||
end | ||
|
||
# Handle errors with appropriate HTTP status codes | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This comment doesn't tell more than what the code tells. Comments have a specific purpose throughout the code. You can check this paper for when comments might be a good idea: https://stackoverflow.blog/2021/12/23/best-practices-for-writing-code-comments/ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be deleted as well: # Validate CSV row structure 🗑️ |
||
def handle_error(error) | ||
status_code = case error.message | ||
when /Invalid CSV/ then 400 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 400 is not correct here according to industry standards: https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/400. "The HTTP 400 Bad Request client error response status code indicates that the server would not process the request due to something the server considered to be a client error. The reason for a 400 response is typically due to malformed request syntax, invalid request message framing, or deceptive request routing. Clients that receive a 400 response should expect that repeating the request without modification will fail with the same error." It's not requester fault that the server's data is invalid. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The requester may not modify the request and stop receiving 400 at later times There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yea correct this is a mistake. The fact that our DB data is invalid is not on the client side. |
||
when /Failed to fetch CSV/ then 502 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 502 is not correct here according to industry standards: https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/502 "The HTTP 502 Bad Gateway server error response status code indicates that a server was acting as a gateway or proxy and that it received an invalid response from the upstream server. This response is similar to a 500 Internal Server Error response in the sense that it is a generic "catch-call" for server errors." Our server does not act as a gateway or proxy, although it requests another server. In our case, the remote CSV acts as a database There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree with you. I treated the remote CSV as another server, so |
||
else 500 | ||
end | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any other alternative considered? Can you share the pros and cons of different approaches? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes so I choose this approach because it was simple and fast to implement for this particular coding exercise. It's not a scalable solution though, other better approaches I think would be:
|
||
|
||
status status_code | ||
{ error: error.message }.to_json | ||
end | ||
|
||
def error!(code, message) | ||
halt code, { error: message }.to_json | ||
end | ||
|
||
def coffee_shop_finder_service | ||
CoffeeShopFinderService.new | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,36 @@ | ||
# frozen_string_literal: true | ||
|
||
class CoffeeShop | ||
class InvalidCoordinatesError < StandardError; end | ||
|
||
attr_reader :name, :latitude, :longitude | ||
|
||
def initialize(name, latitude, longitude) | ||
@name = validate_name(name) | ||
@latitude = validate_coordinate(latitude.to_f, -90..90, 'Latitude') | ||
@longitude = validate_coordinate(longitude.to_f, -180..180, 'Longitude') | ||
end | ||
|
||
def distance_to(user_lat, user_lon) | ||
user_lat = validate_coordinate(user_lat.to_f, -90..90, 'User Latitude') | ||
user_lon = validate_coordinate(user_lon.to_f, -180..180, 'User Longitude') | ||
|
||
Math.sqrt(((user_lat - latitude)**2) + ((user_lon - longitude)**2)).round(4) | ||
end | ||
|
||
private | ||
|
||
def validate_name(name) | ||
name = name.to_s.strip | ||
raise ArgumentError, 'Name cannot be empty' if name.empty? | ||
|
||
name | ||
end | ||
|
||
def validate_coordinate(coord, range, name) | ||
raise InvalidCoordinatesError, "#{name} must be a number" unless coord.is_a?(Numeric) | ||
raise InvalidCoordinatesError, "#{name} out of range" unless range.include?(coord) | ||
|
||
coord | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,50 @@ | ||
# frozen_string_literal: true | ||
|
||
require 'csv' | ||
require 'httparty' | ||
|
||
class CoffeeShopFinderService | ||
def closest_shops(user_lat, user_lon, limit = 3) | ||
coffee_shops = fetch_and_parse_coffee_shops | ||
|
||
# Used for faster distance retrieval | ||
distances = Hash.new { |h, shop| h[shop] = shop.distance_to(user_lat, user_lon) } | ||
closest_shops = coffee_shops.min_by(limit) { |shop| distances[shop] } | ||
|
||
closest_shops.map { |shop| { shop: shop, distance: distances[shop] } } | ||
end | ||
|
||
private | ||
|
||
def fetch_and_parse_coffee_shops | ||
response = fetch_csv | ||
parse_coffee_shops(response) | ||
end | ||
|
||
def parse_coffee_shops(response) | ||
CSV.parse(response.body, headers: headers).map do |row| | ||
validate_csv_row!(row) | ||
CoffeeShop.new(row['Name'], row['Lat Coordinate'], row['Lon Coordinate']) | ||
end | ||
rescue CSV::MalformedCSVError => e | ||
raise "Malformed CSV: #{e.message}" | ||
end | ||
|
||
def fetch_csv | ||
url = ENV['CSV_URL'] || APP_CONFIG[:csv_url] | ||
response = HTTParty.get(url) | ||
raise "Failed to fetch CSV: #{response.code}" unless response.success? | ||
|
||
response | ||
end | ||
|
||
# Validate CSV row structure | ||
def validate_csv_row!(row) | ||
missing = headers.reject { |h| row[h] } | ||
raise "Invalid CSV headers: #{missing.join(', ')}" if missing.any? | ||
end | ||
|
||
def headers | ||
['Name', 'Lat Coordinate', 'Lon Coordinate'] | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
#!/usr/bin/env ruby | ||
# frozen_string_literal: true | ||
|
||
puts 'Good luck!' | ||
system('bundle exec rackup config.ru') |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
# frozen_string_literal: true | ||
|
||
require_relative 'app' | ||
|
||
map '/api' do | ||
run CoffeeShopController | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
# frozen_string_literal: true | ||
|
||
require 'yaml' | ||
require 'logger' | ||
|
||
APP_CONFIG = YAML.load_file(File.join(__dir__, 'settings.yml')).transform_keys(&:to_sym) | ||
APP_LOGGER = Logger.new($stdout) | ||
|
||
Dir[File.join(__dir__, '../app/**/*.rb')].each { |file| require file } |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
csv_url: 'https://raw.githubusercontent.com/Agilefreaks/test_oop/master/coffee_shops.csv' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why set it here as json, and override it on line 20?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refer to: Why does success returns a plain/text and failure returns a json?