Check out the source here. I wrote this to pull a prank on a coworker. It looks up words on urban dictionary from the command line. I'm just looking for places where I can improve code quality. I feel like the actual CLI library can be improved a lot.
Posted Sep 3, 2011, 6:13 PM UTC · edited
Nice use of OptionParser, though I would do the following things:
options.exit and the explicit opts.on('--help'); OptionParser gives you that already.STDERR before you print the help on line 66 (of cli.rb)The tests look nice; if you want to do a full-blown integration test, aruba is a bunch of cucumber scenarios to help.
What would putting all the text in the banner accomplish? Would it not double print the information for random and list?
Using separator for banner text is odd. The banner's only printed when you --help, so just use it to put any pre-text you want to appear before the options.
This makes sense. I interpreted your first point incorrectly and thought you meant all of the options.
Urban::Web a module, since it doesn't have state. Maybe inline directly into Urban::Dictionary?Dictionary#process probably doesn't need to return an OpenStruct, a normal Struct or Hash should be fine.You need Bundler if you have even 1 dependency (other than rake). Even if it's just a dev dependency. Can't tell you how many times I've gone to run the tests on a lib whose only dependency was RSpec 1.x, and the tests won't even run. I have to add a gemfile to their app and use trial/error to find what RSpec version they were using. Really annoying and most people won't care that much. Always use Bundler.
Put this in the Rakefile and spec/spec_helper.rb files:
gem 'rspec', '~> 1.4.0'
Also, if your not using Bundler, it's probably a good idea to wrap your rake tasks in begin / rescue:
begin
gem 'rspec', '~> 2.4'
require 'rspec/core/rake_task'
RSpec::Core::RakeTask.new
rescue LoadError
task :spec do
abort "Please run `gem install rspec` to install RSpec."
end
end
Project generators such as Ore can generate Ruby projects withour or with Bundler. That is how I generate projects without Bundler, and prevent the dependency hell you described.
Never, always use Bundler. Bundler works great for large projects, that often use git repositories, not for small libraries.
what's the downside of just using Bundler?
I am inclined to just always use it:
Something that starts out small can grow to be large, why have to worry about it later, just use Bundler.
Even something small with 5 dependencies can theoretically be victim to the sort of conflicting dependency problems that were commonplace before bundler -- After all, those 5 dependencies have their OWN dependencies, the total dependency tree could be quite large even with only 3 or 5 top level dependencies. you know, where something includes WidgetGem unconstrained and gets the most recent one, and then something loaded later wants only an older one, and rubygem raises.
Using bundler gives the end user a clear way to install all gem dependencies -- bundle install. What's the alternative to installing dependencies, reading the documentation and manually installing each one? Even with only 3-5 dependencies, I'd rather just type 'bundle install' -- something any experienced ruby developer recognizes how to do, these days.
What are the downsides to using Bundler, why NOT use it?
On the other hand, the calculus may be different for a gem -- since under ordinary use of the gem, bundler in the gem itself isnt' actually used for dependencies (the gem spec is used instead -- possibly by bundler, but that's up to the end user).
So in a gem, bundler is in fact only used in development by developers. Still, why not just use it?
gem 'foo', '~> 1.2'install:deps rake task.Downsides:
$GEM_HOME/cache/bundler/ with Git repository checkouts.Gemfile with only gemspec in it.bundle execBundler is great for large projects, that often need to use edge-versions from Git repositories. Bundler is not a hammer.
Thanks for the feedback. You make a good point about moving Urban::Web into a module due to its stateless nature. I originally did have it as part of Urban::Dictionary. I moved it because I was thinking if Urban Dictionary ever re-implemented a true API, Urban::Web would just be replaced by that, or an interface to it (With the assumption I would need to clean up their results). Thoughts? I know it is insignificant to be that worried about architecture on a small project like this, but it is more about learning.
I would make Urban::Dictionary the main class for the API, maybe have other classes to represent the different results from the web-scraping.
Nice job. Here's a few thoughts, I'll rank how important I think each is with a star rating (out of 5 stars)
(5/5) If you ask for a word that Urban Dictionary doesn't know about, it blows up: urban -l lkjlkjlkjlkjlkj /Users/joshuajcheek/.rvm/gems/ruby-1.9.2-p180/gems/urban-0.1.3/lib/urban/dictionary.rb:29:in
process': undefined methodcontent' for nil:NilClass (NoMethodError) from /Users/joshuajcheek/.rvm/gems/ruby-1.9.2-p180/gems/urban-0.1.3/lib/urban/dictionary.rb:23:insearch' from /Users/joshuajcheek/.rvm/gems/ruby-1.9.2-p180/gems/urban-0.1.3/lib/urban/cli.rb:19:inrun' from /Users/joshuajcheek/.rvm/gems/ruby-1.9.2-p180/gems/urban-0.1.3/bin/urban:7:in<top (required)>' from /Users/joshuajcheek/.rvm/gems/ruby-1.9.2-p180/bin/urban:19:inload' from /Users/joshuajcheek/.rvm/gems/ruby-1.9.2-p180/bin/urban:19:in `<main>'(5/5) (EDIT: According to comments, this is true for application code, but not library code. I've done a little research, and they appear to be correct) Your Gemfile.lock should be in your version control, but you have it in your .gitignore. This meant that when I tried to run
rake testit appeared to work, but really I wasn't in the Bundler sandbox (at least that's what I think was happening). It took me a while to realize I had no Gemfile.lock and do a bundle install. From the Bundler homepage "Make sure to add Gemfile.lock to your repository. This will ensure that other developers on your app, as well as your deployment environment, use exactly the same third-party code as you just installed."(1/5) My opinion of encapsulation is that you should encapsulate from even your current object. You've defined the
attr_accessor :web_servicebut you access it everywhere through the instance variable. My opinion is that your current object should access it only through setters/getters, unless the method itself is a setter/getter (or presents that interface).(1/5) I have grown to prefer lazy access. So instead of defining
@web_servicein your initialize method, I'd define it in the getter: attr_writer :web_service def web_service @web_service ||= Urban::Web.new end(3/5) I wonder about the interface you've selected. You have
@web_service.query(:random)and@web_service.query(:define, phrase)That feels forced to me. I assume you started by writing the Urban::Web class and after you got it working, you wrote the Urban::Dictionary class to use it. I strongly recommend going the other way around (and if you get into the BDD scene, there area some really awesome tools like Cucumber to help with this). So I'd start by writing Urban::Dictionary, which at some point will probably need a web service (but don't constrain yourself with this knowledge of destination), and then have it write the code it wishes it had. Then go implement that code. You will probably not end up with a publicly facing method like your query method. For me, if you told me to do that, I'd probably come up with something like this: def random process website.random_word end(4/5) Which brings up a point worth taking note of, your random word and search phrase methods don't really seem like they should be dealing with Nokogiri. That really isn't their responsibility, it doesn't have anything to do with getting words, it is a lower level of abstraction that should probably be part of the processor (if we say the web service's responsibility is merely to format queries and actually talk to the site, then it is the processor whose responsibility is to decide how to interpret that data. No one else should have to know whether it uses Nokogiri or Rexml or just a regular expression or anything else)
(1/5) OpenStruct isn't necessary in the Dictionary class as you always have the same keys, a regular struct should be fine. Result = Struct.new :word, :definitions def process(raw) document = to_document(raw) Result.new extract_word(document), extract_definitions(document) end
(2/5) It's kind of awkward to say
Urban::Dictionary.new.random, instantiating the object shouldn't be necessary, I think these should be class methods (maybe make it a module, then extend it onto itself so that others can include it if they like, but you also get the class style methods)(4/5) The CLI interface is unintuitive. It looked to me for a long time that I was supposed to do
urban -l smokey bearas the complement tourban -r. I initially was going to say that --list doesn't really imply that to me, but eventually I got to the test forurban -rl, where I realized I must misunderstand what list does. Maybe have some examples in the help screen. Based on the tests, there are only four ways to use it from the CLI, so it would be easy to show each of these.(2/5) It looks like you might have forgotten -v shorthand flag for --version. I see that you have tested for it, but it is completely unclear to me why it works -- is this some sort of optparse "feature"? Also, not sure how I feel about version also printing out help screen, it seems like that's not what it should do. Consider if you were trying to consume its output from a bash script, you'd have to then parse the specific line you wanted. I suspect it just does that b/c of the procedural way you parse the args and then print the results makes it hard to deviate from that route.
(4/5) CLI#parse is also outputting data. This means its name is lying, you can't just give it options and have it parse them, there is an almost certainly undesirable side effect of writing to stdout.
(4/5)
Bundler::GemHelper.install_tasksrequires you to have a git repo... That's not really your fault, but it means that if I even dorake -Torrake specthat it prints out "fatal: Not a git repository (or any of the parent directories): .git" fifteen times. I shouldn't have to have a git repo to run the tests or see the tasks.(4/5) Your stub module doesn't stub the object like you think, it actually stubs the class (meaning your test stubs Urban::Web#open for every instance of Urban::Web) Consider changing the implementation to be like this (in 1.9.2, there is already a singleton_class method, but since you're trying to be compatible with 1.8, we'll do it this way) module Stub def stub(name, &block) singleton_class = class << self; self; end singleton_class.send(:define_method, name, &block) end end
(1/5) You do
CLIDefinintionOutputTest < CLITestwhich means that CLIDefinintionOutputTest has all of CLITest's methods, including its test methods, so all of it's tests get run twice (ie stick a print statement into any of CLITest's methods that begin with test_)(3/5) Your dependencies in your gemspec say "s.add_dependency 'nokogiri', '>= 1.5.0'" It's generally inadvisable to use >= because that will allow anything, even nokogiri version 5.7.3 which might not be developed for another decade, but will probably not be congruent with your app. Generally, use the ~> operator instead (there's a brief discussion on it at http://guides.rubygems.org/patterns/)
Anyway, you did a really nice job with this, I ran rcov against it, and it only had 1 line that wasn't tested (Urban::Dictionary#parse_definitions last line
end || []), but it might have been used in another version of Ruby (rcov is only 1.8), so I'll assume 100% code coverage, which is a really nice accomplishment. I enjoyed reading your tests, and liked how you broke the responsibility of talking to the website out from parsing results and from interfacing with the user. Your code was generally clean and easy to read, you adhered to a lot of good practices and demonstrated a competency across many diverse topics.Great job, and as I often reference Urban Dictionary, I'll probably be using it :P Also, a feature request: Display the URL the info is taken from that way I can link people to it easily.
Thanks very much for taking the time to go through all of the trouble of reading, analyzing and commenting on my code. You have mentioned a lot of things I was unaware of. I am aware of the problem with entering a word that doesn't exist. This was handled with a catch all rescue in an earlier version. I rewrote the whole program first writing tests then implementing the code. I wanted to properly handle 404's and other exceptions that might arise, but I forgot about it and it was never included.
I changed the OpenStruct returned by the Dictionary class to be a hash, but after further reading it might make more sense to make it a struct as you have mentioned, since the contents are related.
I did not forget the shorthand flag for version. I intentionally left it out because I found usually -v is shorthand for verbose (which could be an alternative to --list but I'm not sold). You found an interesting problem. I forgot to change the test for the -v flag to --version. The result of this seems to either be a mistake on my part or, a side effect of OptionParser.
I will be making a lot of the changes you have mentioned. Do you mind adding an issue for the feature request? I will be implementing it because it is a really good idea. I would like some advice on the placement of it in the results.
according to some other things I've read you should not check in your Gemfile.lock if you are creating a gem
http://stackoverflow.com/questions/4151495/should-gemfile-lock-be-included-in-gitignore
I am thinking you may be right. I just ran a test by creating a gem with bundler and it created the .gitignore with the Gemfile.lock ignored. This is how it ended up in my .gitignore.
I agree with most of this, I've noted a couple areas where my opinions differs slightly.
This is the best practice when developing an app, not a library. The
bundle gemcommand adds Gemfile.lock to the .gitignore automatically.I used to prefer this method, but now I prefer to create it in initialize. I will make it use lazy access if building the object requires some external resource or there is a chance it will not be used during the object's life.
I agree, Web#search and Web#random seem better than lumping all of the logic into #query.
I personally like that Urban::Dictionary is a class that can be instantiated, however, I think that you could expose some convenience methods on the Urban::Dictionary class itself.
These methods could delegate to an instance of Dictionary stored in the class itself.