-----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 -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.9 (GNU/Linux) Comment: Using GnuPG with SUSE - http://enigmail.mozdev.org iEYEARECAAYFAkn0uTQACgkQzR62qWZ+QtEkLgCfVSPfrWXY88ZBG3Wrxj5b/Hnr pKEAoJTQPD9/+3DF27rQa1KVNoOxp6ke =erdq -----END PGP SIGNATURE----- -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org For additional commands, e-mail: yast-devel+help@opensuse.org