我该如何改进?

我想知道是否有人可以指出一种更清洁的更好的方式来编写粘贴在这里的代码。 代码从yelp中删除一些数据并将其处理为json格式。 我没有使用hash.to_json的原因是因为它抛出某种堆栈错误,我只能假设是由于散列太大(它不是特别大)。

  • 响应对象=哈希
  • text =保存到文件的输出

无论如何指导赞赏。

 def mineLocation client = Yelp::Client.new request = Yelp::Review::Request::GeoPoint.new(:latitude=>13.3125,:longitude => -6.2468,:yws_id => 'nicetry') response = client.search(request) response['businesses'].length.times do |businessEntry| text ="" response['businesses'][businessEntry].each { |key, value| if value.class == Array value.length.times { |arrayEntry| text+= "\"#{key}\":[" value[arrayEntry].each { |arrayKey,arrayValue| text+= "{\"#{arrayKey}\":\"#{arrayValue}\"}," } text+="]" } else text+="\"#{arrayKey}\":\"#{arrayValue}\"," end } end end 

看起来你的所有代码最终都是这样的:

 require 'json' def mine_location client = Yelp::Client.new request = Yelp::Review::Request::GeoPoint.new(latitude: 13.3125, longitude: -6.2468, yws_id: 'nicetry') response = client.search(request) return response['businesses'].to_json end 

哪个适合我。

如果由于某种原因你真的必须编写自己的JSON发射器实现,这里有几个提示。

您在代码中完全忽略的第一件事是Ruby是一种面向对象的语言,更具体地说是一种基于类的面向对象语言。 这意味着通过构建通过消息传递相互通信的对象网络来解决问题,并通过执行这些对象所属的类中定义的方法来响应这些消息。

这给了我们很多力量:动态调度,多态,封装和其他许多function。 利用这些,您的JSON发射器看起来像这样:

 class Object def to_json; to_s end end class NilClass def to_json; 'null' end end class String def to_json; %Q'"#{to_s}"' end end class Array def to_json; "[#{map(&:to_json).join(', ')}]" end end class Hash def to_json; "{#{map {|k, v| "#{k.to_json}: #{v.to_json}" }.join(', ')}}" end end 

mine_location看起来就像上面一样,除了显然没有require 'json'部分。

如果你想要你的JSON格式很好,你可以尝试这样的事情:

 class Object def to_json(*) to_s end end class String def to_json(*) inspect end end class Array def to_json(indent=0) "[\n#{' ' * indent+=1}#{ map {|el| el.to_json(indent) }.join(", \n#{' ' * indent}") }\n#{' ' * indent-=1}]" end end class Hash def to_json(indent=0) "{\n#{' ' * indent+=1}#{ map {|k, v| "#{k.to_json(indent)}: #{v.to_json(indent)}" }.join(", \n#{' ' * indent}") }\n#{' ' * indent-=1}}" end end 

实际上在这段代码中没有特定于Ruby的东西。 这正是解决方案在任何其他基于类的面向对象语言(如Java)中的样子。 它只是面向对象的设计101。

唯一特定于语言的是如何“修改”类并向其添加方法。 在Ruby或Python中,您只需修改类。 在C#和Visual Basic.NET中,您可能会使用扩展方法,在Scala中您将使用隐式转换,在Java中可能使用Decorator设计模式。

您的代码的另一个巨大问题是您正在尝试解决一个显然是递归的问题,而实际上并没有递归。 这只是行不通。 您编写的代码基本上是Fortran-57代码:没有对象且没有递归的过程。 即使只是从Fortran向上移动一步,比如Pascal,也会给你一个很好的递归程序解决方案:

 def jsonify(o) case o when Hash "{#{o.map {|k, v| "#{jsonify(k)}: #{jsonify(v)}" }.join(', ')}}" when Array "[#{o.map(&method(:jsonify)).join(', ')}]" when String o.inspect when nil 'null' else o.to_s end end 

当然,你可以在这里用缩进玩同一个游戏:

 def jsonify(o, indent=0) case o when Hash "{\n#{' ' * indent+=1}#{ o.map {|k, v| "#{jsonify(k, indent)}: #{jsonify(v, indent)}" }.join(", \n#{' ' * indent}") }\n#{' ' * indent-=1}}" when Array "[\n#{' ' * indent+=1}#{ o.map {|el| jsonify(el, indent) }.join(", \n#{' ' * indent}") }\n#{' ' * indent-=1}]" when String o.inspect when nil 'null' else o.to_s end end 

这是puts mine_location的缩进输出,使用puts mine_location的第二个(缩进版)或to_json的第二个版本jsonify ,它并不重要,它们都具有相同的输出:

 [ { "name": "Nickies", "mobile_url": "http://mobile.yelp.com/biz/yyqwqfgn1ZmbQYNbl7s5sQ", "city": "San Francisco", "address1": "466 Haight St", "zip": "94117", "latitude": 37.772201, "avg_rating": 4.0, "address2": "", "country_code": "US", "country": "USA", "address3": "", "photo_url_small": "http://static.px.yelp.com/bpthumb/mPNTiQm5HVqLLcUi8XrDiA/ss", "url": "http://yelp.com/biz/nickies-san-francisco", "photo_url": "http://static.px.yelp.com/bpthumb/mPNTiQm5HVqLLcUi8XrDiA/ms", "rating_img_url_small": "http://sofzh.miximages.com/ruby/stars_small_4.png", "is_closed": false, "id": "yyqwqfgn1ZmbQYNbl7s5sQ", "nearby_url": "http://yelp.com/search?find_loc=466+Haight+St%2C+San+Francisco%2C+CA", "state_code": "CA", "reviews": [ { "rating": 3, "user_photo_url_small": "http://static.px.yelp.com/upthumb/ZQDXkIwQmgfAcazw8OgK2g/ss", "url": "http://yelp.com/biz/yyqwqfgn1ZmbQYNbl7s5sQ#hrid:t-sisM24K9GvvYhr-9w1EQ", "user_url": "http://yelp.com/user_details?userid=XMeRHjiLhA9cv3BsSOazCA", "user_photo_url": "http://static.px.yelp.com/upthumb/ZQDXkIwQmgfAcazw8OgK2g/ms", "rating_img_url_small": "http://sofzh.miximages.com/ruby/stars_small_3.png", "id": "t-sisM24K9GvvYhr-9w1EQ", "text_excerpt": "So I know gentrification is supposed to be a bad word and all (especially here in SF), but the Lower Haight might benefit a bit from it. At least, I like...", "user_name": "Trey F.", "mobile_uri": "http://mobile.yelp.com/biz/yyqwqfgn1ZmbQYNbl7s5sQ?srid=t-sisM24K9GvvYhr-9w1EQ", "rating_img_url": "http://sofzh.miximages.com/ruby/stars_3.png" }, { "rating": 4, "user_photo_url_small": "http://static.px.yelp.com/upthumb/Ghwoq23_alkaXawgqj7dBA/ss", "url": "http://yelp.com/biz/yyqwqfgn1ZmbQYNbl7s5sQ#hrid:8xTNOC9L5ZXwGCMNYY-pdQ", "user_url": "http://yelp.com/user_details?userid=4F2QG3adYIUNXplqqp9ylA", "user_photo_url": "http://static.px.yelp.com/upthumb/Ghwoq23_alkaXawgqj7dBA/ms", "rating_img_url_small": "http://sofzh.miximages.com/ruby/stars_small_4.png", "id": "8xTNOC9L5ZXwGCMNYY-pdQ", "text_excerpt": "This place was definitely a great place to chill. The atmosphere is very non-threatening and very neighborly. I thought it was cool that they had a girl dj...", "user_name": "Jessy M.", "mobile_uri": "http://mobile.yelp.com/biz/yyqwqfgn1ZmbQYNbl7s5sQ?srid=8xTNOC9L5ZXwGCMNYY-pdQ", "rating_img_url": "http://sofzh.miximages.com/ruby/stars_4.png" }, { "rating": 5, "user_photo_url_small": "http://static.px.yelp.com/upthumb/q0POOE3vv2LzNg1qN8MMyw/ss", "url": "http://yelp.com/biz/yyqwqfgn1ZmbQYNbl7s5sQ#hrid:pp33WfN_FoKlQKJ-38j_Ag", "user_url": "http://yelp.com/user_details?userid=FmcKafW272uSWXbUF2rslA", "user_photo_url": "http://static.px.yelp.com/upthumb/q0POOE3vv2LzNg1qN8MMyw/ms", "rating_img_url_small": "http://sofzh.miximages.com/ruby/stars_small_5.png", "id": "pp33WfN_FoKlQKJ-38j_Ag", "text_excerpt": "Love this place! I've been here twice now and each time has been a great experience. The bartender is so nice. When we had questions about the drinks he...", "user_name": "Scott M.", "mobile_uri": "http://mobile.yelp.com/biz/yyqwqfgn1ZmbQYNbl7s5sQ?srid=pp33WfN_FoKlQKJ-38j_Ag", "rating_img_url": "http://sofzh.miximages.com/ruby/stars_5.png" } ], "phone": "4152550300", "neighborhoods": [ { "name": "Hayes Valley", "url": "http://yelp.com/search?find_loc=Hayes+Valley%2C+San+Francisco%2C+CA" } ], "rating_img_url": "http://sofzh.miximages.com/ruby/stars_4.png", "longitude": -122.429926, "categories": [ { "name": "Dance Clubs", "category_filter": "danceclubs", "search_url": "http://yelp.com/search?find_loc=466+Haight+St%2C+San+Francisco%2C+CA&cflt=danceclubs" }, { "name": "Lounges", "category_filter": "lounges", "search_url": "http://yelp.com/search?find_loc=466+Haight+St%2C+San+Francisco%2C+CA&cflt=lounges" }, { "name": "American (Traditional)", "category_filter": "tradamerican", "search_url": "http://yelp.com/search?find_loc=466+Haight+St%2C+San+Francisco%2C+CA&cflt=tradamerican" } ], "state": "CA", "review_count": 32, "distance": 1.87804019451141 } ] 

我注意到的第一件事就是你的使用

 response['businesses'].length.times do |i| # the business you want is response['businesses'][i] end 

用于迭代。 使用Array.each可以大大简化这一过程,它为您提供:

 response['businesses'].each do |businessEntry| # here, businessEntry is the actual object, so forego something like # response['business'][businessEntry], just use businessEntry directly end 

你实际上在下面做了同样的事情:response [‘business’] [businessEntry] .each

关于样式的注意事项,如果块在多行上,常常(虽然没有强制执行)常用样式使用do / end,如果块是一行,则使用{}。

另外,不确定为什么要在字符串中构建这些键/值对,只需要进行哈希。 然后你可以很容易地转换为json,或者正如Jorg指出的那样,只需将整个响应转换为json,它确实可以手动完成你所做的…除非你需要先处理数据(它看起来不像你需要做)

我有兴趣看到你从hash.to_json得到的错误,看看是否可以找到原因。

就Ruby代码而言,有几点意见:

  • 当你可以使用each例如response['businesses'].each do时,使用.length.times做..有点奇怪response['businesses'].each do ..

  • 在你的else情况下, text+="\"#{arrayKey}\":\"#{arrayValue}\","看起来像arrayKeyarrayValue超出范围,因为它们仅用作上面each的块变量。

  • text =""在外观的每次迭代中将文本设置回空字符串,因此代码看起来就像丢弃了循环的前一次迭代构建的文本。

我不是Ruby的专家,但我知道如果它们出现在我的代码中会发生什么,我的教授会对我大吼大叫。 Mikej已经遇到了重大问题,特别是使用#each而不是#length#times

如果我正在迭代某种类型的集合,我唯一一次使用#each以外的东西就是当我需要使用自定义迭代器时,即使这样你仍然可以使用#each ,但你必须有一个传递块内的控制语句,以确保该块不在某些实例上执行。 即使它变得太复杂,我真正使用的唯一其他自定义迭代方法是for i in Range.new( begin, end, optional_exclusion )语句中的for i in Range.new( begin, end, optional_exclusion ) 。 这仍然可以转换为#each的块中的条件,但它有时会保存代码并使其更明确,我故意不在集合的所有元素上执行代码以及明确显示我正在设置进入循环时受影响元素的边界,而不是硬编码值或整个集合。

当在if语句的else部分调用arrayKey和arrayValue时,Mikej已经指出了范围的错误,所以我不打扰它。 他也已经指出,您应该将text =""代码行向上移动两行以退出响应代码块范围。

之后我唯一担心的是一些问题,不是代码本身,而是更多类型的编码风格和Ruby社区中普遍使用的东西。 所以这些建议绝不是你必须采取的。 它只是使阅读Ruby代码更容易。

所以我的第一个建议是每当你调用一个方法来传递一个块时,除非你能够适当缩进,对象调用,方法调用,块变量声明,代码块和块关闭所有在同一行,然后不要使用花括号。 在多行代码块的情况下,请使用关键字doend

我的第二个建议是使用适当的缩进,在Ruby语言中是两个空格而不是在许多其他语言的编码风格中找到的正常缩进。 你有一个适当的缩进代码,然后你搞砸了,这种情况造成一些线路看起来像是在他们不在的范围级别。 现在,这个提示远不是编码风格标准,但我确保我在正确的范围级别的一般技巧只是在使用end关键字后立即添加一个结束注释并放入范围的名称它刚关闭。 它已经从一些范围错误中拯救了我,但我从来没有听说过其他任何人这样做过,它可能会使用这些随机的终点评论来混淆代码,但这是一种很好的方法。

我的第三个建议是改进你对字符串和符号的使用。 我几乎不敢这样说,因为我仍然需要提高我对Ruby中符号的理解,我不记得在任何最近的脚本中使用Yelp类,所以我对这个问题视而不见。 但是,看起来您使用字符串'businesses'就像Hash键一样。 Ruby中的一般规则是,如果你只是为它所代表的字符串使用一个字符串,那么你应该使用一个符号,而如果你使用一个字符串,因为你实际上需要它的字符内容,那么你应该坚持使用一个字符串。 这只是因为每次调用字符串文字时,都会生成一个新字符串并将其存储在系统内存中。 所以在这里,因为你在内部和每个块的每个块内使用'businesses' ,你可能会分配该字符串O(n²)次(尽管内部块的分配在下一次迭代期间被垃圾收集。而如果你使用像’:BUSINESS’这样的符号,它只初始化一次。对于text ="" line,你应该将它修改为单引号字符串文字.Ruby解释器可以更快地解析单引号字符串文字而不是双引号,所以通常如果你可以使用单引号字符串文字,那么这样做。

我得到了所有的建议。 随心所欲地带上它们或想要它们。 假设你接受我的建议并且我没有在这个过程中破坏任何东西,那么这也就是你的代码会是什么样子。

 def mineLocation client = Yelp::Client.new request = Yelp::Review::Request::GeoPoint.new(:latitude=>13.3125, :longitude => -6.2468, :yws_id => 'nicetry') response = client.search(request) text = '' response[:businesses].each do |businessEntry| response[:businesses][businessEntry].each do |key, value| if value.kindOf( Array ) value.each do |arrayEntry| text += "\"#{key}\":[" value[arrayEntry].each do |arrayKey, arrayValue| text += "{\"#{arrayKey}\":\"#{arrayValue}\"}," end #each text += ']' end #each else # Didn't fix because I didn't know you intentions here. text += "\"#{arrayKey}\":\"#{arrayValue}\"," end #if end #each end #each end #def 

我没有用符号替换'nicetry'只是因为我不知道Yelp类是如何工作的,并且可能明确需要一个字符串而不是符号。 我也不知道预期的代码效果是什么,因为这个代码执行的唯一时间是变量超出范围,所以我无法知道你在该行中引用了什么。 特别是从那时起你的价值也不是一个数组。

我知道这是一个很长的答案,但我希望其中一些有帮助!