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.

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

Coding challenge #23

Open
wants to merge 37 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
2fc361a
Make assumptions about the shape of our world
madalindaniel92 Aug 27, 2021
afb5fd4
Add MIT License
madalindaniel92 Aug 27, 2021
cef208f
Add copyright notice regarding the ownership of the coding challenge …
madalindaniel92 Aug 27, 2021
0c5ee0c
Add RSpec gem for testing
madalindaniel92 Aug 27, 2021
1c8cbab
Add version
madalindaniel92 Aug 27, 2021
7493561
Add Rubocop for style guide enforcement
madalindaniel92 Aug 27, 2021
46d6b3a
Fix rubocop style errors
madalindaniel92 Aug 27, 2021
53c6ca2
Add byebug debugger
madalindaniel92 Aug 27, 2021
0178864
Set project ruby version
madalindaniel92 Aug 27, 2021
4fc1893
Add Result class for representing success values
madalindaniel92 Aug 27, 2021
a4fa48d
Add argument parser using Ruby's OptParser
madalindaniel92 Aug 27, 2021
b7bb3c4
Small fix gemfile - move rspec into correct gem group
madalindaniel92 Aug 27, 2021
41ea710
Small tweaks rubocop config
madalindaniel92 Aug 27, 2021
d36cbf0
Add simple option parsing CLI
madalindaniel92 Aug 27, 2021
62009e6
Merge branch 'feature/cli_options' into develop
madalindaniel92 Aug 27, 2021
751f9b0
Add constants and helpers for geographic information
madalindaniel92 Aug 27, 2021
4549062
Add Location class for validating coordinates
madalindaniel92 Aug 27, 2021
d086581
Fix latitude and longitude max constants
madalindaniel92 Aug 27, 2021
b379bab
Add be_almost_eq RSpec helper
madalindaniel92 Aug 27, 2021
e6c0236
Stop printing rspec example run times
madalindaniel92 Aug 27, 2021
3aaae73
Calculate flat Earth distance between locations
madalindaniel92 Aug 27, 2021
88d9a42
Merge branch 'feature/location_distance' into develop
madalindaniel92 Aug 27, 2021
b9819b1
Implement equality for locations
madalindaniel92 Aug 28, 2021
e30c241
Add remote file importer
madalindaniel92 Aug 28, 2021
02c8b97
Merge branch 'feature/import_locations' into develop
madalindaniel92 Aug 28, 2021
00e2ede
Add ability to search for closest locations
madalindaniel92 Aug 28, 2021
2ceca3c
Merge branch 'feature/search_locations' into develop
madalindaniel92 Aug 28, 2021
b9beda3
Refactor CLI#run using smaller purpose built methods
madalindaniel92 Aug 28, 2021
7016e70
Refactor Importer, fixing Rubocop offenses
madalindaniel92 Aug 28, 2021
742d9ed
Add CLI flag for number of search results returned
madalindaniel92 Aug 28, 2021
d290b3f
Shh, nothing to see here Rubocop...
madalindaniel92 Aug 28, 2021
0caee5b
Fix earth_distance latitude calculation, avoiding teleportation betwe…
madalindaniel92 Aug 28, 2021
c406cad
Add setup and running instructions to ReadMe
madalindaniel92 Aug 28, 2021
482136f
Move location validation logic from class to instance methods
madalindaniel92 Sep 4, 2021
033654a
Refactor geo_spec to test single scenario per example
madalindaniel92 Sep 4, 2021
89e854c
Rescue SocketError when downloading remote file
madalindaniel92 Sep 4, 2021
abb0fb0
Check remote file uri scheme before trying to open
madalindaniel92 Sep 4, 2021
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
3 changes: 3 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
# Ignore RSpec slow examples file
spec/examples.txt
/.byebug_history
1 change: 1 addition & 0 deletions .rspec
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
--require spec_helper
4 changes: 4 additions & 0 deletions .rubocop.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
AllCops:
NewCops: enable
Exclude:
- 'coffee_place'
1 change: 1 addition & 0 deletions .ruby-version
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
ruby-3.0
12 changes: 12 additions & 0 deletions Gemfile
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
# frozen_string_literal: true

source 'https://rubygems.org'

git_source(:github) { |repo_name| "https://github.com/#{repo_name}" }

# gem "rails"
group :development, :test do
gem 'byebug', '~> 11.1'
gem 'rspec', '~> 3.10'
gem 'rubocop', '~> 1.20', require: false
end
49 changes: 49 additions & 0 deletions Gemfile.lock
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
GEM
remote: https://rubygems.org/
specs:
ast (2.4.2)
byebug (11.1.3)
diff-lcs (1.4.4)
parallel (1.20.1)
parser (3.0.2.0)
ast (~> 2.4.1)
rainbow (3.0.0)
regexp_parser (2.1.1)
rexml (3.2.5)
rspec (3.10.0)
rspec-core (~> 3.10.0)
rspec-expectations (~> 3.10.0)
rspec-mocks (~> 3.10.0)
rspec-core (3.10.1)
rspec-support (~> 3.10.0)
rspec-expectations (3.10.1)
diff-lcs (>= 1.2.0, < 2.0)
rspec-support (~> 3.10.0)
rspec-mocks (3.10.2)
diff-lcs (>= 1.2.0, < 2.0)
rspec-support (~> 3.10.0)
rspec-support (3.10.2)
rubocop (1.20.0)
parallel (~> 1.10)
parser (>= 3.0.0.0)
rainbow (>= 2.2.2, < 4.0)
regexp_parser (>= 1.8, < 3.0)
rexml
rubocop-ast (>= 1.9.1, < 2.0)
ruby-progressbar (~> 1.7)
unicode-display_width (>= 1.4.0, < 3.0)
rubocop-ast (1.11.0)
parser (>= 3.0.1.1)
ruby-progressbar (1.11.0)
unicode-display_width (2.0.0)

PLATFORMS
x86_64-linux

DEPENDENCIES
byebug (~> 11.1)
rspec (~> 3.10)
rubocop (~> 1.20)

BUNDLED WITH
2.2.25
23 changes: 23 additions & 0 deletions LICENSE.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
The MIT License (MIT)
====================

* Copyright © 2021 Spataru Madalin Daniel

Permission is hereby granted, free of charge, to any person obtaining
a copy of this software and associated documentation files (the
"Software"), to deal in the Software without restriction, including
without limitation the rights to use, copy, modify, merge, publish,
distribute, sublicense, and/or sell copies of the Software, and to
permit persons to whom the Software is furnished to do so, subject to
the following conditions:

The above copyright notice and this permission notice shall be
included in all copies or substantial portions of the Software.

THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE
LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION
OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION
WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
69 changes: 68 additions & 1 deletion Readme.md
Original file line number Diff line number Diff line change
@@ -1,4 +1,71 @@
## Overview
## Copyright notice

While the code itself is MIT licensed, the coding challenge itself, and its description,
included in this `Readme.md` file belongs to Agile Freaks et al.


## Coffee Place

This is an attempt at solving the closest coffee shop coding challenge.

All good software starts with making flawed assumptions.
Copy link
Member

Choose a reason for hiding this comment

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

💯

Copy link
Author

Choose a reason for hiding this comment

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

If that ain't the truth, I don't know what is! 😹


To make things easier on ourselves, we will assume that the Earth is flat.
We all know this to be true - all of those satellite pictures are hoaxes!

Furthermore, walking over the edge of the Earth teleports you to the opposing
side - like in Snake, or other old school video games.

This only works for the "left-right" sides, as crossing the 180 meridian takes
you to the other hemisphere. This does **not** work for the "up-down" sides,
as we don't have a good way of teleporting from the North Pole to the South Pole yet.

```

Oy
-180_______-90_______|________90_______180 ____ 90 latitude
| | |
| | |
| | |
|------------------0-----------------|-Ox-> ---- 0 latitude
| | |
| | |
| | |
|__________________|_________________| ____ -90 latitude


```


This project will use the Ruby programming languge to implement the CLI application.

Let's do this!

## Setup instructions

This project uses Ruby 3.0.

Setup steps:
- clone repo
- make sure you have Ruby 3.0 installed
using `rvm` or `chruby` will switch Ruby to the right version
- `gem install bundler` # Make sure bundler is installed for Ruby 3.0
- `bundle install` # Install dependencies
- `bundle exec rspec` # Run tests
- `bundle exec rubocop` # Delight in code style guide nagging :P


## How to run the program:
```
# Using local file
./coffee_place 47.6 -122.4 coffee_shops.csv

# Using remote file
./coffee_place 47.6 -122.4 https://raw.githubusercontent.com/Agilefreaks/test_oop/master/coffee_shops.csv
```

## Coding challenge Overview

You have been hired by a company that builds a mobile app for coffee addicts. You are
responsible for taking the user’s location and returning a list of the three closest coffee shops.
Expand Down
6 changes: 6 additions & 0 deletions coffee_place
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
#! /usr/bin/env ruby

require_relative './lib/coffee_place/cli.rb'

cli = CoffeePlace::CLI.new
cli.run(ARGV)
6 changes: 6 additions & 0 deletions lib/coffee_place.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
# frozen_string_literal: true

module CoffeePlace
VERSION = '0.1'
BANNER = 'Usage: ./coffee_place <user x coordinate> <user y coordinate> <shop data url or file>'
end
99 changes: 99 additions & 0 deletions lib/coffee_place/cli.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
# frozen_string_literal: true

require_relative '../coffee_place'
require_relative './location'
require_relative './cli_parser'
require_relative './importer'
require_relative './search'

module CoffeePlace
# Parses and runs CLI application using supplied args.

Choose a reason for hiding this comment

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

Why did you feel the need to explain what the classes do through additional comments?

Copy link
Author

@madalindaniel92 madalindaniel92 Sep 4, 2021

Choose a reason for hiding this comment

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

tldr; Rubocop is being naggy as always! 🤖 😛

Rubocop has a style cop that emits warnings when you haven't added a class comment description.
While disabling this cop is valid, when method names are explicit, I prefer using a consistent style.

Basically, some classes need no further explanation - their name is sufficient to understand their purpose.
Other classes need additional explanation as to their responsibility.
This would result in some classes having a documentation comment at the top, while others would
not have it - a minor style inconsistency.

This rubocop linter forces you to consistently add documentation at the top of any class,
making sure the style remains consistent.

I'm pretty sure the rubocop config for rails disables this, as convention over configuration makes
alot of class descriptions redundant.

Copy link
Author

Choose a reason for hiding this comment

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

I apologise for writing you a short novel in response to your comment ... I don't know how to stop myself from rambling sometimes 😛

Choose a reason for hiding this comment

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

I usually disable that cop, I find it naggy too, but recently the thought occured to me that it can be much more useful in more complex projects, as it would help new developers understand the project implementation-wise more easily.

Out of curiosity - have you used this documentation cop in production? If so, has it helped the team understand the project better?

Copy link
Author

@madalindaniel92 madalindaniel92 Sep 6, 2021

Choose a reason for hiding this comment

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

We usually used the rails-rubocop config, which I think has this cop turned off.

What helped us understand new projects best were design documents and mockups,
which are initially created when first interacting with the client. These explain what the
project aims to be, which is usually more than its current implementation state.

Maybe including links to helpful documents in the project's own ReadMe would help
with onboarding on new projects. We usually just shared these on Slack :D

Comments at the top of more complicated methods, or specs in which these methods are
tested usually help clarify specific parts of the code - but its hard to build a 'big picture' only
from these.

class CLI
attr_accessor :cli_opts, :parsed_args

def initialize(parser: CLIParser.new,
importer: Importer.new)
@parser = parser
@importer = importer
end

def run(args)
cli_opts = parse_cli_options!(args)

show_help_and_exit! if cli_opts.print_help?
show_version_and_exit! if cli_opts.show_version?
Comment on lines +23 to +24

Choose a reason for hiding this comment

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

I'm curious - why did you write up the methods with a bang?

Copy link
Author

@madalindaniel92 madalindaniel92 Sep 4, 2021

Choose a reason for hiding this comment

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

I like using a bang when naming methods that have important side effects.
This warns the programmer to be weary of side effects.
These methods exit the program - which is pretty severe, as far as side effects go.

In ActiveRecord's design, most methods with a bang have the side effect of throwing errors.
(e.g. save!, validate!)

In Ruby's standard library, a bang usually means that a method mutates data in place,
instead of creating a new data structure - for example Array#map!

Choose a reason for hiding this comment

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

Yes, I imagined that's the reason. Thank you for answering. :)


user_location = validate_user_location!(cli_opts)
locations = import_locations_from!(cli_opts)

search = Search.new(locations)
closest_locations = search.find_closest_to(user_location, cli_opts.num_results)

display_closest_locations(closest_locations)
end

private

def parse_cli_options!(args)
result = @parser.parse(args)
show_help_and_exit!(exit_status: 1) unless result.success?

cli_opts = result.value

# We must have [lat, lon, filename] as args
unless cli_opts.num_args == 3
puts 'Incorrect number of CLI arguments passed in'
show_help_and_exit!(exit_status: 1)
end

cli_opts
end

def validate_user_location!(cli_opts)
lat, lon = cli_opts.remaining
location = Location.new(lat: lat, lon: lon, name: 'Current user')

if location.valid?
location
else
print_errors_and_exit!(location.errors)
end
end

def import_locations_from!(cli_opts)
source_name = cli_opts.source_name
locations_result = @importer.import_from(source_name)

if locations_result.success?
locations_result.value
else
puts "Failed to import locations from: #{source_name.inspect}"
print_errors_and_exit!(locations_result.error)
end
end

def display_closest_locations(locations)
locations.each do |distance, location|
puts "#{location.name},#{distance.round(4)}"
end
end

def show_help_and_exit!(exit_status: 0)
@parser.show_help
exit exit_status
end

def show_version_and_exit!(exit_status: 0)
puts CoffeePlace::VERSION
exit exit_status
end

def print_errors_and_exit!(errors)
Array(errors).each do |error|
puts error
end

exit 1
end
end
end
35 changes: 35 additions & 0 deletions lib/coffee_place/cli_options.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
# frozen_string_literal: true

module CoffeePlace
# Keeps parsed CLI options, also defining defaults.
class CLIOptions
attr_accessor :version, :help, :remaining, :num_results

def initialize(version: false, help: false, remaining: [])

Choose a reason for hiding this comment

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

Interesting use of flags. Generally, are there any issues that could arise from the use of boolean params?

Copy link
Author

@madalindaniel92 madalindaniel92 Sep 4, 2021

Choose a reason for hiding this comment

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

Accidentally passing in nil is always a major pain.
There is always the possibility of someone passing a String too - such as 'false'.

ActionControlller usually does some light type casting to ensure values sent from checkboxes
or other fields are converted into boolean values.

When I use these sort of params internally in my code, I try to make sure that I always send a boolean,
even when the value is missing - by passing in a default value.

When using user submitted fields, some sort of type casting is useful.
I have found this StackOverflow answer helpful when parsing user submitted CSV files,
that contain columns which represent boolean values.

I would use something like:

module BooleanHelper
  module_function

  def cast(value)
     ActiveModel::Type::Boolean.new.cast(value)
  end
end

Copy link
Member

Choose a reason for hiding this comment

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

This are all valid points, but there is also a code smell that arises from using bools in your methods, you can read more about it in this Martin Fowler article, dating way back in 2011

Choose a reason for hiding this comment

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

One more article recommendation from me: Clean code: The curse of a boolean parameter.

Copy link
Author

Choose a reason for hiding this comment

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

I agree that using boolean arguments for control flow leads to needless complexity.
Having simpler methods to handle each case is better.
Thank you for the two articles.

@version = version
@help = help
@remaining = remaining
end

def show_version?
@version
end

def print_help?
@help
end

def source_name
@remaining.last
end

def <<(arg_value)
@remaining << arg_value
end

# Counts free standing CLI arguments, not taking flags into account
def num_args
@remaining.size
end
end
end
70 changes: 70 additions & 0 deletions lib/coffee_place/cli_parser.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
# frozen_string_literal: true

require 'optparse'
require_relative '../coffee_place'
require_relative './result'
require_relative './cli_options'

module CoffeePlace
# Parses CLI arguments into a `CLIOptions` instance.
class CLIParser
attr_accessor :cli_opts, :parsed_args

def parse(args)
# Make sure we are not modifying `args` directly
args = args.clone

@cli_opts = CLIOptions.new
@opt_parser = build_args_parser
@parsed_args = extract_free_standing_arguments(args)

Result.success(@cli_opts)
end

def show_help
puts @opt_parser
end

private

# Since we are declaratively defining independent flags, method length isn't a concern
#
# rubocop:disable Metrics/MethodLength
def build_args_parser
OptionParser.new do |opts|
opts.banner = CoffeePlace::BANNER

opts.on('-h', '--help', 'Print this help message') do
@cli_opts.help = true
end

opts.on('-v', '--version', 'Print version and exit') do
@cli_opts.version = true
end

opts.on('-n[NUM]', '--num=[NUM]', 'Number of search results to return - default is 3') do |num|
@cli_opts.num_results = num.to_i
end
end
end
# rubocop:enable Metrics/MethodLength

# Sadly, ruby's OptParser does not like arguments that start with a minus sign,
# as it interprets them as CLI command switches, e.g -122.4 raises OptionParser::InvalidOption
#
# This method catches `InvalidOption`, and gathers arguments that `optparser` can't parse.
#
# https://stackoverflow.com/questions/3642331/can-optionparser-skip-unknown-options-to-be-processed-later-in-a-ruby-program
def extract_free_standing_arguments(args)
@opt_parser.order!(args) do |unrecognized_opt|
@cli_opts << unrecognized_opt
end
rescue OptionParser::InvalidOption => e
# Push back unrecognized argument onto args so we can shift it afterwards
e.recover(args)
@cli_opts << args.shift

retry
end
end
end
Loading