danielharan

[Ruby] Password update code

2008-07-16T16:59:17-07:00

Mostly straight from a tutorial, but it's uuuugly:

class AccountsController < ApplicationController

  layout 'application'
  before_filter :login_required, :except => :show

  # Change password action  
  def update
  return unless request.put?
    # we authenticate with email 
    if Dj.authenticate(current_dj.email, params[:old_password])
      if ((params[:password] == params[:password_confirmation]) && !params[:password_confirmation].blank?)
        current_dj.password_confirmation = params[:password_confirmation]
        current_dj.password = params[:password]        
    if current_dj.save
        flash[:notice] = "Password successfully updated."
        redirect_to dj_path(current_dj.login) #profile_url(current_dj.login)
      else
        flash[:error] = "An error occured, your password was not changed."
        render :action => 'edit'
      end
    else
      flash[:error] = "New password does not match the password confirmation."
      @old_password = params[:old_password]
      render :action => 'edit'      
     end
   else
      flash[:error] = "Your old password is incorrect."
      render :action => 'edit'
    end 
  end    
end

[Ruby] On Squash a collection.

2008-07-06T03:31:13-07:00

def all_shops(products)
    products.collect { |product| product.shops }.flatten.uniq
end

[Ruby] On if else if else

2008-06-17T13:50:10-07:00

You could put those in a helper instead of assigning them.

Again, "if current_user.carrier_id == 0" smells. You should be able to do current_user.carrier? and leave out implementation details that belong in the model.

def email
  return '' unless logged_in?
  current_user.email
end

[Ruby] On collecting attributes from find(: all) method

2008-06-17T13:47:03-07:00

While Fabien's solution does exactly what you asked, I'd venture that what you asked is not what you want or need. Leave off the call to flatten - can you make the place where you use this simpler now?

[Ruby] On Looking for comments on style and clearness

2008-06-16T15:13:22-07:00

Can you link to information about your problem domain, or provide an explanation?

The comment for get_number_sequence isn't terribly useful - it re-iterates the what, instead of the WHY.

def get_number_sequence(number)
  @sequence = %w{ 24142 15151 25232 25252 14251 23252 23242 25151 24242 24252 }[number]
  ...
end

[Ruby] On RHTML Refactoring

2008-06-12T15:47:56-07:00

having carrier_id == 0 in the view smells. what does that mean? Is there a mysql default that's polluting data?

<% if @user.carrier? %>
  <li style="width:190px" id="mobile_no_div">
    <%= @user.carrier.name %> &nbsp; <%= @user.mobile_no %>
  </li>
<% end %>

[Ruby] On Array iteration

2008-06-12T00:02:33-07:00

def find_npa(dialed_nxx)
  return 819 if [ 205, 208, 210, 213, 243 ].include? dialed_nxx
  return 613 if [ 203, 204, 212, 216, 218 ].include? dialed_nxx
end

[Ruby] On time to second, second to time

2008-06-11T21:44:20-07:00

Why not just use Time ?

>> start = Time.at 0
=> Wed Dec 31 19:00:00 -0500 1969
>> later = start + 60 * 60 * 2 + 60 * 3 + 1 # add 2 hours, 3 minutes and 1 second
=> Wed Dec 31 21:01:01 -0500 1969
>> later.hour - start.hour
=> 2
>> later.min  - start.min
=> 3
>> later.sec - start.sec
=> 1

[Ruby] On Can this scraper be simplified?

2008-05-31T17:27:45-07:00

You're mixing too many concerns. In particular, separating the scraping from the processing is usually a huge win for readability; I save fetched pages in /tmp/cache. That way if I mess up the retrieval of data from the HTML (which happens, oh, always), I don't have to hit their servers again.

If there are two levels of pages to retrieve - search listings and actual data pages, you can write two different scrapers.

# why does the alphabet include '*' anyways?
(('a'..'z').to_a << '*').each do |letter|
  `curl|request somesite.com/search?q=#{letter} to /tmp/cache/searches/#{letter}`
end

[Ruby] On Building a dynamic date range

2008-05-09T00:53:13-07:00

This version is a bit longer, but should be more readable / easier to understand for other team members.

  # Find all articles in the given date range based on the published_at field
  def self.find_archived(year, month=nil)
    if month.nil?
      start_date = Date.parse("01/01/#{year}")
      end_date   = start_date + 1.year
    else
      start_date = Date.parse("#{month}/01/#{year}")
       end_date  = start_date + 1.month
    end
    self.find(:all, :conditions => {:published_at => start_date .. end_date})
  end

[Ruby] On Rubyize this : 6th edition

2008-05-04T00:11:43-07:00

System is a vile word in a class name. It is meaningless and encourages a pile up of concerns that should remain separated.

Since that dude retired 2 weeks ago, I think it's safe to scrap that piece of code. Normally it's better to refactor than re-write... but this is egregious.

class Poll
  attr_accessor :question
  def initialize(question, answers=[])
    @question, @answers = question, {}
    answers.each {|key| @answers[key] = 0}
  end
  
  def vote_for(answer)
    raise 'unknown answer' unless @answers.has_key?(answer)
    @answers[answer] += 1
  end
  
  def results
    @answers.each { |answer, votes| puts "#{votes} : #{answer}" }
    puts "Out of #{@answers.values.inject {|memo,v| v + memo}} votes"
  end
end

>> p = Poll.new("can haz chezburger?", ["laterz", "nao"])
=> #<Poll:0x35cb2c @answers={"laterz"=>0, "nao"=>0}, @question="can haz chezburger?">
>> p.vote_for "nao"
=> 1
>> p.vote_for "nao"
=> 2
>> p.vote_for "laterz"
=> 1
>> p.results
1 : laterz
2 : nao
Out of 3 votes

[Ruby] On Step-REST uuuuugly controller

2008-04-28T21:01:45-07:00

That's no longer really restful. Instead of what appears to be a wizard on the front-end, consider loading the form all at once - maybe with javascript that shows each successive part of the form, or with tabbed controls. For the RESTful actions, you could use http://jamesgolick.com/resource_controller

[Ruby] Ruby has_finder on has_many ?

2008-04-01T16:10:13-07:00

This smells... one of the main points of using has_finder was to isolate domain logic to the appropriate model (http://jamesgolick.com/2008/2/25/plugins-i-ve-known-and-loved-2-has_finder), but here it's scattered again.

Any ideas on how to elegantly clean this up?

class JobBoard < ActiveRecord::Base
  has_many :job_board_postings
  
  has_finder :distributed_by, lambda {|distributor_code| {:conditions => ['distributor = ?', distributor_code.to_s]} }
  has_finder :push,   :conditions => {:push => true}
  has_finder :manual, :conditions => {:manual => true}
  
  def push
    ! pull
  end
  alias_method :push?, :push
end

class JobBoardPosting < ActiveRecord::Base
  belongs_to :job
  belongs_to :job_board

  has_finder :distributed_by, lambda {|distributor_code| {:conditions => ['job_boards.distributor = ?', distributor_code.to_s], :include => :job_board} }
  
  # TODO: couldn't this be a delegated finder? not very DRY
  has_finder :pushed, :conditions => ["job_boards.pull = ?", false],  :include => :job_board
  has_finder :manual, :conditions => ["job_boards.manual = ?", true], :include => :job_board
end

[Ruby] On Challenge: Ugliest Ruby FizzBuzz

2007-12-21T15:53:17-08:00

No way can I compete with MA... for now, I'll just tweak my solution to make it more maintainable. Really, though, shouldn't this be a script where we can give params for start and end? What if requirements change, and we decide divisibility by 13 is fizzbazzy?

def fizz!
   'fizz'
end

def buzz!
   'buzz'
end

def fizzbuzz!
  fizz! + buzz!
end

number = 0
while number < 101 do
  number += 1
  puts number.fizzy? ? number.buzzy? ? fizzbuzz! : fizz! : number.buzzy? ? buzz! : number.to_s
end

[Ruby] On Challenge: Ugliest Ruby FizzBuzz

2007-12-20T22:18:41-08:00

I was inspired by http://refactormycode.com/codes/102-i-hate-to-do-this-but

I'm sure someone can figure something better than mine, but I thought proper namespacing was important. To keep the line count from going up too high, I used a few ternary operators. Also, that each is quite confusing so used a loop instead.

module Wasabi
  module Fizzy
    def fizzy?
      self % 3 == 0
    end
  end

  module Buzzy
    def buzzy?
      self % 5 == 0
    end
  end

  module SpaceCadet
    def <=>(b)
      self % b
    end
  end
end

class Fixnum
  include Wasabi::Fizzy
  include Wasabi::Buzzy
  include Wasabi::SpaceCadet
end

number = 0
while number < 101 do
  number += 1
  puts number.fizzy? ? number.buzzy? ? 'FizzBuzz' : 'Fizz' : number.buzzy? ? 'Buzz' : number.to_s
end

[C#] On Code to Simplify

2007-12-19T18:19:41-08:00

That code is an abomination. Two things for you to look at: modulo operator and arrays. It should only be about 12 lines once you are done.

[Ruby] On resource_controller: Redesign My API

2007-11-06T01:02:40-08:00

I prefer responds_to. Since we're on the topic, how about making the block optional?

  create do
    before { @post.user = current_user }
    responds_to :html, :js do |format|
      format.gif { create_gif(@post) }
    end
  end

[Ruby] On Is This String Postal?

2007-10-30T16:08:30-07:00

Now that I'm looking at that regexp, I notice the () that aren't necessary. Also wondering if it helps to add ^ and $, and whether the * should be replaced by a ?

In any case, to answer your initial question: this seems like a good direction. It's easy to go too far, or use this type of code where ActiveRecord Validations are more appropriate. If it helps capture a common pattern and makes your code more readable, I say go for it!

class String 
  def postal_code?  
    self.strip =~ /^[a-z]\d[a-z][ -]?\d[a-z]\d$/i
 end
end

[Ruby] On Is This String Postal?

2007-10-30T15:40:59-07:00

That ternary operator is not strictly necessary, since any MatchData object will evaluate to true. If no match exists, nil is returned. Another option is to use =~

class String 
  def postal?  
    self =~ /[a-zA-Z]{1}\d{1}[a-zA-Z]{1}([\x20-])*\d{1}[a-zA-Z]{1}\d{1}/
 end
end

[Ruby] Rails migration conflict repair

2007-10-26T18:25:22-07:00

Explained here: http://www.danielharan.com/2007/10/26/rails-migrations-handling-naming-conflicts/

namespace :migrations do
  
  desc 'reverts, renames and re-executes uncommitted migrations that conflict with already committed migrations'
  task :repair => :environment do
    uncommitted = `svn status`.select {|line| line =~ /db\/migrate\//}.collect {|line| line.match(/ db\/migrate\/(.*rb)/)[1]}
    committed   = Dir.glob('db/migrate/*').collect {|line| line.match(/db\/migrate\/(.*rb)/)[1]} - uncommitted
    
    highest_committed   = committed.collect(&:to_i).max
    lowest_uncommitted  = uncommitted.collect(&:to_i).min
    
    schema_version = ActiveRecord::Base.connection.select_one("select version from schema_info")["version"].to_i
    
    # revert uncommitted migrations
    uncommitted.sort_by(&:to_i).reverse.each do |migration|
      version = migration.to_i
      if version <= schema_version
        name = migration.match(/(\d*_.*).rb/)[1]
        class_name = (require 'db/migrate/' + name)[0]
        puts class_name.constantize.down 
      end
    end
    
    #set it back to the version prior to all the uncommitted migrations - this doesn't get done automatically by calling down
    ActiveRecord::Base.connection.execute("update schema_info set version=#{schema_version-1}")
    
    version_offset = (highest_committed - lowest_uncommitted) + 1
    
    uncommitted.each do |old_name|
      version  = old_name.to_i + version_offset
      new_name = version.to_s.rjust(3,'0') + old_name.gsub(/\d/, '')
      `mv db/migrate/#{old_name} db/migrate/#{new_name}`
    end
    
    puts "Uncommited migrations have been reversed and renamed. You can now migrate."
  end
end

[Ruby] On Integer puzzle

2007-10-18T14:30:47-07:00

Cool, I'm looking forward to seeing your rewrite. This code is far from 'perfect' for either aesthetics or performance.

I was hoping some StandoutJobs candidates would suggest some refactorings!

[Ruby] Dynamic migrations for rails

2007-10-12T04:27:08-07:00

How do you create a rails project if you don't know the schema in advance? ERB to the rescue!

Blogged background: http://www.danielharan.com/2007/10/12/rails-migration-hackery-with-erb/

require 'erb'
class ClientMigrator
  def self.create(fields)
    erb_migration = ERB.new <<-EOS
  class AddClients < ActiveRecord::Migration
    def self.up
      create_table :clients do |t|
        <% fields.each do |name, type| %>t.column :<%= name %>, :<%= type %>
        <% end %>
      end
    end

    def self.down
      drop_table :clients
    end
  end
EOS
    File.open("db/migrate/003_add_clients.rb", "w+") do |f|
      f.puts erb_migration.result(binding)
    end
    `rake db:migrate`
  end
end

# Example call
ClientMigrator.create(:name => :string, :phone => :string)

[Ruby] On Iffy

2007-10-11T19:37:48-07:00

Assuming you are only working with two languages...

if self.location_type.to_s == 'point'
  breaker = {'fr' => 'proximite', 'en' =>'near' }[self.lang]
else  
  breaker = {'fr' => 'dans',      'en' => 'in'  }[self.lang]
end

[Ruby] On Rubyize this : 3rd edition

2007-10-11T18:38:25-07:00

After noticing the issue with repetition, I figured we could also break it with mixing case. Here's a more robust solution ;)

def remove_insults(input)
  %w{ stupid moron dumbass retard }.each {|insult| input.gsub!(Regexp.new(insult, true),'*' * insult.length) }
  input
end
remove_insults("Stupid wabbit! Stupid, stupid, stupid!")

[Ruby] On Integer puzzle

2007-10-03T16:31:20-07:00

Here is the full solution, after some refactorings that speed it up (almost an order of magnitude) and help readability a bit.

class ITA
  
  class Number
    include Comparable

    def initialize(millions, thousands=0, units=0)
      @millions, @thousands, @units = millions, thousands, units
    end

    def value
      @millions * 1_000_000 + @thousands * 1_000 + @units
    end

    def size
      to_s.size
    end

    def to_s
      @st ||= NumberWriter.write(@millions,@thousands,@units)
    end

    def self.sorted_numbers
      @@sorted_nums ||= (0..999).collect {|num| Number.new 0,0,num}.sort.collect {|i| i.value}
    end

    def <=>(anOther)
      to_s <=> anOther.to_s
    end
  end
  
  class NumberWriter
    UNITS = { 0 => '', 1 => 'one', 2 => 'two', 3 => 'three', 4 => 'four',
              5 => 'five', 6 => 'six', 7 => 'seven', 8 => 'eight', 9 => 'nine'}

    TEENS = { 10 => "ten", 11 => "eleven", 12 => "twelve", 13 => "thirteen", 14 => "fourteen",
              15 => "fifteen", 16 => "sixteen", 17 => "seventeen",  18 => "eighteen", 19 => "nineteen" }

    TENS  = {10 => "ten", 20 => "twenty", 30 => "thirty", 40 => "forty", 50 => "fifty",
             60 => "sixty", 70 => "seventy", 80 => "eighty", 90 => "ninety" }

    def self.write(millions, thousands=0, units=0)
      (millions > 0 ?  format_units(millions) + 'million' : '' ) + 
      (thousands > 0 ? format_units(thousands) + 'thousand' : '' ) + 
       format_units(units)
    end
    
    def self.string_size(millions,thousands=0,units=0)
      (millions > 0 ?  @@sizes[millions] + 7 : 0 ) + 
      (thousands > 0 ? @@sizes[thousands] + 8 : 0 ) + 
       @@sizes[units]
    end

    def self.format_units(value)
      ret = @@formatted_units[value]
      ret
    end

    def self.format_unit(value)
      hundreds = value / 100
      st = (hundreds > 0) ? (UNITS[hundreds] + 'hundred') : ''
      remainder = value % 100
      tens    = remainder / 10
      if (tens == 1)
        st += TEENS[remainder]
      else
        st += TENS[tens*10] if (tens > 1)
        units = remainder % 10
        st += UNITS[units] if units > 0
      end
      st
    end

    @@formatted_units = (0..999).collect {|i| format_unit(i)}
    @@sizes = @@formatted_units.collect {|f| f.size}
  end
  
  class Range < Number
    include Enumerable
    
    attr_accessor :millions, :thousands
    def initialize(millions,thousands=0)
      @millions = millions
      @thousands = thousands
    end
    
    def to_s
      @st ||= NumberWriter.write(@millions, @thousands)
    end
  end

  class ThousandRange < Range
    def child_values
      @thousands * 1_000_000 + 499500
    end

    def child_sizes
      NumberWriter.string_size(0, @thousands) * 1_000 + 18440
    end

    def each
      Number.sorted_numbers.each do |i|
        yield Number.new(0, @thousands, i)
      end
    end
  end

  class MixedRange < Range
    def child_values
      @millions * 1_000_000_000 + @thousands * 1_000_000 + 499500
    end

    def child_sizes
      NumberWriter.string_size(@millions, @thousands) * 1_000 + 18440
    end

    def each
      Number.sorted_numbers.each do |i|
        yield Number.new(@millions, @thousands, i)
      end
    end
  end

  class MillionRange < Range
    def child_values
      @millions * 1_000_000_000_000 + 499_999_500_000
    end

    def child_sizes
      NumberWriter.string_size(@millions) * 1_000_000 + 44_872_000
    end
    
    def children
      ((Number.sorted_numbers - [0]).collect {|i| MixedRange.new(@millions, i) } << 
        [MixedRange.new(@millions, 0).to_a]).flatten
    end

    def each
      children.sort.each do |i|
        yield i
      end
    end
  end
  
end

@target = 51_000_000_000
@sum = @size = 0

# recursion isn't nice to puts
def echo(msg)
  puts msg
end

def seek(elements)
  elements.each do |e|
    if e.class== ITA::Number
      @sum  += e.value
      @size += e.size
    else
      if ((@size + e.child_sizes) > @target)
        seek(e) # recurse to avoid overshooting target
      else
        @sum  += e.child_values
        @size += e.child_sizes
      end
    end
    if @size == @target
      echo "number:" + e.to_s
      echo "sum: " + @sum.to_s
      exit
    end
  end
end

seek((1..999).collect {|i| [ITA::Number.new(0,0,i), ITA::MillionRange.new(i), ITA::ThousandRange.new(0,i)]}.flatten.sort)