-----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
Some more ruby tips date parsing:
@time_string = @systemtime.currenttime.hour.to_s @time_string << ":" @time_string << @systemtime.currenttime.min.to_s
->
@time_string = @systemtime.currenttime.strftime("%H:%M")
The other way around is also valid, instead of manually splitting the time, just specfy the format:
time_array = params[:currenttime].split ":" t.currenttime = DateTime.civil(params[:year].to_i, params[:month].to_i, params[:day].to_i, time_array[0].to_i, time_array[1].to_i)
->
t.currenttime = DateTime.parse("#{params[:currentdate]} #{:currenttime}", "%d/%m/%Y %H:%M")