Don't print query params if they are empty

[paulcsmith]

I found this while investigating this issue on Lucky:
luckyframework/lucky#214

It turns out that because I parsed the query params in the handler mentioned in the above link, it
set the URI#query to "". This caused HTTP::Request#resource to add
an extra ? to the end of the path, even though the query params were
empty.

This PR fixes the issue by also checking for an empty string before
printing ?

[Sija]

&& [email protected]?

[paulcsmith]

Good idea!

[straight-shoota]

Better: if (query = @query) && !query.empty?

[paulcsmith]

I like that 👍

[luislavena]

I would personally prefer move the assign and condition to the left side (a regular if)

if (query = @query) && !query.empty?
  str << "?" << query
end

[masukomi]

You should probably replace this with the following which is much clearer as to your intent.

str << "?" << @query if !@query.to_s.empty?

More importantly since @query is literally the same thing as query owing to the getter setting it to itself is just weird. I wanted to see if there was some dark magic I was ignorant of, but as far as i can tell this assignment makes no sense.

class Foo
  getter x
  setter x
  def initialize(@x : String?)
  end
  def test : Bool
    (x = @x) ? true : false
  end
  def test2 : Bool
    x ? true : false
  end
  def test3 : Bool
     @x ? true : false
  end

end

then checking it in icr...

icr(0.23.1) > f = Foo.new("something")
 => #<Foo:0x107231e60 @x="something">
icr(0.23.1) > f.test
 => true
icr(0.23.1) > f.test2
 => true
icr(0.23.1) > f.test3
 => true
icr(0.23.1) > f = Foo.new(nil)
 => #<Foo:0x1070c8e40 @x=nil>
icr(0.23.1) > f.test
 => false
icr(0.23.1) > f.test2
 => false
icr(0.23.1) > f.test3
 => false

[paulcsmith]

@masukomi Thanks for the feedback and the detailed response. That example is not quite right for showing the problem. Here's one that illustrates the issue: https://play.crystal-lang.org/#/r/36hx

class Foo
  @query : String?
  
  def full_path
    String.build do |str|
      str << @query if @query && @query.empty?
    end
  end
end

Foo.new.full_path

Results in:

in line 6: undefined method 'empty?' for Nil (compile-time type is (String | Nil))

I wish I could find the documentation for this, but the gist is that instance variable can change at any time so you need to set it to a local variable first so the compiler can be sure that it is still not nil. Does that make sense?

[email protected]_s.empty? would work, but it requires making an intermediate string object if the query is nil, which IMO is not worth it for slightly improved readability

[masukomi]

You should probably replace this with the following which is much clearer as to your intent.

str << "?" << @query if !@query.to_s.empty?

More importantly since @query is literally the same thing as query owing to the getter setting it to itself is just weird. I wanted to see if there was some dark magic I was ignorant of, but as far as i can tell this assignment makes no sense.

class Foo
  getter x
  setter x
  def initialize(@x : String?)
  end
  def test : Bool
    (x = @x) ? true : false
  end
  def test2 : Bool
    x ? true : false
  end
  def test3 : Bool
     @x ? true : false
  end

end

then checking it in icr...

icr(0.23.1) > f = Foo.new("something")
 => #<Foo:0x107231e60 @x="something">
icr(0.23.1) > f.test
 => true
icr(0.23.1) > f.test2
 => true
icr(0.23.1) > f.test3
 => true
icr(0.23.1) > f = Foo.new(nil)
 => #<Foo:0x1070c8e40 @x=nil>
icr(0.23.1) > f.test
 => false
icr(0.23.1) > f.test2
 => false
icr(0.23.1) > f.test3
 => false

[paulcsmith]

I think this is pretty clean and eliminates the need to create an intermediate variable. I think this is a pretty small change, is reasonably easy to understand, and is unlikely to change much in the future. So it's probably ok like this. However, if it needs to change to change to be merged in, I'm happy to change it!

[straight-shoota]

This won't work: if @query is nil, @query.try(&.empty?) will return nil which is falsey and thus add a question mark. You need add a spec for this case.

And I still think my previous proposal (query = @query) && !query.empty? is probably the best boolean expression for this, but maybe put it in a proper if branch to spare an additional ivar lookup:

if (query = @query) && !query.empty?
  str << '?' << query
end

[RX14]

this please

[paulcsmith]

Yeah I noticed this didn't work and updated it last night. Did you see the latest change using try https://github.com/crystal-lang/crystal/pull/5340/files? I think it is easier to understand, but maybe that's just me. @RX14 if you'd still prefer assigning to the intermediate variable instead of the try that I used, LMK and I'll change it.

[luislavena]

I personally think if branching is easier to understand over a .try block, but performance wise, a try block might show better performance than if branch.

https://gist.github.com/luislavena/4096693021ca38f6be4c7539625716a3

The difference might be marginal, so leave that decision to core in relation to readability/simplicity vs performance.

Cheers.

[paulcsmith]

I switched it to what @straight-shoota suggested: #5340 (comment)

[bcardiff]

Thanks @paulcsmith !