Suggestions for refactoring this ugly code?

Ruby
1

I my code below could be better. I know enough to know its ugly, not
quite enough as to why its ugly.

I’m not sure if these are valid “smells” for refactoring: It bothers me
that I have so many exit points in one loop, not sure why. Having 3 if
statements all looking very similar bothers me. Not being able to
express each if statement on one simply line each also bothers me.

Anyone got any pointers on how I can refactor and make it better?

…

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
}

pngmaker.cleanup

…

thanks in advance, michelle

2

On Tue, Aug 16, 2011 at 5:41 PM, Michelle P.
[email protected] 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.

}
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