×Закрыть

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

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

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

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

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

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

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

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

Почему не сделали возможность того что бы Run принимала список того что нужно заранить, вместо того что бы считывать stdin?

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

немного сложновато иногда — руны, байты,

facepalm

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

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

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

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

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

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

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

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

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.

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

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

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

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

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

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

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

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

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

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 );
}

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

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

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

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

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

:)

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

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

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

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

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

Но может быть достаточно, чтобы не давать :)
Про стиль ничего не могу сказать, так как на го не пишу, но как-то боевой код обычно _сильно_ иначе выглядит (включая для начала доку для экспортируемых типов/функций). Но там и по существу много вопросов прямо на поверхности:
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 файл не может в реале не иметь метаданных?
(вся логика вокруг плейлиста выглядит странно — зачем его писать в файл чтобы просто тут же прочитать обратно и воспроизвести? но возможно ТЗ было так написано)

Хорошо, признаюсь, я немного поспешил, т.к. вакансии на 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 :)

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

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

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

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

после закрытия все данные будут утеряны

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

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

Добавлю: 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. Тогда можно на что-то расчитывать.

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