From: Robert Klemme <shortcutter@...>
Date: 2011-08-17T06:58:37+09:00
Subject: Re: Code refactoring: making my method prettier

On Tue, Aug 16, 2011 at 5:41 PM, Michelle Pace
<michelle@michellepace.com> wrote:
> 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

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