Как я могу улучшить? - PullRequest
       3

Как я могу улучшить?

1 голос
/ 10 июня 2010

Мне было интересно, если кто-нибудь может указать более чистый способ написания моего кода, который вставлен здесьКод собирает некоторые данные из 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

Ответы [ 4 ]

8 голосов
/ 10 июня 2010

Похоже, что весь ваш код в конечном итоге делает это:

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, вот пара советов для вас.

Номер 1, который вы полностью игнорируете в своем коде, - это то, что Ruby является объектно-ориентированным языком, а точнее объектно-ориентированным языком на основе классов. Это означает, что проблемы решаются путем создания сети объектов, которые взаимодействуют друг с другом посредством передачи сообщений и отвечают на эти сообщения, выполняя методы, определенные в классах, к которым принадлежат эти объекты.

Это дает нам много энергии: динамическая отправка, полиморфизм, инкапсуляция и множество других. Используя их, ваш 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: процедурный, без объектов и без рекурсии. Даже простое перемещение на один на шаг вверх от Фортрана, скажем, к Паскалю, дает вам хорошее рекурсивное процедурное решение:

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, полученный с использованием либо второй (с отступом) версии 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://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
  }
]
8 голосов
/ 10 июня 2010

Первое, что я замечаю вне летучей мыши, это то, что вы используете

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

На самом деле вы делаете то же самое ниже со своим: ответ [ 'бизнес'] [businessEntry] .each

Обратите внимание на стиль, это часто (хотя и не принудительно) общий стиль, использующий do / end, если ваш блок состоит из нескольких строк, и {}, если блок состоит из одной строки.

Кроме того, не уверен, почему вы строите эти пары ключ / значение в строке, просто сделайте хеш. Затем вы можете легко конвертировать в json или, как указал Jorg, просто конвертировать весь ответ в json, который делает ТОЧНО то, что вы делаете вручную ... если только вам не нужно сначала работать с данными (что не похоже на нужно сделать)

2 голосов
/ 10 июня 2010

Я не специалист по Ruby, но я знаю, что, если они появляются в моем коде, мой профессор кричит на меня.Mikej уже коснулся основных вещей, особенно с использованием #each вместо #length и #times.

. Если я когда-либо перебираю какую-то коллекцию, то единственный раз, когда я используючто-то отличное от #each - это когда мне нужно использовать пользовательский итератор, и даже тогда вы все равно можете использовать #each, но вам нужно будет иметь управляющий оператор внутри переданного блока, чтобы убедиться, что блок не выполняется наопределенные случаи.Даже если это становится слишком сложным, единственный другой метод пользовательской итерации, который я действительно использую, - это оператор for i in Range.new( begin, end, optional_exclusion ).И это все еще может быть превращено в условное в блоке для #each, но это иногда экономит мне код и делает более явным, что я намеренно не выполняю код для всех элементов коллекции, а также явно показываю, что я устанавливаюграницы затронутых элементов во время входа в цикл вместо жестко закодированных значений или только всей коллекции.

Mikej уже указал на ошибку области действия при вызовах arrayKey и arrayValueв остальной части заявления if, поэтому я не буду беспокоиться об этом.Он также уже отметил, что вам, вероятно, следует переместить строку кода text ="" вверх на две строки, чтобы выйти из области видимости блока кода ответа.

После этого меня беспокоит только некоторые проблемы, не связанные с самим кодом,но в сообществе Ruby практикуется более широкий стиль программирования.Так что эти предложения ни в коем случае не надо принимать.Это просто делает чтение кода Ruby в целом проще.

Итак, мое первое предложение - всякий раз, когда вы вызываете метод, которому вы передаете блок, если вы не можете подобрать правильный отступ, вызов объекта, вызов метода, объявление переменной блока,блок кода и блок, закрывающий все в одной строке, не используйте фигурные скобки.В таких ситуациях многострочных кодовых блоков используйте ключевые слова do и end .

Второе мое предложение - использовать правильные отступы, которые в языке Rubyдва пробела вместо нормальных четырех, встречающихся в стилях кодирования многих других языков.У вас были правильные отступы для хорошего куска вашего кода, а затем вы все испортили, и это заставило некоторые строки в будущем выглядеть так, как будто они находятся на уровне области видимости, а не на уровне.Теперь этот совет далеко не соответствует стандарту стиля кодирования, но мой общий трюк, чтобы убедиться, что я нахожусь на правильном уровне контекста, - просто добавить комментарий в конце строки сразу после использования ключевого слова end и ввести имясфера его просто закрыта.Это спасло меня от ряда ошибок в области видимости, но я никогда не слышал, чтобы кто-то другой делал это, и это, возможно, может загромождать код этими случайными комментариями в конце строки, но этот метод хорошо мне помог.

Мое третье предложение - улучшить использование строк и символов. Я почти боюсь сказать это только потому, что мне все еще нужно улучшить мое понимание символов в Ruby, и я не припоминаю использование класса Yelp в каких-либо недавних сценариях, поэтому я ослеп на этом. Тем не менее, похоже, что вы используете строку 'businesses' как ключ Hash. Общее правило в Ruby: если вы используете строку только для того, что она представляет, вам следует использовать символ, а если вы используете строку, потому что вам действительно нужно ее символьное содержимое, то вам, вероятно, следует придерживаться использования строки. Это просто потому, что каждый раз, когда вы вызываете строковый литерал, создается новая строка и сохраняется в системной памяти. Так что здесь, так как вы используете 'businesses' внутри каждого блока, который находится внутри и каждого блока, вы потенциально выделяете эту строку O (n²) раз (хотя выделения для внутреннего блока будут собираться мусором во время следующей итерации. вместо этого использовался символ, такой как «: companies», он инициализирует его только один раз. Также в случае строки text ="" вы должны преобразовать ее в строковый литерал в одну кавычку. Интерпретатор 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, и ему явно может понадобиться строка вместо символа. Я также не знал, каков был предполагаемый эффект кода, поскольку единственный раз, когда этот код выполняется, это когда переменные находятся вне области видимости, поэтому у меня не было никакого способа узнать, на что вы пытались ссылаться в этой строке. Тем более что в этот момент ваше значение также не является массивом.

Я знаю, что это длинный ответ, но я надеюсь, что это поможет!

2 голосов
/ 10 июня 2010

Мне было бы интересно увидеть ошибку, которую вы получили от hash.to_json, чтобы увидеть, можно ли найти причину этого.

С точки зрения вашего кода на Ruby пара замечаний:

  • Использование .length.times do .. немного странно, когда вы можете просто использовать each например response['businesses'].each do ..

  • В вашем else case text+="\"#{arrayKey}\":\"#{arrayValue}\"," похоже, что arrayKey и arrayValue находятся вне области видимости, потому что они используются только как блочные переменные в each выше.

  • text ="" устанавливает текст обратно в пустую строку на каждой итерации внешнего вида, так что, если код стоит, он выглядит так, как будто текст, созданный предыдущими итерациями цикла, отбрасывается.

Добро пожаловать на сайт PullRequest, где вы можете задавать вопросы и получать ответы от других членов сообщества.
...