-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1

Some stuff I noticed in the code that I collected as tips :-)

- - obj == nil -> obj.nil?

- - str.length > 0 -> str.empty?

- - if str && str.length > 0 This is tricky, not only because str.length should be tested with empty, but this test is so common in web forms, that rails adds the method Object#blank? so one can test nil? and empty? in one shot. -> str.blank?

- - comments next to 'end', if we need to comment next to an end to give a hint to which block it belongs, it means we have to refactor that code and split it/un-nest it. If you are commenting an end block, you need then to feel guilty. Your face has to go red and you make a concrete appointment to fix that code ;-)

- - Something like:

if not @user.home_directory.blank? command << "home=#{@user.home_directory}" end

(already refactored to use blank?)

can be refactored to:

command << "home=#{@user.home_directory}" if not @user.home_directory.blank?

This is not very important, but if you have 20 of those blocks in a row, the continuous starts and end of one line blocks make code too verbose and eyes have to indent and unindent all the time, while a one liner reads as english.

- - Use and learn stdlib methods:

if not @user.groups.blank? counter = 0 grp_string = "" @user.groups.each do |group| if counter == 0 grp_string = group[:id] else grp_string += ",#{group[:id]}" end counter += 1 end command << "grouplist=#{grp_string}" end

What are we doing here? just converting an array into a string, but with commas. So first we are duplicating Array#join. I guess the code does that because it has a collection of Group objects, and it want to create an array of ids of those groups.

You can get [1,2,3..] as the array of the group ids by using Array#map: groups.map { |group| group[:id] }. Add Array#join to the equation: groups.map { |group| group[:id] }.join(',') and you have all the code above in one line! :-) it returns "1,2,3" as a String.

I fixed most of the issues mentioned, but sure there are more. Fix them as you spot them.

cheers Duncan