Как не надо писать код
От: AlexNek  
Дата: 08.04.11 21:59
Оценка: 3 (2) -2
Что то часто мне стал попадаться код в который сразу не врубишься, при этом код делает свою задачу правильно. Предлагаю постить сюда ваши образчики и комментарии.

        public void WriteTransformation(ref byte[] buffer, ref int bufferLength, WriteByteHandler writeByteHandler)
        {
            if (writeByteHandler != null)
            {
                bufferLength = 1 + Math.Min(Math.Max(bufferLength, 0), (buffer != null) ? buffer.Length : 0);

                byte[] newBuffer;

                if (bufferLength - 1 < ((buffer != null) ? buffer.Length : 0))
                {
                    newBuffer = buffer;
                }
                else
                {
                    newBuffer = new byte[bufferLength];

                }

                for (int i = bufferLength - 1; i > 0; i--)
                {
                    if (newBuffer != null) if (buffer != null)
                            newBuffer[i] = buffer[i - 1];
                }

                if (newBuffer != null) newBuffer[0] = PREFIX;

                buffer = newBuffer;
            }
        }

Самое смешное в том, что часть строки я прочел как
bufferLength = .... Math.Max(bufferLength, 0), (buffer != null) ? buffer.Length : 0

и долго не мог врубиться как же оно работает

Интересно сколько времени вам понадобилось, что бы понять как функция точно работает?
avalon 1.0rc3 rev 380, zlib 1.2.3

04.05.11 09:32: Перенесено модератором из '.NET' — TK
Re: Как не надо писать код
От: AlexNek  
Дата: 08.04.11 22:10
Оценка:
Здравствуйте, AlexNek, Вы писали:

foreach (var group in hash
        .GroupBy(rq => rq.IsRead)
        .Select(grp => new { IsRead = grp.Key, Ids = grp.SelectMany(rq => rq.MsgIds) }))
    MarkMsgsRead(
        _provider,
        group.Ids,
        group.IsRead);


Незнаю как вы, но большой одной строки для foreach я как то слабо представляю, поэтому также не смог сразу врубиться, не говоря уж об обилии лямбд
avalon 1.0rc3 rev 380, zlib 1.2.3
Re: Как не надо писать код
От: xobotik Россия  
Дата: 09.04.11 03:47
Оценка:
Здравствуйте, AlexNek, Вы писали:
if (buffer != null)

Порадовало такое ощущение что код писал параноик)

Да и на фига массив по ссылке передавать?)))

А можно ли постановку задачи, чтобы выложить свою версию метода?
С уважением!
Re: Как не надо писать код
От: Sinix  
Дата: 09.04.11 04:30
Оценка: +1
Здравствуйте, AlexNek, Вы писали:

AN>Что то часто мне стал попадаться код в который сразу не врубишься, при этом код делает свою задачу правильно. Предлагаю постить сюда ваши образчики и комментарии.


А CopyTo + запись префикса — слишком просто?

За ref-ы, допущение того, что bufferLength может быть меньше 0, непроверку на переполнение — приговорить к пожизненному сопровождению своего кода.
Re: Как не надо писать код
От: Andrey Rubayko  
Дата: 09.04.11 06:22
Оценка:
Здравствуйте, AlexNek, Вы писали:

AN>Что то часто мне стал попадаться код в который сразу не врубишься, при этом код делает свою задачу правильно. Предлагаю постить сюда ваши образчики и комментарии.
Re[2]: Как не надо писать код
От: Andrey Rubayko  
Дата: 09.04.11 06:27
Оценка:
AN>>Что то часто мне стал попадаться код в который сразу не врубишься, при этом код делает свою задачу правильно. Предлагаю постить сюда ваши образчики и комментарии.

Мде. Поспешил запостить.

Неужели вы занимаетесь ревью чужого кода?

В целом написано, конечно, топорно. Человек не знал что есть такая штука как "??", но код будет работать. Единственное, что умиляет — newBuffer = buffer. Вот тут как раз надо человека засадить за изучение описания C# из MSDN и конкретно книгу Джеффри Рихтера.
Re[2]: Как не надо писать код
От: Andrey Rubayko  
Дата: 09.04.11 06:33
Оценка:
Здравствуйте, AlexNek, Вы писали:

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


AN>
foreach (var group in hash
AN>        .GroupBy(rq => rq.IsRead)
AN>        .Select(grp => new { IsRead = grp.Key, Ids = grp.SelectMany(rq => rq.MsgIds) }))
AN>    MarkMsgsRead(
AN>        _provider,
AN>        group.Ids,
AN>        group.IsRead);


AN>Незнаю как вы, но большой одной строки для foreach я как то слабо представляю, поэтому также не смог сразу врубиться, не говоря уж об обилии лямбд


Вот тут как раз надо смотреть как работает механика на лямбда. Иначе у меня такое подозрение, что запрос энтот будет выполнятся на каждой итерации цикла. Возможно даже цикл получится бесконечным. Ну и помаркать сообщения можно бы было напрямую лямбда-запросом. Но это уже всякие извращения.
Re[2]: Как не надо писать код
От: User239 Россия  
Дата: 09.04.11 06:52
Оценка:
Здравствуйте, AlexNek, Вы писали:

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


AN>
foreach (var group in hash
AN>        .GroupBy(rq => rq.IsRead)
AN>        .Select(grp => new { IsRead = grp.Key, Ids = grp.SelectMany(rq => rq.MsgIds) }))
AN>    MarkMsgsRead(
AN>        _provider,
AN>        group.Ids,
AN>        group.IsRead);


AN>Незнаю как вы, но большой одной строки для foreach я как то слабо представляю, поэтому также не смог сразу врубиться, не говоря уж об обилии лямбд


По-моему как раз наоборот простой читаемый код
Конечно можно было бы написать
void FindAndMarkMsgs(bool isRead)
{
   MarkMsgsRead(_provider, hash.Where(rq => rq.IsRead == isRead).Select(rq => rq.Id), isRead);  
}

FindAndMarkMsgs(true);
FindAndMarkMsgs(false);


Возможно выглядит проще. Но при изменении типа IsRead, к примеру было bool, стало bool?, придётся добавить вызов FindAndMarkMsgs(null). В варианте же с группировками ничего менять не надо.
Re: Как не надо писать код
От: SergeyT. США http://sergeyteplyakov.blogspot.com/
Дата: 09.04.11 07:09
Оценка:
Здравствуйте, AlexNek, Вы писали:

Вчера наткнулся на следующий код разбора аргументов командной строки (код упрощен, но сам цикл и разбор аргументов сохранен):

static void ParseCommandLineArgs(params string[] args)
{
    string param1 = "undefined";
    string param2 = "undefined";
    int i = 0;
    while(i < args.Length)
    {
        if (args[i] == "param1" && args.Length > ++i)
        {
            param1 = args[i++];
        }
        else if (args[i] == "param2" && args.Length > ++i)
        {
            param2 = args[i++];
        }
    }
    Console.WriteLine("param1 = {0}, param2 = {1}", param1, param2);
}
void Main()
{
    // Выведет: param1 = value1, param2 = value2
    ParseCommandLineArgs("param1", "value1", "param2", "value2");
    // А здесь: param1 = value1, param2 = undefined
    ParseCommandLineArgs("param1", "value1", "param2");

}


С кодом все нормально, но чтобы понять, нет ли ошибки на единицу нужно каждый раз напрягаться.
Re[3]: Как не надо писать код
От: _FRED_ Черногория
Дата: 09.04.11 07:15
Оценка:
Здравствуйте, Andrey Rubayko, Вы писали:

AR>…Человек не знал что есть такая штука как "??", но код будет работать.


А где бы вы применили этот оператор в показанном коде?

AR>Единственное, что умиляет — newBuffer = buffer. Вот тут как раз надо человека засадить за изучение описания C# из MSDN и конкретно книгу Джеффри Рихтера.


А не расскажете поподробнее?
Help will always be given at Hogwarts to those who ask for it.
Re[4]: Как не надо писать код
От: Andrey Rubayko  
Дата: 09.04.11 07:39
Оценка:
Здравствуйте, _FRED_, Вы писали:

_FR>Здравствуйте, Andrey Rubayko, Вы писали:


AR>>…Человек не знал что есть такая штука как "??", но код будет работать.


_FR>А где бы вы применили этот оператор в показанном коде?


Ааа. Недосмотрел.

Там таки длину присваиваем

Просто давече видел другой код:


(buffer != null) ? buffer : 0


Вот и подумал что тоже самое.

AR>>Единственное, что умиляет — newBuffer = buffer. Вот тут как раз надо человека засадить за изучение описания C# из MSDN и конкретно книгу Джеффри Рихтера.


_FR>А не расскажете поподробнее?


Ну как. В данном случае просто копируется указатель на массив buffer в newBuffer. Честно говоря, мож так и надо и мы не будем использовать начальное значение переменной buffer. Так что ладно.
Re: Как не надо писать код
От: SergeyT. США http://sergeyteplyakov.blogspot.com/
Дата: 09.04.11 08:26
Оценка:
Здравствуйте, AlexNek, Вы писали:

AN>Что то часто мне стал попадаться код в который сразу не врубишься, при этом код делает свою задачу правильно. Предлагаю постить сюда ваши образчики и комментарии.

[skiped]

AN>Интересно сколько времени вам понадобилось, что бы понять как функция точно работает?


Более того, я осмелился этот код переписать. Вот что у меня вышло:

public byte[] WriteTransformation(byte[] buffer, int bufferLength)
{
    // Выделяем проверку предусловий в отдельный блок, что на порядок упрощает весь остальной код.
    // Контракты просто упрощают запись, но никто не мешает проверить предусловия руками.
    Contract.Requires(buffer != null);
    Contract.Requires(bufferLength >= 0);
    Contract.Ensures(Contract.Result<byte[]>() != null);

    // Прежде всего убираем не используемый параметр, если же он используется, то добавляем
    // соответствующее предусловие.

    // А теперь демонстрируем уличную магию того, как проверка предусловий упрощает код:
    // Было: bufferLength = 1 + Math.Min(Math.Max(bufferLength, 0), (buffer != null) ? buffer.Length : 0);
    // Меняем: (buffer != null) ? buffer.Length : 0 --> buffer.Length, благодаря buffer != null
    // Меняем: Math.Max(bufferLength, 0) --> bufferLength, благодаря bufferLength >= 0
    // Добавляем комментарий и вот, что мы получаем в итоге:
            
    // Необходимость +1 обусловлена тем, что в нулевой байт выходного буфера мы поместим PREFIX
    bufferLength = 1 + Math.Min(bufferLength, buffer.Length);

    byte[] newBuffer;

    // Теперь меняем следующий код:
    // Было: if (bufferLength - 1 < ((buffer != null) ? buffer.Length : 0))
    // Проверка buffer != null, уже не нужна
    // Получаем: if (buffer.Length > bufferLength - 1)
    // Заменяем > на >=:
    // if (buffer.Lengh >= bufferLength)
    // Инвертируем логику, поскольку она проще
    // Результат: if (buffer.Lenfth < bufferLenfth) ...
    // Добавляем комментарий и вот, что получаем в результате:

    // Нам нужно удостовериться, что в исходном массиве есть место под дополнительный
    // байт префикса:
    if (buffer.Length < bufferLength )
    {
        newBuffer = new byte[bufferLength];
    }
    else
    {
        newBuffer = buffer;
    }

    // Все, что удалось улучшить в этом коде, так это избавиться от лишних
    // проверок и добавить комментарий, поясняющий причину перебора элементов
    // "справа". Кстати, Array.CopyTo здесь не подойдет, поскольку он копирует
    // *все* элементы массива, а реальный размер массива buffer может быть большим

    // Добавляем комментарий:
    // Теперь нам нужно скопировать все элементы из массива buffer в newBuffer, сместив
    // каждый из них на  один индекс вправо, чтобы в нулевой индекс поместить PREFIX
    for (int i = bufferLength - 1; i > 0; i--)
    {
        newBuffer[i] = buffer[i - 1];
    }

    // Здесь тоже проверка уже не нужна
    newBuffer[0] = PREFIX;

    // У нас все еще есть проблема, поскольку в некоторых случаях мы меняем переданный буфер,
    // так что, если размер этого массива обычно небольшой, то я бы предпочел, чтобы всегда
    // возвращался новый объект.
    // Неизменяемость (или имутабельность) - это отличная техника программирования!
    return newBuffer;
}
Re[2]: Как не надо писать код
От: AlexNek  
Дата: 09.04.11 12:06
Оценка:
Здравствуйте, SergeyT., Вы писали:

ST> AN>Что то часто мне стал попадаться код в который сразу не врубишься, при этом код делает свою задачу правильно. Предлагаю постить сюда ваши образчики и комментарии.


ST> [skiped]


ST> AN>Интересно сколько времени вам понадобилось, что бы понять как функция точно работает?


ST> Более того, я осмелился этот код переписать. Вот что у меня вышло:


А для NET 2.0 это тоже будет работать?
avalon 1.0rc3 rev 380, zlib 1.2.3
Re[2]: Как не надо писать код
От: AlexNek  
Дата: 09.04.11 12:06
Оценка:
Здравствуйте, SergeyT., Вы писали:

ST> Вчера наткнулся на следующий код разбора аргументов командной строки (код упрощен, но сам цикл и разбор аргументов сохранен):


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


Чтобы понять как работает также нужно напрягаться.
Называется "экономия строк"
avalon 1.0rc3 rev 380, zlib 1.2.3
Re[3]: Как не надо писать код
От: AlexNek  
Дата: 09.04.11 12:06
Оценка:
Здравствуйте, User239, Вы писали:

U> AN>
foreach (var group in hash

U> AN>        .GroupBy(rq => rq.IsRead)
U> AN>        .Select(grp => new { IsRead = grp.Key, Ids = grp.SelectMany(rq => rq.MsgIds) }))

U> AN>    MarkMsgsRead(
U> AN>        _provider,
U> AN>        group.Ids,
U> AN>        group.IsRead);


U> AN>Незнаю как вы, но большой одной строки для foreach я как то слабо представляю, поэтому также не смог сразу врубиться, не говоря уж об обилии лямбд


U> По-моему как раз наоборот простой читаемый код

Может дело привычки, но первое что я увидел, было
foreach (var group in hash

Затем уже начинаешь соображать, а нахрена остальные строки. Даже такой записи уже было бы достаточно
foreach (var group in hash
        .GroupBy(rq => rq.IsRead)
        .Select(grp => new { IsRead = grp.Key, Ids = grp.SelectMany(rq => rq.MsgIds) }))
{
    MarkMsgsRead(
        _provider,
        group.Ids,
        group.IsRead);
}


Кстати, не поясните, что делает эта часть (какой тип возвращает return)? (У нас в проекте linq не используют)

            var groups = groupBy.Select(
                delegate(IGrouping<bool, MarkRequest> grp)
                    {
                        return new {IsRead = grp.Key, Ids = grp.SelectMany(rq => rq.MsgIds)};
                    }
                );
avalon 1.0rc3 rev 380, zlib 1.2.3
Re[3]: Как не надо писать код
От: AlexNek  
Дата: 09.04.11 12:06
Оценка:
Здравствуйте, Andrey Rubayko, Вы писали:

AR> AN>
foreach (var group in hash

AR> AN>        .GroupBy(rq => rq.IsRead)
AR> AN>        .Select(grp => new { IsRead = grp.Key, Ids = grp.SelectMany(rq => rq.MsgIds) }))

AR> AN>    MarkMsgsRead(
AR> AN>        _provider,
AR> AN>        group.Ids,
AR> AN>        group.IsRead);


AR> AN>Незнаю как вы, но большой одной строки для foreach я как то слабо представляю, поэтому также не смог сразу врубиться, не говоря уж об обилии лямбд


AR> Вот тут как раз надо смотреть как работает механика на лямбда. Иначе у меня такое подозрение, что запрос энтот будет выполнятся на каждой итерации цикла. Возможно даже цикл получится бесконечным. Ну и помаркать сообщения можно бы было напрямую лямбда-запросом. Но это уже всякие извращения.


Что то у меня есть такое подозрение что foreach вызывает "enumerable-expression" только один раз.
avalon 1.0rc3 rev 380, zlib 1.2.3
Re[2]: Как не надо писать код
От: AlexNek  
Дата: 09.04.11 12:06
Оценка:
Здравствуйте, Sinix, Вы писали:

S> AN>Что то часто мне стал попадаться код в который сразу не врубишься, при этом код делает свою задачу правильно. Предлагаю постить сюда ваши образчики и комментарии.


S> А CopyTo + запись префикса — слишком просто?

Вопрос не ко мне . Если есть желание, можете просто выложить Вашу версию.

S> За ref-ы,

У функции уже были параметры (без рефов), менять слишком много было просто запрещено.
S> допущение того, что bufferLength может быть меньше 0,
Пути господни неисповедимы
avalon 1.0rc3 rev 380, zlib 1.2.3
Re[2]: Как не надо писать код
От: AlexNek  
Дата: 09.04.11 12:06
Оценка:
Здравствуйте, xobotik, Вы писали:

x>
 if (buffer != null)

x> Порадовало такое ощущение что код писал параноик)
Эээ... батенька вас надо с нашей QA группой познакомить
x> Да и на фига массив по ссылке передавать?)))
Я же говорю код тяжело понять Входной массив не обязательно будет "константой".

x> А можно ли постановку задачи, чтобы выложить свою версию метода?

Но проблемо. Кстати, эта функция раньше была гораздо проще.
Есть некоторый массив байт, который нужно в итоге переслать в выходной поток. Но перед этим нужно в начале добавить один байт.
Примечание:
Добавить байт в буфер вначале, до данных, 
непонятно как, так как при другом типе выходного потока байт добавлять не нужно, 
а данные приходят "снаружи".

Раньше, копировать данные абсолютно не хотелось и дополнительный байт просто выводился прямо в поток, а после все остальное. Однако оказалось, что минимум под виндовс 7 такой способ передачи резко не нравится приемнику он требовал передавать все сразу.
Кстати размер буфера не может быть больше 256 байт и в реале получается от 10 до 150 байт.
avalon 1.0rc3 rev 380, zlib 1.2.3
Re[4]: Как не надо писать код
От: Ziaw Россия  
Дата: 09.04.11 12:23
Оценка:
Здравствуйте, AlexNek, Вы писали:

AN>Кстати, не поясните, что делает эта часть (какой тип возвращает return)? (У нас в проекте linq не используют)


AN>
            var groups = groupBy.Select(
AN>                delegate(IGrouping<bool, MarkRequest> grp)
AN>                    {
AN>                        return new {IsRead = grp.Key, Ids = grp.SelectMany(rq => rq.MsgIds)};
AN>                    }
AN>                );

AN>


возвращает анонимный класс с двумя полями bool IsRead и IEnumerable<T> Ids где T тип элемента в rq.MsgIds.
Re[3]: Как не надо писать код
От: AlexNek  
Дата: 09.04.11 12:25
Оценка:
Здравствуйте, AlexNek, Вы писали:

AN> ST> AN>Что то часто мне стал попадаться код в который сразу не врубишься, при этом код делает свою задачу правильно. Предлагаю постить сюда ваши образчики и комментарии.


AN> ST> [skiped]


AN> ST> AN>Интересно сколько времени вам понадобилось, что бы понять как функция точно работает?


AN> ST> Более того, я осмелился этот код переписать. Вот что у меня вышло:


AN> А для NET 2.0 это тоже будет работать?

Будет, если либу загрузить...

А если не нужны "чужие сообщения" либо вообще какие либо сообщения?
avalon 1.0rc3 rev 380, zlib 1.2.3
Подождите ...
Wait...
Пока на собственное сообщение не было ответов, его можно удалить.