Comments 20
Не понравилось огромное количество магических чисел.
Ну можно почти все магические числа заменить на sizeof(unsigned int) или sizeof(double) и не факт, что стало бы читабельнее, мне нужно было ехать, а не «шашечки». Да и все константы очевидны из описания формата.
Не можно, а нужно, не нужно ничего самому выдумывать, если можно воспользоваться средствами языка. В данном случае еще не страшно, но вот когда где-то используются магические числа, потом к примеру рефакторинг под x64 и замена unsigned int на size_t, а потом еще неделя поиска багов.
Все «магические» числа, это смещения в байтах для заранее известной внешнеопределенной бинарной структуры. И поля структуры, совершенно спокойно, могут не иметь соответствий в типах определенных в языке. Так что, указания конкретного количества байт смещения, будет нагляднее и очевиднее чем что-либо другое. А в случае сферического рефакторинга, Вы правы.
По поводу «магических» чисел. Для начала пример наглядности:
Первый вариант мне кажется более очевидным для чтения.
Второй не содержит «магических» чисел, но стал ли он более читаемым(очевидным для понимания)?
inline unsigned int size() const { return *(unsigned int *)m_data+5; } // 1
// или
inline unsigned int size() const { return *(unsigned int *)m_data+sizeof(unsigned char)+sizeof(unsigned int); } // 2
Первый вариант мне кажется более очевидным для чтения.
Второй не содержит «магических» чисел, но стал ли он более читаемым(очевидным для понимания)?
А так:
inline unsigned int size() const { return *(unsigned int *)m_data+SizeOffset; }
Так да! Но есть ли в этом объективный смысл? Улучшает ли это понимание кода, облегчает использование?
Тот кто использует этот класс все равно вызовет метод size() и не увидит что за этим скрывается.
А при чтении кода нужно будет отвлекаться и смотреть на определение SizeOffset.
В предыдущем Вашем примере уже становится не так красиво, количество Size'ов на строку кода великовато, да и смысл каждого Size'а различен.
Тот кто использует этот класс все равно вызовет метод size() и не увидит что за этим скрывается.
А при чтении кода нужно будет отвлекаться и смотреть на определение SizeOffset.
В предыдущем Вашем примере уже становится не так красиво, количество Size'ов на строку кода великовато, да и смысл каждого Size'а различен.
ptr += HeaderSize + (*(unsigned int*)(ptr+SizeOffset) * sizeof(double)*2);
На мой взгляд, этот вариант легче читается.
Еще один довод в пользу именованных констант (может быть немного надуманный, но все же) — если вдруг формат когда-нибудь поменяется и, к примеру, размер заголовка увеличится, будет затруднительно исправлять все девятки в коде. А вдруг какая-то из этих магических девяток означает не размер заголовка, а что-то другое?
Еще один довод в пользу именованных констант (может быть немного надуманный, но все же) — если вдруг формат когда-нибудь поменяется и, к примеру, размер заголовка увеличится, будет затруднительно исправлять все девятки в коде. А вдруг какая-то из этих магических девяток означает не размер заголовка, а что-то другое?
Читается легче, но где-то читал, может быть даже на этом сайте, что читаемость кода понижается если определения участвующих в выражении сущностей не доступны на этом же экране. Т. е. если для понимания что же такое, например, SizeOffset придется прокручивать экран или открывать другой файл, читаемость падает.
Для абстрактного кода и его рефакторинга, именованные константы безусловно предпочтительнее. Но в конкретном случае мы имеем дело со сложившимся и неизменным долгое время форматом, и по моему мнению, тут нет необходимости городить такой огород.
Для абстрактного кода и его рефакторинга, именованные константы безусловно предпочтительнее. Но в конкретном случае мы имеем дело со сложившимся и неизменным долгое время форматом, и по моему мнению, тут нет необходимости городить такой огород.
А почему нельзя было просто взять кусок gdal вместо придумывания колеса?
GDAL, а конкретнее OGR, выполняет много ненужной в конкретной задаче работы. В частности он копирует данные из WKB в свои собственные объекты, что снижает производительность.
а сколько оверхеда получается в численном выражении?
«снижает производительность», это, конечно, плохо, но в очень большом классе сценариев вполне приемлимо.
«снижает производительность», это, конечно, плохо, но в очень большом классе сценариев вполне приемлимо.
В численном выражении сказать не получится, тестов не ставили. Если хочется конкретных цифр, то у Вас теперь есть оба инструмента для сравнения, можете попробовать. Если же рассматривать чисто умозрительно, по сравнению с нашим подходом добавляются две операции, выделение памяти и копирование. Их можно избежать, что и было сделано.
Вопрос больше не в этом.
Это шестая или седьмая реализация чтения GDAL'овских стандартных форматов, проходящая мимо меня. В двух случаях, когда проект пошёл развиваться дальше, собственная имплементация была выкинута и заменена на что-то другое.
Чем не подошёл тот же lwgeom из postgis? Там и чтение WKB реализовано полностью, и в geos интерфейсы есть, и в geojson сериализация вполне себе ничего. Вот, 250к linestring'ов в виде 59-мегабайтной таблички только что за восемь секунд в geojson сконвертилось, выгребясь из базы на моём не самом новом ноуте.
Соответственно, хочется понять, что движет людьми, раз за разом переписывающими стандартные библиотеки вместо дописывания нужных вещей, которых в них нет. Хотя бы в виде «раньше работало часы на таком-то датасете, теперь минуты».
Это шестая или седьмая реализация чтения GDAL'овских стандартных форматов, проходящая мимо меня. В двух случаях, когда проект пошёл развиваться дальше, собственная имплементация была выкинута и заменена на что-то другое.
Чем не подошёл тот же lwgeom из postgis? Там и чтение WKB реализовано полностью, и в geos интерфейсы есть, и в geojson сериализация вполне себе ничего. Вот, 250к linestring'ов в виде 59-мегабайтной таблички только что за восемь секунд в geojson сконвертилось, выгребясь из базы на моём не самом новом ноуте.
Соответственно, хочется понять, что движет людьми, раз за разом переписывающими стандартные библиотеки вместо дописывания нужных вещей, которых в них нет. Хотя бы в виде «раньше работало часы на таком-то датасете, теперь минуты».
Ну в статье и так написано, что я отдаю себе отчет что пишу велосипед. Ну а по времени получается примерно следующее:
Нужной вещью, в моем случае, оказалась производительность, за нее и пришлось побороться в этом велосипеде принеся определенные жертвы.
- Версия с копированием данных в промежуточный формат (примерно так поступает OGR) — примерно 40 секунд.
- Версия первоначальной реализации которая приведена в статье — Fill bundle map2012_bundle: 2.26405 sec.
- Версия в работе — Fill bundle map2012_bundle: 0.0550931 sec.
Нужной вещью, в моем случае, оказалась производительность, за нее и пришлось побороться в этом велосипеде принеся определенные жертвы.
Забавная у вас карта — всего два объекта (Москва и Питер), но низкое качество спутникового фото (засветы и радуга на объектах). Зашел посмотреть свой город в хайрезе (режим Спутник и увидел кусок телевизора)
По коду — не понравилось, что вы представление JSON жестко зашиваете в код. А если вдруг понадобится XML?
По коду — не понравилось, что вы представление JSON жестко зашиваете в код. А если вдруг понадобится XML?
Понадобится XML — сделаем. Будет перспектива добавления множества форматов выдачи — напишем обобщенное решение. А тратить время на код который никогда не пригодится, слишком затратно. А что Вам в коде понравилось?
Если честно — ничего выдающегося. По моему скромному мнению, пост ради поста на Хабре — было бы интересней реализовать парсинг и выдачу в нужном формате — как самих данных, так и в представлениях JSON/XML/BSON) с точки зрения расширения архитектуры, например, форкнув аналогичный код от GDAL, выделить в отдельный модуль и предложить публике.
А так — одноразовый велик, который никому, кроме вас скорее всего не понадобится — слишком узкий коридор применимости данного кода.
А так — одноразовый велик, который никому, кроме вас скорее всего не понадобится — слишком узкий коридор применимости данного кода.
Sign up to leave a comment.
Разбор WKB формата без сторонних библиотек