Code refactoring: making my method prettier

ruby-talk
1

Hi there, i'm looking at my below code. It doesn't seem pretty at all. I
am sure there must be a better way to achieve the same functionality.

Could anyone please provide some pointers as to how I could refactor?

cards = results.root.elements

pngmaker = PngMaker.new(100,100)

cards.each{ |card|
  card = Card.new(card)
  next if (card.drawing).to_s.empty?

  found_tif = Dir.glob(File.join(@search_path, '**',
"#{card.drawing}#{TIF}"), File::FNM_CASEFOLD).first

  if (!found_tif)
    puts "#{card.number} couldn't find #{card.drawing}#{TIF} in:
#{@search_path}"
    next
  end

  new_png = pngmaker.make_png_copy_of(found_tif)
  if (!new_png)
    puts "#{card.number} couldn't make png copy of: #{found_tif}"
    next
  end

  if (!card.attach_to_card_descrip(card.number, new_png))
    puts "#{card.number} couldn't attach: #{new_png}"
    next
  end
}

···

--
Posted via http://www.ruby-forum.com/.

2

Hi there, i'm looking at my below code. It doesn't seem pretty at all. I
am sure there must be a better way to achieve the same functionality.

Could anyone please provide some pointers as to how I could refactor?

There is a whole lot of stuff missing (class Card for example) which
probably makes it hard to understand what's going on.

cards = results.root.elements

pngmaker = PngMaker.new(100,100)

cards.each{ |card|
card = Card.new(card)

This is a bad idea: you overwrite the block variable with a new value.
This may lead to confusion and subtle bugs. Also, if you intend to
modify cards this code won't do what you probably expect.

next if (card.drawing).to_s.empty?

found_tif = Dir.glob(File.join(@search_path, '**',
"#{card.drawing}#{TIF}"), File::FNM_CASEFOLD).first

if (!found_tif)
puts "#{card.number} couldn't find #{card.drawing}#{TIF} in:
#{@search_path}"
next
end

new_png = pngmaker.make_png_copy_of(found_tif)
if (!new_png)
puts "#{card.number} couldn't make png copy of: #{found_tif}"
next
end

if (!card.attach_to_card_descrip(card.number, new_png))
puts "#{card.number} couldn't attach: #{new_png}"
next
end
}

I prefer to use less "next" in these cases but instead use structuring
of the code with "if else end". Then it is easier to see what's
happening via the indentation. The "next"s can be easily overlooked.

Kind regards

robert

···

On Tue, Aug 16, 2011 at 5:41 PM, Michelle Pace <michelle@michellepace.com> wrote:

--
remember.guy do |as, often| as.you_can - without end
http://blog.rubybestpractices.com/