Здравствуйте, Nuald, Вы писали:

N>В некоторых случаях там действительно может прийти null. Может прийти и пустая строка. Use-case сценарий приводить не буду, но пару раз ловил исключения при тестировании. Проверки я ставил только там, где есть смысл.

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

VD>>Если на оба последних вопроса ответ отрицательный, и этот код создан для чистоты, так сказать, то будь добр, замени его на генерацию исключений ArgumentNullException/ArgumentException.

N>Хм, может лучше ассерты?

Нет, лучше исключения. Что программа будет делать после того как этот ассерт закроется? Как продолжить работу в этой ситуации? Ведь на null и пустые строки там рассчета нет.

N>Если разработчик точно уверен, что в этом методе не придет null, тогда он ставит Debug.Assert.

Нет, см. выше. Конечно все проверки — это вообще сфера предпочтений и стратегий, но у нас принята стратегия в которой Assert и темболее Debug... ставится для того чтобы проверить свои предполжения. А когда ты пишишь метод который может быть вызван черт знает откуда, черт знает кем, то неверные входные условия являются исключительной ситуацией для функции, а значит должны генерировать исключения.

Исключения выгдны еще и тем, что в релизе они позволят востановиться до работоспосбного состояния. Ассерты же будут только досождать пользователю своими окнами. За ними будут появляться исключения, только уже менее информативные.

N> Я так глубоко не копал, чтобы точно знать, какие данные передаются в методы.

Темболее странно в таких условиях лепить if-ы замазывающие аварийные ситуации.

N> Поиск по вызывающим функциям показал, что вполне могут приходить некорректные строки.

Конкретнее пожалуйста. Что значит "некорретные"? И в следствии чего они могут приходить.

Еще раз повторю. Никаких замазываний проблем. Или это штатное поведение метода продиктованное проектным решением, или там обязан быть ассер. Уж лучше никакой обработки чем такая.

N>Ага, и в итоге блок if-a вообще становится незаметным в общем объеме кода.

Понимаеш ли в чем дело. Это не обсуждается. Это правила форматирования принятые в проекте. Фигурные скобки ставятся тольк для случаев когда есть более одного statment-а и возможно когда есть многострочное условие if-а и его тело плохо различимо.

N> Странно, почему вы тогда не используете форматирование K&R

Потому что по принятому у нас стилю оформления кода. Фигурные скобки ставятся всятся на новой строке.

N>P.S. Все-таки не понимаю, к чему придирки к проверкам?

Это очень плохо что не понимаешь. Я попытался обяснить. Посторайся понять. Основная причина — такие проекри могу скрыть реальные ошибки и превратить их из ошибок приводящих к исключению (кторое легко отслеживается) к ошибкам в логике работы программы (которые ловятся и исправляются намного сложнее).

N> Сами же сказали, что код находится на еще очень ранней стадии.

Это не повод писать код от балды.

N> Я отловил пару раз NullReferenceException и поставил проверки.

NullReferenceException не повод замазывать ошибку. Он скорее всего является следсвием другой ошибки. Вот ее и надо устранять. А проверки на null не должны быть связаны с тем что где-то там есть NullReferenceException. Они должны быть частью логики работы программы. Поэтому я и спрашиваю в чем эта логика. Так как мне кажется, что в некоторых местах эта логика нарушена или как минимум усложнена.

ОК, если не понимаешь, давай разберем конкретные случаи:

if (!String.IsNullOrEmpty(location))
    _projectLocation = location;
else
    _projectLocation = Path.GetDirectoryName(fileName);

Здесь можно сказать все ОК. Только зачем делать отрицающее условие? Оно усложняет чтение. В таких случаях нужно писать:

if (String.IsNullOrEmpty(location))
    _projectLocation = Path.GetDirectoryName(fileName);
else
    _projectLocation = location;

Отрицающие условия хуже читаются и в if/else и ?: их лучше не использовать.

public bool IsFileInProject(string filePath)
{
    if (String.IsNullOrEmpty(filePath))
    {
        return false;
    }
    return _filesMap.ContainsKey(filePath);
}

Вот здесь уже большой вопрос. Я не представляю себе шататной ситуации когда путь к файлу будет пустым. Хотя бы имя то у него должно быть? А раз так тут надо вставлять проверки с возбуждением исключеий. Или хотя бы ничего не делать.

Заметь, даже если не ставить проверки, то функция ContainsKey() класса Dictionary проверит аргументи и выдаст исключение ArgumentNullException. Твой же код скроет ошибку.

Если же у нас допустимы пустые имена файлов, то я хотел бы выслушать обоснование этого факта. Зачем это нужно, где и для чего используется.

public void UpdateFile(ParseRequest request)
{
    if (request == null)
    {
        return;
    }
    int    oldTimestamp;
    string filePath  = request.FileName;
    int    timestamp = request.Timestamp;

    if (!_filesMap.TryGetValue(filePath, out oldTimestamp))
        throw new ArgumentException("File '" + filePath + "' not found.", "filePath");

    if (timestamp != oldTimestamp)
    {
        Engine.Sources.AddOrUpdate(filePath, GetFileContent(request));
        _filesMap[filePath] = timestamp;
    }
}

А вот здесь на лицо явное замазывание ошибки. Если где-то произойдет ошибка и в request окажется null, то мы не узнаем о ней, так как твоя проверка скроет ошибку.

string GetFileContent(ParseRequest request)
{
    if (request == null)
    {
        return String.Empty;
    }
    string text = request.Text;

    if (String.IsNullOrEmpty(text))
        text = File.ReadAllText(request.FileName);

    return text;
}

Еще хуже! Здесь мы уже наверняка получим неверное поведение. Зачем нам пустая строка? null в request является ошибкой!

private static NemerleFileNodeProperties GetNodeProperties(IVsHierarchy hierarchy, uint itemID)
{
    if (hierarchy == null)
    {
        return new NemerleFileNodeProperties(null);
    }
    object propertyValue;
    int    hr = hierarchy.GetProperty(itemID, (int)__VSHPROPID.VSHPROPID_BrowseObject, out propertyValue);

    if (hr != VSConstants.S_OK)
        throw new ArgumentException("Can't obtain VSHPROPID_BrowseObject for item with ID " + itemID, "itemID");

    FileNodeProperties properties = propertyValue as FileNodeProperties;
    if (properties != null)
    {
        return new NemerleFileNodeProperties(properties.Node);
    }
    return new NemerleFileNodeProperties(null);
}

Тут не ясно. На лицо изменение поведения метода, но есть ли в этом смысл? Или это очередная замазка другой проблемы?

public static ProjectInfo FindProject(string fileName)
{
    if (String.IsNullOrEmpty(fileName))
    {
        return null;
    }
    foreach (ProjectInfo proj in _projectsInfos)
        if (proj.IsFileInProject(fileName))
            return proj;

    return null;
}

Здесь та же ситуация что и с IsFileInProject(). Вместо диагностики ошибки мы получим ее замазывание и будем часами искать ошибку.

N> Я еще не настолько разобрался в проекте, чтобы знать корень этого null-а, и поставил проверки, чтобы просто студия не вываливалась.

Ставить проверки чтобы что-то не вываливалось категорически не надо! Если ты не в силах понять причину проблемы, то просто опиши ее в этом форуме или добавь описание в баг-трекер. При этом обясни как воспроизвести эту ошибку. И тогда мы уже вместе решим что с ней делать.

N> Предложи мне другую стратегию разработки, и тогда я не буду так делать.

Стратегия проста. Баги надо устранять, а не замазывать. Любая провекра должна делаться обоснованно. Фраза "чтобы исключение не вылетала" не может быть обоснованием.

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