Комментарии 4
И тут можно заметить, что, если isSuperTab – true, значение settings.PortalId не обрабатывается. Может показаться, что вот такой неявный контракт заложен, и в принципе всё ОК.
Нет.
Да. Вот именно из-за этого статическими анализаторами и не пользуются. Можно сколько угодно говорить, что код корявый и так не должно быть, но это не изменит факта того, что проблема именно в анализаторе.
if ( isSuperTab )
settings = new(...);
NavigateURL( isSuperTab, settings );Причем вы и сами это понимаете - если тул выкатывает 10К предупреждений, на проекте, который хотя-бы запускается, значит что-то не так с тулом, а не проектом. Это как "секъюрити сканер", который дает "секьюрити варнинг" на любое вхождение слова pass в имя переменной... в том числе и на passportId, bypass и т.д.
И до кучи: этот варнинг и голая студия дает.
Фух, я думал ни одного комментария не будет... Спасибо то, что обозначили свою позицию. :)
Да.
Нет. Если бы у меня не было никаких вопросов/замечаний к этому коду, я бы его не выписывал. Когда же я читал этот код (не вызывающий его, ни какой-то ещё, а именно код метода - у меня возникли вопросы). В нём всегда, прежде чем работать с settings, их проверяют на null, здесь - нет.
Останусь при своём мнении. Если Вас такой код устраивает - ОК, хозяин - барин.
Вот именно из-за этого статическими анализаторами и не пользуются.
Ага.
Причем вы и сами это понимаете - если тул выкатывает 10К предупреждений, на проекте, который хотя-бы запускается, значит что-то не так с тулом, а не проектом.
Тут я не знаю, с чем спорить. В том плане, что в статье про 10К предупреждений я не говорил. Код, когда не учитывается иммутабельность строк и т.п., тоже нормальным навряд ли можно назвать, хотя проект может и запускаться.
Конечно, можно выкрутить всё на максимум, и тогда завалиться (привет, MISRA). А можно поднастроить анализатор, даже если изначально он выдавал какие-то лишние предупреждения - исключить ненужные файлы из анализа, отключить ненужные диагностики и т.п.; и пользоваться с удобством.
И до кучи: этот варнинг и голая студия дает.
Не знаю, что у вас за инстанс VS. Мой (2019) ни о чём не репортит.
Не, вы не поняли - я не говорю, что к коду нет вопросов. Я говорю что в данном случае код работает и ошибки (в данный момент) нет. Мне может нравиться или не нравиться этот код. Но именно конкретно той самой ошибки (возможное обращение к нулевой переменной) по факту нет. Есть другое - опасный код, неявное соглашение... да что угодно, только не то, про что предупредили. Это проблема.
Насчет 70К предупреждений я вот про это:
Загрузили его, проанализировали проект и… получили кучу предупреждений. Десятки, сотни, тысячи, может, даже десятки тысяч. Ого, вот это номер…
Проблема очевидна - если видишь несколько тысяч предупреждений в рабочем проекте, значит однозначно большенство из них - ложные.
А можно поднастроить анализатор, даже если изначально он выдавал какие-то лишние предупреждения - исключить ненужные файлы из анализа, отключить ненужные диагностики и т.п.; и пользоваться с удобством.
Так нет-же! Произойдет вот что: программист посмотрит на 10-20 первых, увидит, что там реально проблемы нет и выпилит анализатор. Никто не будет настраивать его! Обычный баланс качества и добства. В этом плане анализаторы студии гораздо удобнее (я кстати не знаю может и у вас так-же) - показываются цветные метки в коде и никакой стены текста. Психологически это очень удобно - тебя не заставляют что-то делать (текст - это уже приказ). Кроме того метки обучают отлично. Ты просто начинаешь сначала без описания понимать что там не так, а потом и вообще не допускать.
Никто не спорит, что анализатор - штука полезная. Но вас тянут назад две вещи - подавляющее большенство мусорных предупреждений и факт того, что для C# вы конкурируете не только с ReSharper (который фактически стандарт), но и со всей экосистемой анализаторов Roslyn.
Чтоб не просто языком молоть, я бы предложил вам сделать "динамическую приоритезацию" предупреждений. Например для проекта выводим не больше 20-30 предупреждений, но только самых серьезных. Все исправлены? Добавляем до 20 менее приоритетных и т.д. Т.е. не надо озадачивать 200 сообщениями - их никто ситать не будет (я точно не стану).
Я говорю что в данном случае код работает и ошибки (в данный момент) нет.
Так статический анализ - он как раз про потенциальные ошибки / уязвимости и пр. В каких-то случаях - более точно, в каких-то - менее. Собственно, в доках к диагностике так и написано: "Анализатор обнаружил потенциальную ошибку, которая может привести к доступу по нулевой ссылке."
Возвращаясь к обсуждаемому кода. Если кто-нибудь вызовет этот метод, когда settings - null, а isSuperTab - true, будет проблема? Будет. То, что эти параметры связаны неявным контрактом - ну уж извините, тут и человеку нужно повникать, чтобы сделать такое заключение. :)
Проблема очевидна - если видишь несколько тысяч предупреждений в рабочем проекте, значит однозначно большенство из них - ложные.
Стоит помнить, что во всех (известных мне) анализаторах есть какое-то разделение по уровням опасности/достоверности и т.п. Если мы говорим про то, чтобы получить 70К предупреждений, то я не знаю... Это нужно MISRA врубить на здоровом проекте, который писался не под неё. :)
Навряд ли будет такое кол-во критичных предупреждений. Ну может только если макрос какой загадит статистику, да и то, всё равно не такие цифры)
Тем не менее, в целом на legacy действительно может быть много предупреждений. Но оно же работает! И пусть работает. Посмотрели наиболее критичный уровень, поправили, что нужно - baselinig остального - и вперёд и с песней работаем только с новыми предупреждениями.
Вот тут про всё это написано подробнее.
Так нет-же! Произойдет вот что: программист посмотрит на 10-20 первых, увидит, что там реально проблемы нет и выпилит анализатор.
В целом мысль верная. Действительно, если, полистав предупреждения, пользователь увидит только фолсы, желание разгребать остальные предупреждения и что-то настраивать у него поубавится.
Здесь как раз может помочь то, о чём вы (и на самом деле - я тоже) писали ранее:
Например для проекта выводим не больше 20-30 предупреждений, но только самых серьезных.
Я как раз и писал про них - best warnings. Нажимаете кнопку, и вам выводится только 10 наиболее интересных (с точки зрения анализатора) предупреждений.
Повторюсь, фича новая (в прошлом релизе только появилась), но уже неплохо показывает себя на проектах в плане выборки. Я думаю, она ещё будет улучшаться и этот как раз примерно то направление, о котором вы говорите.
Разнообразие ошибок в C# коде на примере CMS DotNetNuke: 40 вопросов к качеству