Where communities thrive


  • Join over 1.5M+ people
  • Join over 100K+ communities
  • Free without limits
  • Create your own community
People
Activity
sobaka_v_kepke
@chabapok_twitter
просто уже достало
sobaka_v_kepke
@chabapok_twitter
cp dark*.css light*.css
каждый раз после rustup update
Anton Sinitsyn
@sinitcin

Вопрос про философию!

Как правильно в расте писать secure, robust, и вообще high code quality чтобы было? Есть ли литература с примерами?

Понятно, что точно так же как и на любом другом языке, но все же вдруг кто-то что-то полезное посоветует
tanriol
@tanriol:matrix.org
[m]
Вопрос просто про "качественный код" или про какие-то специфические штуки в направлении, например, сертификации для каких-то областей?
Anton Sinitsyn
@sinitcin
@tanriol:matrix.org не, без всяких сертификатов. Просто интересно как писать идеальный код на расте
tanriol
@tanriol:matrix.org
[m]
В какой-то степени это зависит от области применения. Что-то очевидное - из серии "не оставляйте предупреждения, используйте rustfmt и clippy". Из специфичного именно для раста... хороший вопрос.
Я бы выделил "не используйте unsafe без необходимости", хотя некоторые программисты придерживаются другой точки зрения. Вроде бы никто не спорит против тезиса "используешь unsafe - не забывай тестировать с помощью miri".
Ну и глобальное "думай о структуре своих данных" - это нужно везде, но в расте нужнее, чем во многих других языках.
Anton Sinitsyn
@sinitcin
@tanriol:matrix.org столкнулся с тем, что последний ревью на работе в иностранной фирме не пропускают из-за того, что код недостаточно хорош. Unforgivable quality of code. Типа не достаточно чтобы код просто работал и проходил тесты. Отсутствие предупреждений тоже не достаточно. Было несколько осязаемых замечаний типа реже используйте mut, но в целом код должен быть secure, robust, и вообще high code quality. Вот теперь голову ломаю как эти общие термины привести к конкретным шагам
tanriol
@tanriol:matrix.org
[m]
@sinitcin: Не знаю без понимания специфики задачи, что именно здесь имеется в виду под secure (в предположении, что это чистая обработка без unsafe). По части robust я бы предположил, что это может быть про "меньше используйте unwrap/expect, обрабатывайте ошибки полноценно". Что до high code quality... единственное, что я здесь могу сказать - общие слова про "код должно быть легко читать". Незнакомый с кодом человек должен иметь возможность, глядя на него, понять его... и, что важно, не должен иметь возможности "легко" понять код неправильно.
Общие принципы в стиле "называйте переменные понятно, комментируйте при необходимости, что и почему код делает" и тому подобное.
Anton Sinitsyn
@sinitcin
@tanriol:matrix.org спасибо, но это в целом и так делается, но надо больше. Ну что же, покопаю по глубже, может статью напишу если разберусь
tanriol
@tanriol:matrix.org
[m]
Извини, более разумно не смогу посоветовать - я пока что не в "профессиональной" разработке, так что про ревью знаю больше в теории :-)
Lepuroid
@Lepuroid
Где-то вычитывал (или выслушивал), что как только получил что-то из unsafe, нужно сразу проверить, что это что-то - то, что нужно и дальше уже постулировать, что оно безопасно и не проверять больше никогда, т.е. все ошибки надо обрабатывать в момент их возможного появления, а не в процессе работы логики программы.
sobaka_v_kepke
@chabapok_twitter
все, что получено из любого внешнего места, должно проверяться на соответствие ожидаемым диапазонам
@sinitcin если хочешь, можешь скинуть мне свой код - а я попристебываюсь. Я конечно и сам наверняка не чисто пишу - но пристебаться к чужому всегда легче. Типа, с указанием конкретно, что непонравилось
Anton Sinitsyn
@sinitcin
@chabapok_twitter сорри, сюда не часто захожу, пропустил сообщение
tanriol
@tanriol:matrix.org
[m]
А зачем методы трейтов продублированы на самом типе - чисто чтобы не пришлось делать use этого трейта или есть какая-то более важная причина?
Anton Sinitsyn
@sinitcin
Скрыть реализацию, хотя лучше скинь конкретно где ты это увидел? Chainstate и ChainstateRef?
Или ты про врапперы типа таких?
    pub fn get_best_block_id(&self) -> Result<Id<GenBlock>, PropertyQueryError> {
        self.db_tx
            .get_best_block_id()
            .map_err(PropertyQueryError::from)
            .map(|bid| bid.expect("Best block ID not initialized"))
    }
tanriol
@tanriol:matrix.org
[m]
Да, реализации для ChainstateRef, где никакого преобразования типов вроде бы не видно.
Anton Sinitsyn
@sinitcin
Ну тут обычно проверки всунуты, как например тут что лучший блок должен быть в любом случае или что-то сломалось и map_err из ошибки БД к ошибке более высокого ранга
tanriol
@tanriol:matrix.org
[m]
Я скорее, например, про эту пару методов
tanriol
@tanriol:matrix.org
[m]
Или я вообще не туда смотрю, а вопросы относятся к PR 269?
Anton Sinitsyn
@sinitcin
Ааа, ну тут вышло так) Сначала были просто блоки в блокчейне, потом решили отделить генезис от остальных блоков и ID блока генезиса стало иметь другой тип. Потом добавили обобщенный тип для всех ID. Так вот get_gen_block_index возвращает обобщенный тип, а get_block_indexтолько для блоков в цепочке. Соответственно, где генезиса быть не должно пытаемся использовать get_block_index, а где он может быть то обобщенную версию
Да можно смотреть итоговый код, я тут активно накидал
Или сейчас скину
Вот тут все что относиться к токенам моё
Правда еще не закончил, но в целом код более грязный
Anton Sinitsyn
@sinitcin

Вот мои функции

calculate_tokens_total_inputs
filter_transferred_and_issued_amounts
calculate_transfered_and_burned_tokens
check_connected_burn_data
check_connected_transfer_data
get_change_amount
check_tokens_values
check_connected_tokens

tanriol
@tanriol:matrix.org
[m]
Хм... я не уверен, как у вас принято, но множественные коммиты "small fixes", "more fixes", "more fixes after merge" лично на мой взгляд выглядят не очень структурированно. Хотя это зависит от того, предпочитают ли у вас merge или rebase, да.
for (_input_idx, input) in inputs.iter().enumerate() - вот и зачем делать enumerate только ради того, чтобы проигнорировать индекс?
outpoint.output_index() as usize - а это каст с потерей информации или без? Не зная оригинальный тип, не скажешь. Многие за это не любят as-касты.
tanriol
@tanriol:matrix.org
[m]
match с одним "рабочим" вариантом обычно принято писать как if let, если задача не в том, чтобы гарантированно отрабатывать новые варианты в случае их добавления. Отдельный вопрос, зачем в хвостовой позиции continue вместо {}
В ту же степь - несколько удивляет явное игрорирование всех полей... или у вас там так принято?
tanriol
@tanriol:matrix.org
[m]
Ничего радикально ужасного там вроде бы не видно. Возможно, стоит спрашивать об их пожеланиях к коду их, а не нас...
Anton Sinitsyn
@sinitcin

for (_input_idx, input) in inputs.iter().enumerate() - вот и зачем делать enumerate только ради того, чтобы проигнорировать индекс?
outpoint.output_index() as usize - а это каст с потерей информации или без? Не зная оригинальный тип, не скажешь. Многие за это не любят as-касты.

Согласен косяк

По поводу каста, output_index() возвращает u32, аргумент был такой, мол, после сериализации данных все типы должны иметь фиксированный размер, тогда как usize может размер менять. Иначе ноды собранные для x86 и x64 могут не прочитать данные друг друга по сети
Anton Sinitsyn
@sinitcin

match с одним "рабочим" вариантом обычно принято писать как if let, если задача не в том, чтобы гарантированно отрабатывать новые варианты в случае их добавления. Отдельный вопрос, зачем в хвостовой позиции continue вместо {}

За if let ругаются у нас, типа не очевидно как раз будут ли новые элементы и не очевидно обработаны ли все элементы.

По поводу, continueтоже косяк

tanriol
@tanriol:matrix.org
[m]
Тогда я бы это писал как let index: usize = outpoint.output_index().try_into().expect("16-bit systems not supported"); (а скорее сделал бы, чтобы метод output_index делал это внутри себя и возвращал уже usize)...
Anton Sinitsyn
@sinitcin

Ничего радикально ужасного там вроде бы не видно. Возможно, стоит спрашивать об их пожеланиях к коду их, а не нас...

Ну собственно с этим я и пришёл сюда, что пожелания robust, secure, high quality)) Ну ладно, буду узнавать что конкретно не так, а не эти абстрактные термины

Спасибо
tanriol
@tanriol:matrix.org
[m]
Ну у них могут быть свои предпочтения :-) плюс я не смотрел на структуру кода "сверху", в смысле разбиение по функциям - возможно, там тоже что-то есть проблемное.
tanriol
@tanriol:matrix.org
[m]
А возможно, сказывается уже мой недостаток опыта и я не вижу крупные проблемы - тут уже упс, опыт у меня ограниченный и специфический.
Anton Sinitsyn
@sinitcin
Аналогично, да и с разработкой, то пусто, то густо. Несколько лет вообще только тим лидерством занимался
sobaka_v_kepke
@chabapok_twitter
@sinitcin Сразу хочу сказать, что когда пристебіваюсь я - то может и зря. Навскидку что бросилось в глаза. Строка 231 break Ok(block_index_walk), и аналогичные - это на самом деле не брик а ретурн. Такой стиль сбивает с толку. Я бы писал break только если там что-то типа let v = loop{ .... break result....}. А если это возврат - то это ретурн! Форматирование - ну на любителя. Во-первых, я бы разделял функции пустой строчкой. Между строкой 59 и 60 (и по всему коду аналогично) я бы вставил пустую. Кроме этого, я предпочитаю, чтобы аргументы функций были в строчку, а не в столбик. Но когда их много и функцию не разобьешь - то приходится и в столбик. Но я стараюсь не делать так. Но честно говоря, о стиле просто договариваются заранее, так что это ж не предмет, к которому пристебываться надо. Между сторокой с импл и функцией я бы тоже вставил пустую строчку(например - между 93 и 94). Всякие log::trace! я б заменил на trace!. Но это все неособо принципиальные замечания. Может, у них были претензии именно к архитектуре? Что еще? Например, в 156 строке использование Arc предполагает, что это все многопоточный код. Если код однопоточный, то там Rc уместней (но все зависит от общей архитектуры, о которой я не знаю ничего). И кстати, говоря. В строках 754-760 дергается запись в базу данных. Там идет add_to_block_index, set_block_index, add_block. Я ничего не знаю о базе и может я не прав - но если это все выполняется в несколько потоков (на что намекает упомянутый выше Arc), то эти 3 операции будут не атомарны? И если это действительно так - то вот это и было реальной причиной недовльства! А еще db_tx имеет тип BlockchainStorageRead - хотя там запись в строках 754-760. Может его надо просто обозвать BlockchainStorage, если там и запись тоже.
А подробней - это уже надо вникать.