wxruby users - Aug 2005 - listbook.rb patch

Hopefully a good listbook.rb patch


_______________________________________________
wxruby-users mailing list
wxruby-users@rubyforge.org
http://rubyforge.org/mailman/listinfo/wxruby-users

Roy Sutton wrote:
> Hopefully a good listbook.rb patch
Sadly, no.

kevins@aria:~/work/wxruby2$ patch -p1 </download/patches/listbook.rb.patch
missing header for unified diff at line 3 of patch
patching file samples/listbook/listbook.rb
Hunk #1 FAILED at 64.
Hunk #2 FAILED at 84.
Hunk #3 FAILED at 111.
3 out of 3 hunks FAILED -- saving rejects to file 
samples/listbook/listbook.rb.rej
kevins@aria:~/work/wxruby2$



Looking at the patch itself, I have a couple comments/questions/concerns:
> --- wxruby2/samples/listbook/listbook.rb	2005-08-19 20:35:00.000000000
-0400
> +++ listbook.rb	2005-08-21 12:29:40.518958400 -0400
> @@ -64,15 +64,15 @@
>      evt_button(Wx::xrcid(''ID_PIZZA_BUTTON'')) do |event|
>        #get selections and add to order
>        order_string = @text_output.get_value
> -      if order_string != "" then order_string <<
"\n" end
> +      if order_string != "" then order_string = "\n"
end
This seems like a confusing way to do this. Could we instead say:

    if @text_output.get_value.length == 0
       order_string = ""
    else
       order_string = "\n"
    end

Or even better, could we just always append a blank line at the end of 
the order string, instead of conditionally adding it at the beginning?
> -      @pizza_toppings.each { |obj| order_string <<
obj.get_string_selection + "\n" }
> +      @pizza_toppings.each { |obj| order_string <<
obj.get_string_selection + "\n" unless obj.get_string_selection ==
"" }
Personal bias on my part, but I *hate* the "unless" keyword. I always 
find it incredibly jarring, since mentally I have already executed the 
first part, and then find out that it was conditional.

I certainly won''t reject the patch because of the unless, because
it''s
in a sample. But I probably will reject any unless''s in the core
wxruby2
code.

Kevin

I agree with you.  I was just trying to impact the code as minimally as 
possible.  I can always refactor the whole thing later.

Roy

Kevin Smith wrote:
>
> This seems like a confusing way to do this. Could we instead say:
>
>    if @text_output.get_value.length == 0
>       order_string = ""
>    else
>       order_string = "\n"
>    end
>
> Or even better, could we just always append a blank line at the end of 
> the order string, instead of conditionally adding it at the beginning?
>
>> -      @pizza_toppings.each { |obj| order_string << 
>> obj.get_string_selection + "\n" }
>> +      @pizza_toppings.each { |obj| order_string << 
>> obj.get_string_selection + "\n" unless
obj.get_string_selection == "" }
>
>
> Personal bias on my part, but I *hate* the "unless" keyword. I
always
> find it incredibly jarring, since mentally I have already executed the 
> first part, and then find out that it was conditional.
>
> I certainly won''t reject the patch because of the unless, because
it''s
> in a sample. But I probably will reject any unless''s in the core 
> wxruby2 code.
>
> Kevin
> _______________________________________________
> wxruby-users mailing list
> wxruby-users@rubyforge.org
> http://rubyforge.org/mailman/listinfo/wxruby-users
>
>
>