current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 4 down vote favorite

I made a command line Hacker News reader in Ruby that pulls the latest 10 stories from HN and allows the user to cycle through them or open them with Launchy. This was to play around with Ruby and APIs.

The data that I pulled from HN was messy, like an array of hashes with arrays and hashes inside. I feel like my code is messy, and I am unsure of how it can be made cleaner.

Any advice? I'm still a begninner.

news.rb --

class News
  def initialize title, url, author
    @title = title
    @url = url
    @author = author
  end

  # Format display of each story.
  def to_s
    "\n\tTitle:  #{@title}\n
    \tAuthor: #{@author}\n
    \tUrl: #{@url}\n\n"
  end

  # Insert current object's url into a variable accessible by Launchy
  def get_url
    @url
  end

end

# Show the news story, and ask if user wants open link or move on.
def show_story
    puts @stories[@counter]
    @link = @stories[@counter].get_url
    puts "To view this story in your browser, type OPEN\n
    To see the next story, type NEXT"
    @start = gets.chomp.downcase
    shuffle
  end

# Determine if user wants to open link or move on.
def shuffle
  if @start == "open"
    Launchy.open(@link)
    next_story
  elsif @start == "next"
    @counter += 1
    show_story
  else
    puts "Sorry, you didn't type a correct command.\n
    To view this story in your browser, type OPEN\n
    To see the next story, type NEXT"
    @start = gets.chomp.downcase
    shuffle
  end
end

# After opening link, ask if user still wants to see next story
def next_story
  puts "Would you still like to see the next story? Type NEXT"
  @start = gets.chomp.downcase
  shuffle
end

runner.rb --

require 'json'
require 'rest-client'
require 'launchy'
require_relative 'news.rb'

# Pull the latest/top 10 stories from Hacker News
data = RestClient.get('http://api.thriftdb.com/api.hnsearch.com/items/_search?limit=10&sortby=product(points,pow(2,div(div(ms(create_ts,NOW),3600000),72)))%20desc&pretty_print=true')

# Parse using JSON
parsed = JSON.parse data

# Take out the everything not under "Results" key
results = parsed.reject { |key, value| key != "results"}

# Create array and slap the value of "Results" into it
pull_items = []
pull_items = results["results"].each do |items|
  pull_items << items
end

# Create an array and make each "Item" into its own item
pull_info = []
pull_items.each do |x|
  pull_info << x["item"]
end

# Create a new object for each news story
@stories = pull_info.map do |s|
  News.new s["title"], s["url"], s["username"]
end

# Logo and Welcome Message
puts <<STRING
  _   _            _               _   _                    
 | | | |          | |             | \\ | |                   
 | |_| | __ _  ___| | _____ _ __  |  \\| | _____      _____  
 |  _  |/ _` |/ __| |/ / _ | '__| | . ` |/ _ \\ \\ /\\ / / __| 
 | | | | (_| | (__|   |  __| |    | |\\  |  __/\\ V  V /\\__ \ 
 \\_| |_/\\__,_|\\___|_|\\_\\___|_|    \\_| \\_/\\___| \\_/\\_/ |___/ 
        Welcome to Hacker News... from the Command Line!
STRING

# View the first story
puts "\nTo see the latest post, press any key"
start = gets
@counter = 0
show_story
share|improve this question
add comment

1 Answer

up vote 3 down vote

Redundant operations
After reading the data from the web service, you go over it a number of times, which seems redundant:

results = parsed.reject { |key, value| key != "results"}

Why is that important? You go over the whole JSON, and discard everything - but you gain nothing. Simply use only what you need, no need to actively remove it.

pull_items = []
pull_items = results["results"].each do |items|
  pull_items << items
end

This is simply a very convoluted way of saying pull_items = results['results']...

pull_info = []
pull_items.each do |x|
  pull_info << x["item"]
end

Ruby's idiomatic way of saying the same thing is pull_info = pull_items.map { |x| x['item'] }

@stories = pull_info.map do |s|
  News.new s["title"], s["url"], s["username"]
end

If this is what you actually wanted, you don't really need all the temporary artifacts in the middle - simply do it in one swoop: 

@stories = results['results'].map do |r| 
  item = r['item']
  News.new item['title'], item['url'], item['username']
end

Naming Conventions
It is conventional in ruby to name getters by the name of the member, without the get_XXX. For that convention, there is even a shortcut - attr_reader. So, instead of writing def get_url you should do:

class News
  attr_reader :url
  ...
end

@link = @stories[@counter].url

and it will just work...

Code Encapsulation
A ruby file should contain a single class. It should not contain any floating methods.
Actually, there should not be any floating methods. Create a different class, or a module, which will contain the show_story, shuffle, and next_story. This will also force you to name the module, and make it more obvious what is its role in your application.

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.