0

私は最近 Ruby の仕事に応募し、「"query_to_hash" という名前の Ruby 関数を作成するメソッドをコーディングするように求められました。この関数は、HTTP クエリ文字列 (例: foo=bar&abc=1%202%203) として単一の文字列引数を取り、次のような名前と値のペアのハッシュを返します。 :

 {"foo" => "bar", "abc" => "1 2 3"}

添付のコード例を提供したところ、Ruby の私のスタイルは彼らが望んでいたものではないというフィードバックがありました。

開発者が添付ファイルでどのようなスタイルの問題を目にするか知りたいので、建設的なフィードバックをいただければ幸いです。

require 'rubygems'
require 'uri'
require 'rack'

include Rack::Utils

$SAFE = 3

# HTTP Query string (from wikipedia)

#field1=value1&field2=value2&field3=value3...
# The query string is composed of a series of field-value pairs.
# Within each pair, the field name and value are separated by an equals sign. The equals sign may be omitted if the value is an empty string.
# The series of pairs is separated by the ampersand, '&' (or semicolon, ';' for URLs embedded in HTML and not generated by a <form>...</form>; see below).
# While there is no definitive standard, most web frameworks allow multiple values to be associated with a single field[3][4]:

# field1=value1&field1=value2&field1=value3...

def query_to_hash(qry, sep = '&')

  # assume input string conforms to spec (no validation)
  # assume only & or ; is used - not both
  # return a null string if value is not defined
  # return null hash if query is null string
  # return array of values in hash if field has multiple values

  #@qry = qry.gsub(/%20/, " ")
  @qry = URI.unescape(qry)

  rtn = Hash.new {|h,k| h[k]=[]}

  if @qry == "" then 
  # return an empty hash
  #
    return {}
  else
      qry_a = @qry.split(sep)     
  end

  qry_a.each do |fv_pair|
    pair = fv_pair.split('=')

    # append multiple values if needed and ensure that null values are accommodated
    #
    rtn[pair[0]] << pair[1] ||= ""
  end 

  # collapse array if it contains only one item
  #
  rtn.each{|k,v| rtn[k] = *v if v.length == 1}

end



puts "Using 'query_to_hash' method:"
puts
test_values  = %w[foo=bar&abc=1%202%203 
                  foo&abc=1%202%203
                  foo=&abc=1%202%203
                  foo=bar&foo=boo&abc=1%202%203
                 ]

test_values.each { |v| puts "#{sprintf("%30s",v)} is returned as #{query_to_hash(v).inspect}" }           

test_values = %w[ foo=bar;foo=boo;abc=1%202%203
                  foo=bar;foo=boo;abc=1%0A2%203
                  foo=bar;foo=boo;abc=1%092%0D3
                 ]

test_values.each { |v| puts "#{sprintf("%30s",v)} is returned as #{query_to_hash(v, ';').inspect}" }       

puts "#{sprintf("%30s", "null string")} is returned as #{query_to_hash("").inspect}"

# compare with Rack::Utils::parse_query
#
puts
puts "Using 'Rack::Utils::parse_query' method:"
puts
test_values  = %w[foo=bar&abc=1%202%203 
                  foo&abc=1%202%203
                  foo=&abc=1%202%203
                  foo=bar&foo=boo&abc=1%202%203
                 ]

test_values.each { |v| puts "#{sprintf("%30s",v)} is returned as #{parse_query(v).inspect}" }           

test_values = %w[ foo=bar;foo=boo;abc=1%202%203
                  foo=bar;foo=boo;abc=1%0A2%203
                  foo=bar;foo=boo;abc=1%092%0D3
                 ]

test_values.each { |v| puts "#{sprintf("%30s",v)} is returned as #{parse_query(v, ';').inspect}" }       

puts "#{sprintf("%30s", "null string")} is returned as #{parse_query("").inspect}"
4

2 に答える 2

1

「これは間違っている」と私に叫ぶものは何も見えません。あなたのコードは十分にコメントされており、非常に明確であり、コアの言語構造と基本的な API を把握しています。

具体的な理由が示されましたか、それとも単に「あなたのスタイルが気に入らない」というキャッチフレーズを使用しただけですか?

免責事項: 私は Ruby に移行した Java 開発者であるため、適切な Ruby の「スタイル」のイメージを思い浮かべていない可能性がありますが、私たちのシステムにははるかに悪いコードがあると言えます。

于 2013-02-04T12:26:42.637 に答える
1

本当に私に飛びついたのは2つだけです。

qry_a = @qry.split(sep)

上記の行の変数はローカルであり、else句内でのみ存在しますが、後で再度参照します。

@qry = URI.unescape(qry)

オブジェクトがない限り、インスタンス変数を使用する必要はありません。メソッドを超えて変数のスコープがすぐに開かれるため、問題になることをお勧めします。個人的には、できるだけ地元の人を利用するようにしています。

それを除けば、私には問題ないようです。テストに Minitest や RSpec などのテスト フレームワークを使用することもできたかもしれませんが、スタイル的には問題ないようです。

より具体的なポイントがあなたにとってより役立つだろうという@mcfinniganに同意します。

追加します (少し性急だったので)、配列インデックスを直接使用することも、すぐに混乱するため、あまり優れたスタイルではありません。たとえば、これは Perl ie のライン ノイズのように見えます :)

rtn[pair[0]] << pair[1] ||= ""

また、副作用として値に影響を与えるのではなく、明示的に値に影響eachを与えるため、何かのようなmap、またはselectより良い可能性がある場合に、値に影響を与えることもありました。明確で明示的であることは、Ruby の良いスタイルです (IMO)。

もう 1 つ考えてみると… (コーヒーが入ってきました :) これは就職の面接だったので、おそらく彼らはこれ以上の何かを探していたのでしょう。たとえば、仕様を見て最初に思ったのは、「なぜ文字列を引数として渡すのか」ということでした。あなたは次のようなことをした方が良いかもしれません：

class String
  def query_to_hash( separator="&" )
    # more code

そして、それを from から呼び出すことができます。"foo=bar&abc=1%202%203".query_to_hashこれは、はるかに Ruby っぽいです。

モンキーパッチ、継承よりもさらに優れています（見過ごされがちです）。

class QueryString < String
  def to_hash( separator="&" )
    # some code that refers to `self`

そして、次の方法で使用できます。

QueryString.new("foo=bar&abc=1%202%203").to_hash

これにより、読みやすく理解しやすくなり、何を達成しようとしているのかがより明確になります。これを行う他の方法もあり、おそらくそれがあなたを群衆から引き離すものです.

于 2013-02-04T12:33:32.887 に答える