Re[4]: Нужно ваше мнение.. - О работе

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

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

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

D>Но мне что-то расхотелось идти на собеседование

"Не попробуешь, не узнаешь" (с)

и зачем всем этим кодерским онанизмом заниматься?? ИМХО и так понятный и простой код. Вы его только усложняете.

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

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

N>>Очень много if-ов — следствие плохого дизайна

D>А как избавится от каскадных if-ов? Мне в голову ничего не приходит

Много есть рефакторинго по борьбе с этим

Например,

                if (!build_op_tree(root, '-'))
                    return false;
                if (!build_op_tree(root, '+'))
                    return false;
                if (!build_op_tree(root, '/'))
                    return false;
                if (!build_op_tree(root, '*'))
                    return false;

можно заменить на что-то такое

            foreach (char eachSpecSymbol in "+-/*")
            {
                if (!build_op_tree(root, eachSpecSymbol))
                    return false;
            }

Это

switch (m.Groups["op"].Value)
                {
                    #region init op_compare
                    case "=":
                        op_compare = 0;
                        break;
                    case "!=":
                        op_compare = 1;
                        break;
                    case ">":
                        op_compare = 2;
                        break;
                    case "<":
                        op_compare = 3;
                        break;
                    case ">=":
                        op_compare = 4;
                        break;
                    case "<=":
                        op_compare = 5;
                        break;
                    #endregion
                }

на

op_compare = equityOperatorToCode[m.Groups["op"].Value];
// где equityOperatorToCode - заранее инициализированный Dictionary<string, int>

А вот поинтереснее,

case "a":
                        {
                            ...
                                        if (rvtype == 0)
                                        {
                                            float f = Convert.ToSingle(m.Groups["rvalue"].Value);
                                            bw.Write(f);
                                        }
                                        else if (rvtype == 1)
                                        {
                                            if (m.Groups["rvalue"].Value == "true")
                                                bw.Write((byte)1);
                                            else
                                                bw.Write((byte)0);
                                        }
                                        else if (rvtype == 2)
                                        {
                                            int n = Convert.ToInt32(m.Groups["rvalue"].Value);
                                            bw.Write(n);
                                        }
                            ...
                        }
                    case "s":
                        {
                            ...
                                if (rvtype == 0)
                                {
                                    float f = Convert.ToSingle(m.Groups["rvalue"].Value);
                                    bw.Write(f);
                                }
                                else if (rvtype == 1)
                                {
                                    if (m.Groups["rvalue"].Value == "true")
                                        bw.Write((byte)1);
                                    else
                                        bw.Write((byte)0);
                                }
                                else if (rvtype == 2)
                                {
                                    int n = Convert.ToInt32(m.Groups["rvalue"].Value);
                                    bw.Write(n);
                                }
                            ...
                        }
                    case "p":
                        {
                            ...
                                if (rvtype == 0)
                                {
                                    float f = Convert.ToSingle(m.Groups["rvalue"].Value);
                                    bw.Write(f);
                                }
                                else if (rvtype == 1)
                                {
                                    if (m.Groups["rvalue"].Value == "true")
                                        bw.Write((byte)1);
                                    else
                                        bw.Write((byte)0);
                                }
                                else if (rvtype == 2)
                                {
                                    int n = Convert.ToInt32(m.Groups["rvalue"].Value);
                                    bw.Write(n);
                                }
                            ...
                        }
                    case "tr":
                        {
                            ...
                                if (rvtype == 0)
                                {
                                    float f = Convert.ToSingle(m.Groups["rvalue"].Value);
                                    bw.Write(f);
                                }
                                else if (rvtype == 1)
                                {
                                    if (m.Groups["rvalue"].Value == "true")
                                        bw.Write((byte)1);
                                    else
                                        bw.Write((byte)0);
                                }
                                else if (rvtype == 2)
                                {
                                    int n = Convert.ToInt32(m.Groups["rvalue"].Value);
                                    bw.Write(n);
                                }
                            ...
                        }

Тут не только дублирование, кода, но и явно лишние ифы. Тут можно на тринарный оператор (?

заменить, можно на массив делегатов заменить. А лучше всего заменить тип rvtype с int на какой-нибудь самописный, дополнительно к числу содержащий в себе обработку данных для записи в поток. Аналогично надо избавлять от case. Может быть что-то вроде

bw.Write(rvtype.ConvertValueForStream(m.Groups["rvalue"].Value));

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

D>Меня вызвали на собеседование по .NET и сказали привезти с собой проекты. Но я так волнуюсь, у меня их мало, всего 2, и немножко стесняюсь показать им свой проект. Мне нужно ваше мнение, что бы вы сказали о моих знаниях глядя на следующий код?
D>http://d4rkme551ah.hostzi.com/DBManager.cs
D>http://d4rkme551ah.hostzi.com/PlxDevice.cs

Сразу оговорюсь, я не умудренный опытом гуру и на истинность в последней инстанции не претендую. Опыт коммерческой разработки небольшой, ниже я напишу то, как я бы подредактировал этот код перед тем как показывать на собеседовании, а уж следовать моим предложениям или нет — решать Вам. В основном мои замечания будут по стилю оформления, мне кажется, это основное на что смотрят в коде во время собеседования (прошу более опытных коллег написать так ли это или это лишь мое заблуждение)

Складывается ощущение что код писался в первую очередь для себя, что его смотреть или сопровождать будет только разработчик, пусть так, но, как мне кажется, на собеседование нужно представить код который будет легко читаться совсем посторонним человеком. Отсюда —

Во-первых, я бы добавил гораздо больше коментариев, уж по крайней мере закоментировал бы описания методов, свойств (желательно XML-коментариями) и заменил бы коментарии типа " //axtung >_<" чем-нибудь более осмысленным.

Во-вторых, вынес бы в константы магические числа из выражений типа

fs.SetLength(4 + 6 * max_db_record_count);

В-третьих, стиль наименования переменных и методов. Я бы переделал по стандартный стиль дотНЕТа, то есть методы вместо add_record сделал бы AddRecord, переменные вместо rec_count сделал бы хотябы recCount. Переименовал переменные типа op во что-то более осмысленное. Не понятно, зачем в названии bCounter буква b, скорее показывает тип bool, но тогда почему для других переменных такое правило не соблюдается? То есть если даже не приводить к стандартному стилю, то хотя бы к однобразному на весь кусок кода.

И последнее, убрал бы все зкоментированые куски кода (Вы же всё-таки представляете уже завершенный код, а не промежуточный вариант), убрал ненужные куски кода типа

if (File.Exists(fpath))
{
}

Удачи.

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

D>Мне нужно ваше мнение, что бы вы сказали о моих знаниях глядя на следующий код?
Код написан сишником недавно начавшим учить C#. Почему?
1) Названия функций и переменных
2) Перехват исключений и использование возвращаемых значений, типа true/false для индикации ошибок. Очень сильно захламляет код. Достаточно перехватывать на самом верху, ведь при исключении все равно продолжение работы невозможно.
3) Куча классов в одном файле
4) Неиспользование IDisposable и using
5) Совершенно непонятно зачем нужно было городить функции open/close/add_record и member variables типа FileStream, когда данные и так грузятся целиком в память после parse. Логичнее было бы вызвать одну единственную функцию db.WriteRecords(data)
6) Код нуждается в серьезном рефакторинге, он очень плохо читаем — extract method (функции на пару экранов), rename variables (куча сокращений и непонятных названий, типа i1). Да вообще разбивку по классам надо переделывать, тот же метод Parse надо выносить в отдельный класс, типа DataParser. Уверен, что после рефакторинга удастся избежать кучи повторного кода.

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

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

D>Меня вызвали на собеседование по .NET и сказали привезти с собой проекты. Но я так волнуюсь, у меня их мало, всего 2, и немножко стесняюсь показать им свой проект. Мне нужно ваше мнение, что бы вы сказали о моих знаниях глядя на следующий код?
D>http://d4rkme551ah.hostzi.com/DBManager.cs
D>http://d4rkme551ah.hostzi.com/PlxDevice.cs

бегло посмотрел
http://d4rkme551ah.hostzi.com/DBManager.cs

могу сказать следующее
Желательно унаследовать класс MyDB от интерфейса IDisposable и соответсвенно реализовать его.
Потомучто у тебя в классе есть три переменные:
FileStream fs;
BinaryReader br;
BinaryWriter bw;

Которые используют кретические ресурсы (файл) и в твоей реализации я не нашел нигде где эти ресурсы освобождаются. Понятно что придет сборщик мусора — но когда он придет никому не известно, а в этот момент ты сделал с файлом все что тебе нужно и он может потребоваться кому нибудь другому.
То что ты не освобождаеш критические ресурсы может привести к ошибкам.

И еще комментариев вообще нет, особенно во втором файле так как он более менее сложный.

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

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

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

KV>Если нет упомянутой недельки на рефакторинг, то на собеседовании можно пофантазировать "как бы я улучшил этот код" по мотивам полученных тут советов. IMHO подобные рассуждения могут сыграть куда более решающую роль на собеседовании нежели сам код.

Мне нравится вопрос "Что Вам нравится в этом коде?", "Что Вам не нравится в этом коде?"

Меня вызвали на собеседование по .NET и сказали привезти с собой проекты. Но я так волнуюсь, у меня их мало, всего 2, и немножко стесняюсь показать им свой проект. Мне нужно ваше мнение, что бы вы сказали о моих знаниях глядя на следующий код?
http://d4rkme551ah.hostzi.com/DBManager.cs
http://d4rkme551ah.hostzi.com/PlxDevice.cs

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

MS>могу сказать следующее
MS>Желательно унаследовать класс MyDB от интерфейса IDisposable и соответсвенно реализовать его.
MS>Потомучто у тебя в классе есть три переменные:
MS>FileStream fs;
MS>BinaryReader br;
MS>BinaryWriter bw;

MS>Которые используют кретические ресурсы (файл) и в твоей реализации я не нашел нигде где эти ресурсы освобождаются. Понятно что придет сборщик мусора — но когда он придет никому не известно, а в этот момент ты сделал с файлом все что тебе нужно и он может потребоваться кому нибудь другому.
MS>То что ты не освобождаеш критические ресурсы может привести к ошибкам.

Тут создается всего один статический экземпляр MyDB на всю программу, поэтому я не задумался об освобождении ресурсов.
А насчет комментариев, да, все время откладываю написание комментов, а потом и вовсе забиваю. Собственно тут это не так критично, потому что делаю проект только я. Но я стараюсь исправится.

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

D>Меня вызвали на собеседование по .NET и сказали привезти с собой проекты. Но я так волнуюсь, у меня их мало, всего 2, и немножко стесняюсь показать им свой проект. Мне нужно ваше мнение, что бы вы сказали о моих знаниях глядя на следующий код?

Бегло пролистал, и скажу что код хорошо читается. А это уже немало.

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

D>Меня вызвали на собеседование по .NET и сказали привезти с собой проекты. Но я так волнуюсь, у меня их мало, всего 2, и немножко стесняюсь показать им свой проект. Мне нужно ваше мнение

D>http://d4rkme551ah.hostzi.com/DBManager.cs
D>http://d4rkme551ah.hostzi.com/PlxDevice.cs

По стилю всё плохо. Главное что не нравится мне:
http://d4rkme551ah.hostzi.com/DBManager.cs

http://d4rkme551ah.hostzi.com/PlxDevice.cs

Очень много магических чисел. Нужны константы. Стилистических ошибок и недочетов могу найти очень много.

Но кода много, кое-что интересное применяется. Т.е. заблудился человек все-таки далеко не в 3 соснах. Очень интересно, работает ли этот код правильно. Я бы с удовольствием его погонял бы. Если несмотря на такой стиль он работает правильно — большой плюс.

D>что бы вы сказали о моих знаниях глядя на следующий код?
Я бы сказал, что в целом плохо, но не настолько, чтобы сразу fail. Есть какие-то знания FCL. Некоторые конторы могут и взять такого человека, хотя в текущие времена таких фирм все меньше и меньше

только про первый файл и только то что сразу бросилось в глаза.

0. Если вы пишете на C#, то логично пользоваться текущей microsoft code styles. в частности, про именование методов, переменных

1. пустой блок в коде?

if (File.Exists(fpath))
{
}

2. bool create(string fpath) — не закрывает fs, если он уже был открыт

3. try {} catch {} — ловит Exception. Поскольку new BinaryReader() и new BinaryWriter() не бросают исключений, то логично их вынести из try. и если произошел exception — обнулять bw и br, иначе состояние класса будет не consistent

4. "//axtung >_<" ?

5. "(uint)(rec_count + 1)" ?

6. у вас идет мешанина из ридера и райтера в одном флаконе. как можно переписать следующий кусок, чтобы информацию хранить у себя, а не читать из файла заново — ведь мы её сами записали:

br.BaseStream.Position = 4 + 6 * (rec_count - 1);//last record metadata position
uint start = br.ReadUInt32();
ushort length = br.ReadUInt16();
bw.Write((uint)(start + length));//write new record metadata
bw.Write((ushort)buf.Length);
bw.BaseStream.Position = start + length;//beginning of new record data

7. BinaryWriter.Close и BinaryReader.Close закрывают и underlying stream. на всякий случай говорю

8. Объявляйте переменные там, где будете использовать их:

BinaryWriter bw;

public byte[] Compile2Bin()
{
    bw = new BinaryWriter(new MemoryStream(), Encoding.Unicode);

9. "((MemoryStream)bw.BaseStream).ToArray()" — по другому никак?

дальше читать уже труднее. так что:

10. декомпозиция. по крайней мере не скидывайте классы в один файл

11. не вижу тестов

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

D>Меня вызвали на собеседование по .NET и сказали привезти с собой проекты. Но я так волнуюсь, у меня их мало, всего 2, и немножко стесняюсь показать им свой проект. Мне нужно ваше мнение, что бы вы сказали о моих знаниях глядя на следующий код?
D>http://d4rkme551ah.hostzi.com/DBManager.cs
D>http://d4rkme551ah.hostzi.com/PlxDevice.cs

для человека с годом опыта программированя — код нормальный.
другое дело что сказать по такому коду практически ничего нельзя.
вот если бы ты например прислушался ко всем вышеперечисленным и нижепоследующим замечаниям:

зачищать ресурсы через using+dispose, выправить кейсинг чтобы он смотрелся как в MSFT-ких библиотеках,
избавился он unsafe функций, убрал огромные строковые константы из кода, поменьше бы использовал if return, тривиальные кейсы перевел бы в выборку значения из преинициализированного dictionary

и отрефакторил бы за недельку (на досуге) код например из первого файла (DBManager.cs) — вот вот тогда по получившемося можно было бы судить о твоих знаниях гораздо более точно.

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

D>Складывается ощущение что код писался в первую очередь для себя, что его смотреть или сопровождать будет только разработчик, пусть так, но, как мне кажется, на собеседование нужно представить код который будет легко читаться совсем посторонним человеком. Отсюда -

D>Во-первых, я бы добавил гораздо больше коментариев, уж по крайней мере закоментировал бы описания методов, свойств (желательно XML-коментариями) и заменил бы коментарии типа " //axtung >_<" чем-нибудь более осмысленным.

D>Во-вторых, вынес бы в константы магические числа из выражений типа

D>

D>fs.SetLength(4 + 6 * max_db_record_count);
D>

D>В-третьих, стиль наименования переменных и методов. Я бы переделал по стандартный стиль дотНЕТа, то есть методы вместо add_record сделал бы AddRecord, переменные вместо rec_count сделал бы хотябы recCount. Переименовал переменные типа op во что-то более осмысленное. Не понятно, зачем в названии bCounter буква b, скорее показывает тип bool, но тогда почему для других переменных такое правило не соблюдается? То есть если даже не приводить к стандартному стилю, то хотя бы к однобразному на весь кусок кода.

D>И последнее, убрал бы все зкоментированые куски кода (Вы же всё-таки представляете уже завершенный код, а не промежуточный вариант), убрал ненужные куски кода типа

D>

D>if (File.Exists(fpath))
D>{
D>}
D>

D>Удачи.

Да, вы правы. Я единственный разработчик и код писался для себя. Вообще то мой основной профиль C++, наверное отсюда и мешаниниа стилей. Проекты свалились на меня внезапно, до этого на шарпе только Hello World-ы писал. Все нюансы изучал в процессе разработки.

под .net не писал но ты что волнуешься-то? :-D
нет причин для волнения!

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

N>По стилю всё плохо. Главное что не нравится мне:
N>http://d4rkme551ah.hostzi.com/DBManager.cs
N>

N>Немайкрософтовский casing

N>Непрозрачные наименования(а я далеко не ханжа в этом вопросе)

N>Повторяемость кода

N>Очень много if-ов — следствие плохого дизайна

N>

N>http://d4rkme551ah.hostzi.com/PlxDevice.cs
N>

N>Очень много магических чисел. Нужны константы.

N>

N>Стилистических ошибок и недочетов могу найти очень много.

N>Но кода много, кое-что интересное применяется. Т.е. заблудился человек все-таки далеко не в 3 соснах. Очень интересно, работает ли этот код правильно. Я бы с удовольствием его погонял бы. Если несмотря на такой стиль он работает правильно — большой плюс.

Да, код работает, сейчас этот АТС стоит на роботе, пока все ок. Добавляю мал-по малу новую функциональность.
А майкрософтовский кейсинг это как?
D>>что бы вы сказали о моих знаниях глядя на следующий код?
N>Я бы сказал, что в целом плохо, но не настолько, чтобы сразу fail. Есть какие-то знания FCL. Некоторые конторы могут и взять такого человека, хотя в текущие времена таких фирм все меньше и меньше

Огоромное спасибо за критику, теперь я знаю в каком направлении работать.

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

Ovl>только про первый файл и только то что сразу бросилось в глаза.

Ovl>0. Если вы пишете на C#, то логично пользоваться текущей microsoft code styles. в частности, про именование методов, переменных

Где про этот стайл можно прочитать? Долго кодил на C++, наверное это тоже повлияло.

Ovl>1. пустой блок в коде?
Ovl>

Ovl>if (File.Exists(fpath))
Ovl>{
Ovl>}
Ovl>

Ах да, там был код который я удалил, а блок оставил, забыл.

Ovl>2. bool create(string fpath) — не закрывает fs, если он уже был открыт

Ovl>3. try {} catch {} — ловит Exception. Поскольку new BinaryReader() и new BinaryWriter() не бросают исключений, то логично их вынести из try. и если произошел exception — обнулять bw и br, иначе состояние класса будет не consistent

Ovl>4. "//axtung >_<" ?
Согласен, глупо.

Ovl>5. "(uint)(rec_count + 1)" ?
Эх...

Ovl>6. у вас идет мешанина из ридера и райтера в одном флаконе. как можно переписать следующий кусок, чтобы информацию хранить у себя, а не читать из файла заново — ведь мы её сами записали:
Ovl>

Ovl>br.BaseStream.Position = 4 + 6 * (rec_count - 1);//last record metadata position
Ovl>uint start = br.ReadUInt32();
Ovl>ushort length = br.ReadUInt16();
Ovl>bw.Write((uint)(start + length));//write new record metadata
Ovl>bw.Write((ushort)buf.Length);
Ovl>bw.BaseStream.Position = start + length;//beginning of new record data
Ovl>

Спасибо, исправил.

Ovl>7. BinaryWriter.Close и BinaryReader.Close закрывают и underlying stream. на всякий случай говорю

Ovl>8. Объявляйте переменные там, где будете использовать их:
Ovl>

Ovl>BinaryWriter bw;

Ovl>public byte[] Compile2Bin()
Ovl>{
Ovl>    bw = new BinaryWriter(new MemoryStream(), Encoding.Unicode);

Ovl>

Ovl>9. "((MemoryStream)bw.BaseStream).ToArray()" — по другому никак?
А что не так?

Ovl>дальше читать уже труднее. так что:

Ovl>10. декомпозиция. по крайней мере не скидывайте классы в один файл

Ovl>11. не вижу тестов
А их и нет

Сделал потом удолил.

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

N>Очень много if-ов — следствие плохого дизайна

А как избавится от каскадных if-ов? Мне в голову ничего не приходит

D>Где про этот стайл можно прочитать? Долго кодил на C++, наверное это тоже повлияло.
http://msdn.microsoft.com/en-us/library/czefa0ke%28VS.71%29.aspx

D>А что не так?

using(MemoryStream stream = new MemoryStream())
using(BinaryWriter writer = new BinaryWriter(stream, Encoding.Unicode))
{
  // ...

  return stream.ToArray();
}

D>А их и нет

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

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

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

D>>Меня вызвали на собеседование по .NET и сказали привезти с собой проекты. Но я так волнуюсь, у меня их мало, всего 2, и немножко стесняюсь показать им свой проект. Мне нужно ваше мнение, что бы вы сказали о моих знаниях глядя на следующий код?
D>>http://d4rkme551ah.hostzi.com/DBManager.cs
D>>http://d4rkme551ah.hostzi.com/PlxDevice.cs

IS>для человека с годом опыта программированя — код нормальный.
IS>другое дело что сказать по такому коду практически ничего нельзя.
IS>вот если бы ты например прислушался ко всем вышеперечисленным и нижепоследующим замечаниям:

IS>зачищать ресурсы через using+dispose, выправить кейсинг чтобы он смотрелся как в MSFT-ких библиотеках,
IS>избавился он unsafe функций, убрал огромные строковые константы из кода, поменьше бы использовал if return, тривиальные кейсы перевел бы в выборку значения из преинициализированного dictionary

IS>и отрефакторил бы за недельку (на досуге) код например из первого файла (DBManager.cs) — вот вот тогда по получившемося можно было бы судить о твоих знаниях гораздо более точно.

Майкрософтовский кейсинг это как?
Насчет unsafe функции, там вызивается библиотечная функция из dll, который принимает указатели, так что без этого никак.
А вот за кририку огромно спасибо, теперь я хотя бы знаю в каком направлении работать и где у меня слабые места. Но мне что-то расхотелось идти на собеседование

D>А вот за кририку огромно спасибо, теперь я хотя бы знаю в каком направлении работать и где у меня слабые места. Но мне что-то расхотелось идти на собеседование

это конечно зависит от позиции. если на seniour — рано. а на junior — можно. а дальше все зависит от желания.
но зря — расти в компании профессионалов (или просто людей с "альтернативным" мнением) всегда легче и удобнее

У меня возникла идея, пусть кто-нибудь из гуру выложит тут свой эталонный код. Я хочу поглядеть как это выглядит. Но только что нибудь реальное, а не семпл какой нибудь.

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

D>и зачем всем этим кодерским онанизмом заниматься?? ИМХО и так понятный и простой код. Вы его только усложняете.

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

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

D>и зачем всем этим кодерским онанизмом заниматься?? ИМХО и так понятный и простой код. Вы его только усложняете.

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

И после моих изменений код остается достаточно простым и понятным, но при этом код, который не делает ничего интересного, перестает занимать много места => На экране легче видно, какое именно место занимают эти действия в методе. Кроме того, после таких изменений, код становится проще для модификации.

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

D>Меня вызвали на собеседование по .NET и сказали привезти с собой проекты. Но я так волнуюсь, у меня их мало, всего 2, и немножко стесняюсь показать им свой проект. Мне нужно ваше мнение, что бы вы сказали о моих знаниях глядя на следующий код?
D>http://d4rkme551ah.hostzi.com/DBManager.cs
D>http://d4rkme551ah.hostzi.com/PlxDevice.cs

Напишите комментарий на каждый класс и на каждый метод, чего этот класс делает, а на метод дополнительно — чего ожидает на входе и как реагирует на особые/ошибочные случаи, if any

И сверху шапочку напишите с названием модуля, автором/копирайтом, датой написания и коротким описанием, что этот модуль делает

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

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

D>А вот за кририку огромно спасибо, теперь я хотя бы знаю в каком направлении работать и где у меня слабые места. Но мне что-то расхотелось идти на собеседование

а вот это зря — сходить на собеседование и пообщаться всегда полезно.

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

IS>поменьше бы использовал if return

т. е. if(нечто) { return что-нибудь; }? А в чём проблема?

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

IS>>поменьше бы использовал if return
RO>т. е. if(нечто) { return что-нибудь; }? А в чём проблема?

один или два if-а это нормально, но вот такие конструкции:

            {
                if (!build_op_tree(root, '-'))
                    return false;
                if (!build_op_tree(root, '+'))
                    return false;
                if (!build_op_tree(root, '/'))
                    return false;
                if (!build_op_tree(root, '*'))
                    return false;
            }

надо убирать.

Почему?

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

Да и вообще код с большим количеством условий увеличивает его сложность (cyclomatic complexity?).
Ксати, что я заметил — кореляция между опытом человека и простотой его кода, как бы это выразиться — нелинейная.
кто-то пишет почти сразу более менее простой код, хотя и не всегда рабочий — а кто-то вроде бы и научился с опытом
наводить марафет на код, и работает все, но код все равно излишне сложен (не интуитивен) для развития и понимания.

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

D>Меня вызвали на собеседование по .NET и сказали привезти с собой проекты. Но я так волнуюсь, у меня их мало, всего 2, и немножко стесняюсь показать им свой проект. Мне нужно ваше мнение, что бы вы сказали о моих знаниях глядя на следующий код?
D>http://d4rkme551ah.hostzi.com/DBManager.cs
D>http://d4rkme551ah.hostzi.com/PlxDevice.cs
Посмотрел очень бегло. Из хорошего — код отформатирован, у многих и этого не наблюдается. Из плохого — методы очень длинные (методы parseExpr, parseFile это вообще жесть, насмотрелся уже на такое — если встречаю — переписываю). Куча вложенных if наблюдается, magic numbers в константы не вынес (что такое 4, что такое 6). Как минимум к публичным методам сверху комментарии, к классам тоже — что делает, какие параметры и т.д. Закомментированный код наблюдается (справедливости сказать его мало, но он есть). Много copy-paste, не заметил ни одного private метода для вынесения общей логики, утилитными методами для конвертации типов (и других частых операций) не пользуешься — все на месте делаешь. Можно много чего еще накопать.

Резюме — типичный джуниор, на среднего разработчика не тянешь. Что-то серьезное давать опасно, я бы посадил вначале на багфиксинг, бросил бы в пару с опытным разработчиком чтобы он показал как надо писать, через год-полтора возможно будет отдача. Если не попадешь с пару с опытным разработчиком, то будешь писать так же и через 5 лет — проверено. Для начала надо прочитать Макконела "Совершенный код", Фаулера "Рефакторинг" — немного стиль это улучшает.

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

D>и зачем всем этим кодерским онанизмом заниматься?? ИМХО и так понятный и простой код. Вы его только усложняете.
Понятный для тех, кто сам так пишет, кто привык к такому коду, и не видел никогда нормального. Да, я видел код гораздо хуже, хуже на порядки, но — вначале это и был как раз именно вот такой понятный и простой код. Народ тоже считал рефакторинг кодерским онанизмом, при увеличении требований добавляли еще один if — как результат, через несколько лет проект полностью падает под собственной тяжестью. Там, где требовалось бы 3 человека на месяц — набирают где-то сотню человек и делают тоже самое, но за 2 года. От такого стиля выигрывают только менеджеры, у которых зарплата от количества подчиненных зависит — тут да, возможности осваивать бюджет будут большие. Все шишки от такого кода полетят потом на разработчиков, которым этот код впоследствии передадут — они окажутся крайними, знаю, плавали. Кстати да, для того, чтобы стать незаменимым сотрудником код слишком простой и понятный — потому рекомендую названия методов и переменных урезать до одного — двух символов, убрать форматирование, и накрутить кучу мертвого кода для объема.

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

IS>один или два if-а это нормально, но вот такие конструкции:

IS>

IS>            {
IS>                if (!build_op_tree(root, '-'))
IS>                    return false;
IS>                if (!build_op_tree(root, '+'))
IS>                    return false;
IS>                if (!build_op_tree(root, '/'))
IS>                    return false;
IS>                if (!build_op_tree(root, '*'))
IS>                    return false;
IS>            }
IS>

IS>надо убирать.

Я б такое убрал только из-за того, что это ^C^V. Если бы это был язык типа Пайтона, я бы вообще написал в том месте

return all(build_op_tree(root, op) for op in "-+/*")

(там return true дальше по течению), а если бы во всех if были существенно разные выражения, что тогда?

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

RO>Я б такое убрал только из-за того, что это ^C^V. Если бы это был язык типа Пайтона, я бы вообще написал в том месте
RO>

return all(build_op_tree(root, op) for op in "-+/*")

на C# один из вариантов будет такой (может на LINQ и короче будет):

foreach(var op in "-+/*")
{
    if (!build_op_tree(root, op))
    {
        return false;    
    }
}

код можно вытянуть в одну строку убрав две пары необязательных с точки зрения языка curly brackets

RO>(там return true дальше по течению), а если бы во всех if были существенно разные выражения, что тогда?
тогда ничего не надо делать — существенно разные выражения явно показывают сущесвенно разный intent. Ну разве только что добавить коментариев.

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

D>Меня вызвали на собеседование по .NET и сказали привезти с собой проекты. Но я так волнуюсь, у меня их мало, всего 2, и немножко стесняюсь показать им свой проект. Мне нужно ваше мнение, что бы вы сказали о моих знаниях глядя на следующий код?
D>http://d4rkme551ah.hostzi.com/DBManager.cs
D>http://d4rkme551ah.hostzi.com/PlxDevice.cs

Мало комментариев

... << RSDN@Home 1.1.4 stable SR1 rev. 568>>

От:	kochetkov.vladimir	https://kochetkov.github.io
Дата:	30.07.09 10:26
Оценка:	2 (1) +1

	От:	dilmah
	Дата:	30.07.09 10:41
	Оценка:	1 (1) -1

	От:	Nuseraro
	Дата:	30.07.09 10:22
	Оценка:	2 (1)

	От:	Dumka
	Дата:	30.07.09 07:48
	Оценка:	1 (1)

	От:	olegkr
	Дата:	30.07.09 15:09
	Оценка:	1 (1)

От:	Pzz	https://github.com/alexpevzner
Дата:	30.07.09 17:28
Оценка:

	От:	elmal
	Дата:	01.08.09 06:20
	Оценка:

	От:	elmal
	Дата:	01.08.09 06:38
	Оценка:

	От:	AC1D
	Дата:	04.08.09 11:37
	Оценка: