Вхід і реєстрація
Розробка · 5 липня 2018, 14:10 2899
Denis, Do what you love, use what you love

Помогите с code review (Go)

Всем привет. Я занимаюсь веб-разработкой более 5 лет. В основном backend. Стек стандартный — php, js, html и т.д. Некоторое время назад начал изучать Go. Наткнулся на одном из сайтов по поиску работы на позицию джуниора. Скинул им ссылку на github github.com/netseac/player. Получил отказ без объяснения причины. Я конечно понимаю что я пишу на Go несколько месяцев и мой код далек от идеала, но неужели все настолько плохо?

-----------------------------
Всем спасибо за отзывы, не ожидал что их будет так много :)

Теми: code review, Go
👍ПодобаєтьсяСподобалось0
До обраногоВ обраному0
LinkedIn
Дозволені теги: blockquote, a, pre, code, ul, ol, li, b, i, del.
Ctrl + Enter
Дозволені теги: blockquote, a, pre, code, ul, ol, li, b, i, del.
Ctrl + Enter
Denis 13.07.2018 20:39

Отрефакторил немного :)

Відповісти
Підтримати
Alex Furman 10.07.2018 23:59

думаю через копіпасту коду , стандартний приклад якого багато на гітгабі (imho), напішіть щось своє і пошліть ще раз , запитайте що було не так з прикладом, попросіть поради їхнього ліда що вчити, над чим працювати, don’t take no for the answer ;)

Відповісти
Підтримати
Denis 13.07.2018 20:42

Спасибо, но это не имеет смысла. Вакансий очень мало, а тягаться с сеньерами это не мое(пока что во всяком случае). Поэтому пишу в стол — Just for Fun.

Відповісти
Підтримати
Alex Furman
Евгений Козинский 06.07.2018 14:59

Почему не сделали возможность того что бы Run принимала список того что нужно заранить, вместо того что бы считывать stdin? Мне кажется что Ваша либо упростилась бы и стала гибче в использовании.
На мой взгляд лучше бы вообще отказаться от panic и os.Exit в сторону просто возвращения ошибок, думаю, что сложно будет конечному пользователю захендлить все это. Определити перечесления с кодами ошибок/неудачного возарщения программы. Хорошо задокументируйте это.
Избегайте магических цифр (0666)
Больше документирующих комментариев)

Відповісти
Підтримати
Andrew Sevastianov 09.07.2018 13:25
Почему не сделали возможность того что бы Run принимала список того что нужно заранить, вместо того что бы считывать stdin?

В stdin куда проще передавать: cat file | play или find $directory -type f -name "*.in" | grep -v kirkorov | play

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

Ну... Твой совет не в духе философии UNIX: программа должна делать одну вещь, но должна делать её хорошо. Для утилит командной строки это нормальное поведение. И в чём проблема пользователю запустить процесс и отследить код возврата?

Избегайте магических цифр (0666)

А что тут магического, когда это общепринятая практика? chmod 666 ...

Хорошо задокументируйте это.
Больше документирующих комментариев)

И так вроде понятно... Документация будет выглядеть злой тавтологией :)

Відповісти
Підтримати
Евгений Козинский
Евгений Козинский 09.07.2018 16:49
В stdin куда проще передавать: cat file | play или find $directory -type f -name «*.in» | grep -v kirkorov | play

Например если я хочу что бы проигрывание срабатывало не только тогда когда ранится программа с определенными параметрами? Что если я хочу принимать список проигрывания откуда то извне, а потом проигрывать или например хочу насписать таску с каким то интервалом?

Ну... Твой совет не в духе философии UNIX: программа должна делать одну вещь, но должна делать её хорошо. Для утилит командной строки это нормальное поведение. И в чём проблема пользователю запустить процесс и отследить код возврата?

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

И так вроде понятно... Документация будет выглядеть злой тавтологией :)

Программы всегда дополняются и расширяются и проще поддерживать уже существующие доки, чем писать все с нуля когда код усложнился и стал пообъемнее. К тому же читать доки не обязательно если не хочется, но лично я предпочитаю прочитать одно предложение с описанием того что делает функция, а не читать всесь её код. Комменты всегда помогают при поддержки кода.

Відповісти
Підтримати
Andrew Sevastianov
Denis 09.07.2018 16:54

Я специально делал все как можно проще для конечного пользователя, у меня не было цели написать второй foobar или что-то вроде этого :)

Відповісти
Підтримати
Евгений Козинский
Aliaksandr Valialkin программист в VictoriaMetrics 10.07.2018 15:55
Программы всегда дополняются и расширяются и проще поддерживать уже существующие доки, чем писать все с нуля когда код усложнился и стал пообъемнее. К тому же читать доки не обязательно если не хочется, но лично я предпочитаю прочитать одно предложение с описанием того что делает функция, а не читать всесь её код. Комменты всегда помогают при поддержки кода.

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

  • Перед public API функциями, чтобы пользователи понимали, как работать с предоставленным API. Public API не принято ломать, поэтому комменты к нему не должны устареть
  • Перед запутанным / сложным кодом, чтобы лучше понять, как он работает
  • Перед кодом, который несколько раз пытались безуспешно улучшить, с аргументацией, почему код написан именно так, а также с перечислением неудачных попыток по его улучшению

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

Відповісти
Підтримати
Евгений Козинский
Andrew Sevastianov 10.07.2018 17:49
Например если я хочу что бы проигрывание срабатывало не только тогда когда ранится программа с определенными параметрами?

Не UNIX way. Чем плох запуск программы? Просто что мне делать с библиотекой??? Я хочу запустить проигрывание аудиофайлов, и ты предлагаешь мне изучить API библиотеки, написать собственное приложение да ещё на языке, с которым этот API совместим. И только тогда я смогу послушать музыку? И потом если я захочу фильтры, мне надо переписать свою программу, если я захочу добавить перемешивание, мне тоже надо менять код? Это жутко неоптимально, потому что программы, которые реализуют перемешивания, фильтрацию уже есть в UNIX: shuf и grep.

Что если я хочу принимать список проигрывания откуда то извне, а потом проигрывать или например хочу насписать таску с каким то интервалом?

Используй пайпы (см. примеры выше)

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

Это не либа, это утилита командной строки. У неё другое назначение. Другой процесс никак не может повтиять на твоё драгоценное приложение. Если она закрешится, твой процесс получит код возврата и сможет продолжить работу.

лично я предпочитаю прочитать одно предложение с описанием того что делает функция

Мой опыт подсказывает, что в подавляющем большинстве случаев я и так знаю что делает эта функция: названия, списка параметров и контекста вызова обычно более чем достаточно. А вот основная проблема состоит в том, чтобы понять, как именно она это делает...

Відповісти
Підтримати
Евгений Козинский
Евгений Козинский 10.07.2018 18:14

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

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

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

Відповісти
Підтримати
Andrew Sevastianov
Andrew Sevastianov 10.07.2018 19:17
Ведь что бы эта утилита заработала, нужно скачать и установить голанг, научится комплировать

Golang компилирует в бинарник, который и можно скачать, поместить в ppa-репозиторий и т. д.

А как быть еще с юзверями на windows

А у пользователей Windows своя песочница, в которой которой они живут без пайпов со своим .NET и своими правилами игры делать библиотеки, как было указано Вами. Но лично для меня их проблемы меня не волнуют чуть менее чем никак. Для меня очевидно, что это утилита командной строки. И если кто-то под Windows хочет использовать утилиты командной строки, тот должен уметь это готовить. Ну и вообще для golang библиотек обычно UNIX в приоритете, комьюнити под Linux больше, а Windows часто поддерживается по остаточному принципу.

Это избыточный код, для такого рода программ.

Это типичная практика на golang. В конечном счёте куда проще сразу пробросить все ошибки, чем потом, когда возникнет необходимость. Допустим надо отловить в процессе развития либы один из вариантов (file not found, 2). Может оказаться неприятным начать возсхожение от os.Exit и везде менять сигнатуру, добавлять туда error + проверку. И где-то пропустить и словить на production. Куда проще это делать не отходя от кассы.

Опять же, это учебный демонстрационный код, и вряд ли его следует рассматривать как серьёзное приложение, готовое к работе.

Відповісти
Підтримати
Евгений Козинский
Oksana Chuiko Software Engineer в Pure Storage 05.07.2018 23:42

— зачем оставлять закомментированный код? это мусор.
— плохая обработка ошибок. Где-то panic(), где-то return err.
— Step 2 уже должен быть в коде

Ничего мегаужасного не вижу, но кода маловато, чтобы вообще что-то понять

Відповісти
Підтримати
Олексій Пєніє 05.07.2018 20:11

Один отказ Джуну? Эка невидаль. Поузнавай количество вас на одну вакансию, станет понятно что и 20 отказов к ряду не предел, даже если джун вменяемый.

Відповісти
Підтримати
Kyrylo Romanenko QA Engineer 05.07.2018 19:36

Еще странно выглядит, что имея опыт работы 5 лет вы подаетесь на junior. Все-таки смена языка программирования не должна скидывать на уровень junior, когда по местным меркам уже на сеньора претендовать принято.

Відповісти
Підтримати
Denis 05.07.2018 19:51

Ну для меня после php немного сложновато иногда — руны, байты, хеш-таблицы, потоки и т.д. Все же Go более низкоуровневый язык. Делая те же интернет магазины или порталы средней сложности на php фреймворке за столько лет лично я с ними не особо сталкивался. Хотя вру конечно — совсем не сталкивался :)

Відповісти
Підтримати
Kyrylo Romanenko
Vlad Stelmahovsky Software Engineer в Verity AG 06.07.2018 11:43
немного сложновато иногда — руны, байты,

facepalm

Відповісти
Підтримати
Denis
Alex 07.07.2018 13:07

Компаниям удобно, есть повод скинуть зп :) Попробуй докажи что не джун, без коммерческого опыта на другом языке.

Відповісти
Підтримати
Kyrylo Romanenko
Andrew Sevastianov 05.07.2018 16:43

У меня несколько вопросов по использованию git:

1. По сути рабочий код это один коммит, и это не очень хорошо. Обычно что-то законченное сделал, написал что, закоммитил. Да, тут вопрос немного спорный, возможно это можно написать залпом без отладки для про, но... если отправлять задание на ревью, то лучше быть более педантом.
2. Описания коммитов радуют. Зачем надо четыре одинаковых коммита подряд с одинаковым описанием? Почему их нельзя фиксапнуть в один?
3. Уже говорили, что всякие отладочные закоментированные вещи не должны быть видны.
4. Именование переменных не ахти, очень уж танственно выглядит многие названия.
5. Последовательные вызовы setData/getData мне не нравяться. Вообще, непонятно, как можно использовать сгенерированный json? А если это требование ТЗ, то читаем последовательность ближе такая: (1) читаем данные во внутреннее представление (2) записываем json (3) идём проигрывать без всякого чтения.

Получил отказ без объяснения причины.

Без объяснения причины скорее это относится к HR, потому что хотя бы отписать «спасибо, мы остановили выбор на другом кандидате» не должно быть большой проблемой. Вот только было бы это для Вас легче и яснее?

Відповісти
Підтримати
Denis 05.07.2018 17:02

1. Это минимально рабочая версия. Как видите далеко не идеальная как оказалось :)
2. Спешил немного...
3. Приму к сведению, спасибо. Хотя тогда все же проще иметь две ветки я так понимаю.
4. В Go непринято давать длинные названия переменным.
5. Ну чтобы каждый раз ФС не дергать.

было бы это для Вас легче и яснее?

Думаю да. Неужели сложно указать причину отказа ?

Відповісти
Підтримати
Andrew Sevastianov
Andrew Sevastianov 05.07.2018 20:06
3. Приму к сведению, спасибо. Хотя тогда все же проще иметь две ветки я так понимаю.

Можно просто не добавлять в коммит отладочный код

В Go непринято давать длинные названия переменным.

Это не означает, что все переменные должны называться Data :) Во-первых, загрузка из файла это не get, а скорее load. Во-вторых, у тебя данные это либо File, либо Playlist. Так что loadFiles или loadPlaylist куда более читабельно и понятно. Аналогично, set обычно предполагает код сеттера, который не содержит очень большой логикой, и уж точно не лезет в файлы. Тут хочется разбить код на readDir<code> или <code>readPlaylistDir и на saveFiles или savePlaylist. А вообще я бы всё делал в такой последовательности:

var playlist []Mp3File // File путается с обычными файлами
if IsDir(path) {
    readDir(path, playlist) // Читаем список файлов из директоории
} else  {
    loadPlaylist(path, playlist) // Нам передали файл, полагаем что это наш JSON плейлист
}
savePlaylist(playlist, filename) // Сохраняем плейлист
play(playlist) // воспроизводим его

Ну или даже просто load, readFromDir, save, play. Кстати, напрашивается методы засунуть внутрь твоего недоделанного класса playlist.

Відповісти
Підтримати
Denis
Denis 05.07.2018 20:22

Спасибо за конструктивную критику :) Будет время, сяду, допилю. Сейчас сил уже совсем нет.

Відповісти
Підтримати
Andrew Sevastianov
schwarzlichtbezirk 05.07.2018 21:39
Будет время, сяду, допилю. Сейчас сил уже совсем нет.

Я что-то не пойму, вы написали 150 строк джуновского кода, и у вас уже сил нет? Это при том, что Go учится за пару дней, затем можно просто сидеть и писать код особо не напрягаясь над конструкциями языка.

Відповісти
Підтримати
Denis
Владислав Галий Android Developer 06.07.2018 09:50

В этих 150 строках 25 раз встречается "nil’, я бы тоже устал.

Відповісти
Підтримати
schwarzlichtbezirk
Denis 06.07.2018 10:34

Вы видимо из тех кто не отдает ничего при ошибке и предпочитает гадать на кофейной гуще в случае чего ? Хотя не спорю, ваш подход также имеет право на существование :)

Відповісти
Підтримати
Владислав Галий
Владислав Галий Android Developer 06.07.2018 10:46

Дядь, не нужно делать какие-то выводы о моём профессионализме из-за простой шутки, будь проще.

Відповісти
Підтримати
Denis
Andrew Sevastianov 06.07.2018 10:42

Ну... это Go, тут так принято. Почти каждый метод возвращает error одним из параметров, nil это отсутствие ошибки

Відповісти
Підтримати
Владислав Галий
Oksana Chuiko Software Engineer в Pure Storage 07.07.2018 20:28

Это нормально в го коде. Хороший продакшн код вообще на 50% из обработки ошибок состоит

Відповісти
Підтримати
Владислав Галий
Sierżant Kwiatkowski Software Engineer в TS Imagine 09.07.2018 12:58

Кстати не только в го коде, а в коде вообще. Думаю ни для кого не новинка код вроде:

void doSomething( p1, p2 ) {
  if( isNull( p1, p2 ) ) 
    throw SomethingIsNull();
  var x = getOther();
  if( x * p1 < 0 )
    throw SomethingInvalid();
  if (getMoonPhase() instanceof Eclipse)
    return;

  finallyDoSomethingUseful( p1, p2 );
}
Відповісти
Підтримати
Oksana Chuiko
Denis 05.07.2018 16:50

Нет тз, писал для себя, чисто в учебных целях. А до тестового задания дело недошло, мне просто отказали без указания на то причины. Хотя теперь понял почему :(

Відповісти
Підтримати
Yury Gavrilenko
restart 93 05.07.2018 15:19

мало фіч. потрібно ще можливість рандомного тасування, хавання списку файлів

а може у них тестер лінивий і прочитав тільки 3 пункт)

Відповісти
Підтримати
Denis 05.07.2018 15:35

Я придерживался принципа KISS. Причем стараюсь придерживаться его не только при написании кода, но и в целом по жизни :)
Есть куча навороченных плееров, а хотелось чего-то простого.

а може у них тестер лінивий і прочитав тільки 3 пункт)

:)

Відповісти
Підтримати
restart 93
Aleksandr Marchenko some developer 05.07.2018 14:32

Да, Ваш кода не идеален, и даже в некоторых моментах — небрежен. Это действительно может поставить под сомнение

Я занимаюсь веб-разработкой более 5 лет

Но я сильно сомневаюсь, что решение было принято на основе Вашего кода и его стиля. Того, что Вы выложили на гитхабе — просто недостаточно, чтобы делать какие-то выводы или давать офер (обычные поделки-пятиминутки).
Ищите причины в собеседовании/нахождении более подходящего кандидата/своем резюме и т.д.

вот эта штука может указать на большое ко-во ошибок, пользуйтесь — github.com/golang/lint
а еще есть связка — go vet, gofmt, golint

Відповісти
Підтримати
Konstantin Strukov 05.07.2018 14:59
Того, что Вы выложили на гитхабе — просто недостаточно, чтобы делать какие-то выводы или давать офер (обычные поделки-пятиминутки).

Но может быть достаточно, чтобы не давать :)
Про стиль ничего не могу сказать, так как на го не пишу, но как-то боевой код обычно _сильно_ иначе выглядит (включая для начала доку для экспортируемых типов/функций). Но там и по существу много вопросов прямо на поверхности:
github.com/...​blob/master/player.go#L17 — зачем, если дальше не юзается?
github.com/...​blob/master/player.go#L93 — пишем результат в файл до проверки возможной ошибки?
Наименование функций — огонь (setData/getData на самом деле создает/загружает плейлист).
Создание плейлиста бажное:
github.com/...​blob/master/player.go#L69 — а если музычка разложена по сабфолдерам?
github.com/...​blob/master/player.go#L79 — а что, «играбельный» mp3 файл не может в реале не иметь метаданных?
(вся логика вокруг плейлиста выглядит странно — зачем его писать в файл чтобы просто тут же прочитать обратно и воспроизвести? но возможно ТЗ было так написано)

Відповісти
Підтримати
Aleksandr Marchenko
Denis 05.07.2018 15:26

Хорошо, признаюсь, я немного поспешил, т.к. вакансии на Go джуниоров разметаются как горячие пирожки. Я мониторю вакансии больше месяца везде ниже сеньеров никто не нужен особо. Мидл минимум :(

github.com/...​blob/master/player.go#L17 — зачем, если дальше не юзается

На вырост же :)

github.com/...​blob/master/player.go#L93 — пишем результат в файл до проверки возможной ошибки?

Да, тут согласен.

github.com/...​blob/master/player.go#L69 — а если музычка разложена по сабфолдерам?

Это не баг, это фича :) Я просто немного упростил себе задачу...

github.com/...​blob/master/player.go#L79 — а что, «играбельный» mp3 файл не может в реале не иметь метаданных?

Ну у меня валяется несколько таких на компьютере :)

зачем его писать в файл

Хотел в slice или map запихнуть, но тогда при создании плейлистов(подразумевалось что он будет не один)

  1. после закрытия все данные будут утеряны
  2. будет использоваться много RAM
  3. каждый раз нужно будет пересоздавать все списки
возможно ТЗ было так написано

Учебный проект, Just for Fun :)

Відповісти
Підтримати
Konstantin Strukov
schwarzlichtbezirk 05.07.2018 15:57

Только остаётся загадкой, зачем пейсать плейлист в json. Если уже пишите плейлист, то хотя бы в какой-нибудь распространённый формат. Причём пишется в текущую директорию, которая может быть где угодно. Да и вообще проект ни о чём. Надо было написать какой-нибудь небольшой сервер для одностраничного приложения.

Відповісти
Підтримати
Denis
Denis 05.07.2018 17:04

По вашему JSON не распространенный формат ?

Відповісти
Підтримати
schwarzlichtbezirk
schwarzlichtbezirk 05.07.2018 21:49

Писать в форматы, распознаваемые другими плеерами, например m3u, pls. А какой вообще смысл писать этот файл, если даже вы сами его не читаете? И соответственно список воспроизведения пользователем никак не управляется.
p.s. тут выходит как в одном бородатом анекдоте: чукча писатель, а не читатель.

Відповісти
Підтримати
Denis
Kyrylo Romanenko QA Engineer 05.07.2018 16:02

В мастер ветке не оставляют кусков кода «на вырост» и закоментированных принтов. Так вы показываете, что не умеете работать с контролем версий и поддерживать порядок в коде.

Відповісти
Підтримати
Denis
Konstantin Strukov 05.07.2018 16:09
после закрытия все данные будут утеряны

А они и так «утеряны». При запуске вы не проверяете наличие готового плейлиста и каждый раз собираете его заново. Таким образом, даже вполне здравая идея убита реализацией.

Відповісти
Підтримати
Denis
Denis 05.07.2018 15:07

Спасибо. Если ткнете носом в небрежность/неидеальность буду благодарен. Это мои первые проекты на Go, очень интересно мнение более опытных (как я понимаю) коллег. Опыт написал общий включая первые шаги в html. На Go пишу месяца 2-3. Т.е. по сути на гитхабе все что я делал на нем. Сложнее плеера ничего придумать не смог. Хотел сделать проект с распознаванием речи, но такой уже есть, так что на гитхабе только черновик его :)

Відповісти
Підтримати
Aleksandr Marchenko
Ivan Syniavskyi Sr. SWE-SRE в Google 07.07.2018 23:30

Добавлю: 1) func Run() -> main() 2) _, err := setData(path) — never returns []File — why it is needed? 3) //fmt.Printf("Path %v„, path) comments in production code? 4) missing unit tests 5) missing comments for functions 6) os.Exit(1) should not be used outside of main but return error. 7) Список еще можно долго продолжать.... там по хорошему коментов 20 минимум. Из советов: Читайте effective go, „The Go Programming Language”. В вашем коде покажите умение работать с unit tests, concurrency, etc. Тогда можно на что-то расчитывать.

Відповісти
Підтримати
Denis

Підписатись на коментарі

Не підписуватись
Скористайтесь акаунтом
×
з умовами використання сайту і політикою конфіденційності.