Comment puis-je améliorer?

Question

Je me demandais si quelqu'un pourrait trouver un meilleur moyen d'écrire mon code qui est collé ici. Le code récupère certaines données de yelp et les traite dans un format json. La raison pour laquelle je n'utilise pas hash.to_json est parce qu'il génère une sorte d'erreur de pile que je ne peux que supposer due au fait que le hash est trop grand (ce n'est pas particulièrement grand).

• objet Response = un hachage
• text = la sortie qui permet d'économiser de déposer

Anyways apprécié des conseils.

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 

Answer 1

Il ressemble à tout votre code est en train de faire est en fin de compte ceci:

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 

Ce qui fonctionne très bien pour moi.

Si, pour une raison ou une autre, vous devez vraiment écrire votre propre implémentation d'un émetteur JSON, voici quelques conseils pour vous. Le numéro 1 que vous ignorez complètement dans votre code est que Ruby est un langage orienté objet, plus spécifiquement un langage orienté objet basé sur la classe. Cela signifie que les problèmes sont résolus en construisant un réseau d'objets qui communiquent les uns avec les autres via le passage de messages et en répondant à ces messages en exécutant des méthodes définies dans les classes auxquelles ces objets appartiennent. Cela nous donne beaucoup de puissance: répartition dynamique, polymorphisme, encapsulation et une tonne d'autres. Tirer parti ceux-ci, votre émetteur de JSON ressemblerait à quelque chose comme ceci:

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 regarde comme ci-dessus, à l'exception évidemment sans la partie require 'json'.

Si vous voulez que votre JSON bien formaté, vous pouvez essayer quelque chose comme ceci:

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 

Il y a en fait rien Ruby spécifique dans ce code. C'est à peu près exactement à quoi ressemblerait la solution dans n'importe quel autre langage orienté objet basé sur une classe comme Java, par exemple. C'est juste un design orienté objet 101.

La seule chose qui est spécifique au langage est de savoir comment "modifier" les classes et leur ajouter des méthodes. En Ruby ou Python, vous modifiez littéralement la classe. En C# et Visual Basic.NET, vous utiliseriez probablement des méthodes d'extension, dans Scala vous utiliseriez des conversions implicites et en Java peut-être le Pattern Design Pattern.

Un autre énorme problème avec votre code est que vous essayez de résoudre un problème qui est évidemment récursive sans jamais réellement récursion. Ce juste ne peut pas travailler. Le code que vous avez écrit est fondamentalement du code Fortran-57: procédural sans objets et sans récursion. Même simplement déplacer un cran au dessus de Fortran, disons, Pascal, vous donne une belle solution procédurale récursive:

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 

Bien sûr, vous pouvez jouer le même jeu avec des indentations ici:

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 

est ici la sortie de échancré puts mine_location, produit en utilisant soit la deuxième version (en retrait) de to_json ou la deuxième version de jsonify, il ne compte pas vraiment, ils ont tous deux la même sortie:

[ 
    { 
    "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://static.px.yelp.com/static/20070816/i/ico/stars/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://static.px.yelp.com/static/20070816/i/ico/stars/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://static.px.yelp.com/static/20070816/i/ico/stars/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://static.px.yelp.com/static/20070816/i/ico/stars/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://static.px.yelp.com/static/20070816/i/ico/stars/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://static.px.yelp.com/static/20070816/i/ico/stars/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://static.px.yelp.com/static/20070816/i/ico/stars/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://static.px.yelp.com/static/20070816/i/ico/stars/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 
    } 
] 

Answer 2

Je serais curieux de voir l'erreur que vous obtenez de hash.to_json pour voir si la cause qui peut être trouvé.

En termes de votre code Ruby un couple d'observations:

• L'utilisation de .length.times faire .. est un peu bizarre au moment où vous pouvez utiliser par exemple each response['businesses'].each do ..

• Dans votre cas else text+="\"#{arrayKey}\":\"#{arrayValue}\"," il ressemble arrayKey et arrayValue sont hors de portée, car ils ne sont utilisés comme bloc de variables dans le each ci-dessus.

• text ="" texte sets retour à vide chaîne à chaque itération de l'aspect extérieur afin que le code se il semble comme le texte construit par les précédentes itérations de la boucle est mis au rebut.

Answer 3

La première chose que je remarque la chauve-souris est votre utilisation de

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

pour itérer. Cela peut être grandement simplifiée en utilisant Array.each qui vous fournit:

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

Vous vous faites en fait la même chose que ci-dessous avec votre: réponse [ « entreprises »] [businessEntry] .Chaque

Remarque sur le style, il est souvent (bien que non imposé) de style commun utiliser do/end si votre bloc est sur plusieurs lignes et {} si le bloc est une ligne. En outre, vous ne savez pas pourquoi vous construisez ces paires clé/valeur dans une chaîne, créez simplement un hachage. Ensuite, vous pouvez facilement convertir en JSON, ou comme Jorg l'a fait remarquer, il suffit de convertir toute la réponse à JSON qui fait EXACTEMENT ce que vous faites manuellement ... sauf si vous devez d'abord travailler avec les données (ce qui ne ressemble pas vous devez faire)

Answer 4

Je ne suis pas un expert sur Ruby, mais je sais ce que les choses si elles apparaissent dans mon code, mon professeur me crie dessus. Mikej déjà frappé sur les choses importantes, en particulier avec l'utilisation de #each au lieu de #length et #times.

Si je suis toujours itérer à travers une collection de quelque sorte, la seule fois où je l'utilise jamais autre chose que #each est quand je dois utiliser un itérateur personnalisé, et même alors vous pouvez toujours utiliser #each, mais vous auriez à avoir une instruction de contrôle à l'intérieur du bloc passé pour s'assurer que le bloc ne s'exécute pas sur certaines instances. Même si cela devient trop compliqué pour cela, la seule autre méthode d'itération personnalisée que j'utilise vraiment est une instruction for i in Range.new(begin, end, optional_exclusion). Et cela peut encore être transformé en conditionnelle dans un bloc pour #each, mais cela me sauve du code parfois et rend plus explicite que je ne suis pas intentionnellement en train d'exécuter du code sur tous les éléments d'une collection et que je montre explicitement que je suis en train de fixer les limites des éléments affectés au moment de l'entrée dans la boucle au lieu de valeurs codées en dur ou seulement la totalité de la collection. Mikej a déjà souligné l'erreur de portée quand les appels à arrayKey et arrayValue sont faits dans la partie else de l'instruction if, donc je ne m'en soucierai pas. Il a également déjà souligné, vous devriez probablement déplacer votre ligne de code text ="" par deux lignes pour sortir de cette étendue de bloc de code de réponse. Ma seule préoccupation après cela est quelques problèmes non pas avec le code lui-même, mais plus de style de codage et les choses généralement pratiquées dans la communauté Ruby. Donc, ces suggestions, en aucun cas, vous devez prendre. Cela rend la lecture du code Ruby généralement plus facile. Donc, ma première suggestion est quand vous appelez une méthode à laquelle vous passez un bloc, sauf si vous pouvez insérer l'indentation, l'appel d'objet, l'appel de méthode, la déclaration de variable de bloc, le bloc de code et le même ligne, n'utilisez pas d'accolades. Dans ces situations de blocs de code multiligne, utilisez les mots-clés et et et à la place.

Ma deuxième suggestion est d'utiliser une indentation correcte qui, dans le langage Ruby, est de deux espaces au lieu des quatre normaux trouvés dans les styles de codage de beaucoup d'autres langages. Vous avez eu l'indentation appropriée pour un bon mandrin de votre code et ensuite vous avez foiré et cela a causé quelques lignes sur la route pour avoir l'air d'être à un niveau de portée qu'ils ne sont pas. Maintenant, cette astuce est loin d'être une norme de style de codage, mais mon astuce générale pour m'assurer que je suis au bon niveau de portée est d'ajouter un commentaire de fin de ligne juste après l'utilisation du mot-clé end et de mettre le nom de la portée ça vient de se fermer. Il m'a sauvé d'un certain nombre de bugs de portée, mais je n'ai jamais entendu parler de quelqu'un d'autre le faire et il peut éventuellement encombrer le code avec ces commentaires aléatoires, mais c'est une méthode qui m'a bien servi. Ma troisième suggestion est d'améliorer votre utilisation des chaînes et des symboles. J'ai presque peur de le dire simplement parce que j'ai encore besoin d'améliorer ma compréhension des symboles dans Ruby et je ne me souviens pas d'avoir utilisé la classe Yelp dans des scripts récents, donc je suis aveugle sur celui-ci. Cependant, il semble que vous utilisiez la chaîne 'businesses' comme une clé de hachage. Une règle générale dans Ruby est que si vous utilisez une chaîne juste pour ce qu'elle représente, alors vous devriez utiliser un symbole alors que si vous utilisez une chaîne parce que vous avez réellement besoin de son contenu, vous devriez probablement vous en tenir à une chaîne. C'est juste parce que chaque fois que vous appelez un littéral de chaîne, une nouvelle chaîne est créée et stockée dans la mémoire du système. Donc ici puisque vous utilisez 'businesses' à l'intérieur de chaque bloc qui est à l'intérieur et chaque bloc, vous êtes potentiellement allouer cette chaîne O (n²) fois (bien que les allocations pour le bloc interne obtiennent garbage recueillies lors de l'itération suivante. Comme dans le cas de la ligne text ="", vous devez le modifier en un seul guillemet littéral: l'interpréteur Ruby peut analyser les littéraux de chaîne de guillemets simples plus vite que les guillemets doubles. Généralement, si vous pouvez utiliser un seul littéral de chaîne entre guillemets, faites-le.

Tout ce que j'ai pour des suggestions. Prenez-les comme vous en avez besoin ou les voulez. Aussi ici serait ce que votre code ressemblerait à supposer que vous avez pris mes suggestions et je n'ai rien cassé dans le processus.

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 

Je ne remplaçaient pas 'nicetry' avec un symbole juste parce que je ne sais pas comment fonctionne la classe Yelp et peut-être explicitement besoin d'une chaîne il au lieu d'un symbole. Je ne savais pas non plus quel était l'effet de code attendu puisque la seule fois que ce code est exécuté est quand les variables sont hors de portée, donc je n'avais aucun moyen de savoir ce que vous essayiez de référencer dans cette ligne. Surtout qu'à ce moment-là, votre valeur n'est pas non plus un tableau.

Je sais que c'est une réponse longue mais j'espère que cela aidera!