Здравствуйте, d4rkme551ah, Вы писали:
D>Меня вызвали на собеседование по .NET и сказали привезти с собой проекты. Но я так волнуюсь, у меня их мало, всего 2, и немножко стесняюсь показать им свой проект. Мне нужно ваше мнение, что бы вы сказали о моих знаниях глядя на следующий код? D>http://d4rkme551ah.hostzi.com/DBManager.cs D>http://d4rkme551ah.hostzi.com/PlxDevice.cs
могу сказать следующее
Желательно унаследовать класс MyDB от интерфейса IDisposable и соответсвенно реализовать его.
Потомучто у тебя в классе есть три переменные:
FileStream fs;
BinaryReader br;
BinaryWriter bw;
Которые используют кретические ресурсы (файл) и в твоей реализации я не нашел нигде где эти ресурсы освобождаются. Понятно что придет сборщик мусора — но когда он придет никому не известно, а в этот момент ты сделал с файлом все что тебе нужно и он может потребоваться кому нибудь другому.
То что ты не освобождаеш критические ресурсы может привести к ошибкам.
И еще комментариев вообще нет, особенно во втором файле так как он более менее сложный.
Здравствуйте, 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
Сразу оговорюсь, я не умудренный опытом гуру и на истинность в последней инстанции не претендую. Опыт коммерческой разработки небольшой, ниже я напишу то, как я бы подредактировал этот код перед тем как показывать на собеседовании, а уж следовать моим предложениям или нет — решать Вам. В основном мои замечания будут по стилю оформления, мне кажется, это основное на что смотрят в коде во время собеседования (прошу более опытных коллег написать так ли это или это лишь мое заблуждение)
Складывается ощущение что код писался в первую очередь для себя, что его смотреть или сопровождать будет только разработчик, пусть так, но, как мне кажется, на собеседование нужно представить код который будет легко читаться совсем посторонним человеком. Отсюда —
Во-первых, я бы добавил гораздо больше коментариев, уж по крайней мере закоментировал бы описания методов, свойств (желательно XML-коментариями) и заменил бы коментарии типа " //axtung >_<" чем-нибудь более осмысленным.
Во-вторых, вынес бы в константы магические числа из выражений типа
fs.SetLength(4 + 6 * max_db_record_count);
В-третьих, стиль наименования переменных и методов. Я бы переделал по стандартный стиль дотНЕТа, то есть методы вместо add_record сделал бы AddRecord, переменные вместо rec_count сделал бы хотябы recCount. Переименовал переменные типа op во что-то более осмысленное. Не понятно, зачем в названии bCounter буква b, скорее показывает тип bool, но тогда почему для других переменных такое правило не соблюдается? То есть если даже не приводить к стандартному стилю, то хотя бы к однобразному на весь кусок кода.
И последнее, убрал бы все зкоментированые куски кода (Вы же всё-таки представляете уже завершенный код, а не промежуточный вариант), убрал ненужные куски кода типа
Немайкрософтовский casing
Непрозрачные наименования(а я далеко не ханжа в этом вопросе)
Повторяемость кода
Очень много if-ов — следствие плохого дизайна
Стилистических ошибок и недочетов могу найти очень много.
Но кода много, кое-что интересное применяется. Т.е. заблудился человек все-таки далеко не в 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 positionuint 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. декомпозиция. по крайней мере не скидывайте классы в один файл
Здравствуйте, 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-ы писал. Все нюансы изучал в процессе разработки.
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>9. "((MemoryStream)bw.BaseStream).ToArray()" — по другому никак?
А что не так?
Ovl>дальше читать уже труднее. так что:
Ovl>10. декомпозиция. по крайней мере не скидывайте классы в один файл
Ovl>11. не вижу тестов
А их и нет Сделал потом удолил.
Здравствуйте, 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 — можно. а дальше все зависит от желания.
но зря — расти в компании профессионалов (или просто людей с "альтернативным" мнением) всегда легче и удобнее
У меня возникла идея, пусть кто-нибудь из гуру выложит тут свой эталонный код. Я хочу поглядеть как это выглядит. Но только что нибудь реальное, а не семпл какой нибудь.
Здравствуйте, 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;
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. Может быть что-то вроде
Здравствуйте, d4rkme551ah, Вы писали:
D>А вот за кририку огромно спасибо, теперь я хотя бы знаю в каком направлении работать и где у меня слабые места.
Если нет упомянутой недельки на рефакторинг, то на собеседовании можно пофантазировать "как бы я улучшил этот код" по мотивам полученных тут советов. IMHO подобные рассуждения могут сыграть куда более решающую роль на собеседовании нежели сам код.
D>Но мне что-то расхотелось идти на собеседование
Здравствуйте, kochetkov.vladimir, Вы писали:
KV>Здравствуйте, d4rkme551ah, Вы писали:
D>>А вот за кририку огромно спасибо, теперь я хотя бы знаю в каком направлении работать и где у меня слабые места.
KV>Если нет упомянутой недельки на рефакторинг, то на собеседовании можно пофантазировать "как бы я улучшил этот код" по мотивам полученных тут советов. IMHO подобные рассуждения могут сыграть куда более решающую роль на собеседовании нежели сам код.
Мне нравится вопрос "Что Вам нравится в этом коде?", "Что Вам не нравится в этом коде?"
* thriving in a production environment *
Сделал потом удолил.
