LINUX.ORG.RU

Поругайте код

 ,


0

3

Привет!

Учу по вечерам Go, чтобы разобраться с типовыми вариантами использования языка, решил набросать небольшую БД (https://github.com/lochnessathome/RETIA).

Нужна критика моего подхода к разбиению на пакеты, именования переменных, работы со структурами данных и вообще всего :)

P.S. Купил учебник Кернигана, надеюсь найти ответы там, но это труднее и дольше.

Не вдаваясь в детали, пару вещей, которые сразу бросились в глаза:

  1. неправильные import пути. Должно быть «github.com/lochnessathome/RETIA»
  2. имя пакета должно быть предпочтительно в нижнем регистре
  3. Small Package Syndrome — много пакетов однострочников (и без тестов!) — перераспредели код не по содержанию, а по поведению.
beastie ★★★★★ ()
Последнее исправление: beastie (всего исправлений: 2)
Ответ на: комментарий от beastie

1, 2 - ясно.

3 - большая часть пакетов отвечает за работу с одноимёнными сущностями.

Relation (отношение-таблица) есть два метода: создание и вычисления.

Почему это неправильно?

lochness ()
Ответ на: комментарий от lochness

Пардон, у большинства операций над множествами есть свой пакет (класс? сущность?), который выполняет только те функции, которые относятся к этой операции.

Хотел воссоздать STI без классов.

lochness ()

Много избыточных конструкций

Например, вместо

statement := new(unit.RenameStatement)
statement.Relation = relation
statement.Expressions = expressions

нет смысла не делать так:

statement := &unit.RenameStatement{
    Relation: relation,
    Experssions, expressions,
}

Так же не нужно подобные return оформлять в else: https://github.com/lochnessathome/RETIA/blob/master/rename/rename.go#L31

Тут я бы не ставил скобки: https://github.com/lochnessathome/RETIA/blob/master/component/component.go#L29

derlafff ★★★★★ ()
  1. Забыть все, что ты делал раньше в си.
  2. Выше уже указали на импорт пакетов.

    К примеру: https://github.com/lochnessathome/RETIA/blob/master/join/join.go

  3. Огромный вложенный блок if, тогда как если написать if value != nil { return } будет гораздо меньше вложенности
  4. Else не нужен после оператора return
  5. snake case, все должно быть camelCase
  6. строка 122: почему сразу не сделать return?
  7. findCommonAttributes(lrelation, rrelation *unit.Relation) Сразу видно насление си. го не си. У структуры Relation можно создать метод findCommonAttributes(Relation)
  8. Возврат пустых значений и невозврат ошибки при этом.
  9. И самое важное, в общем-то, из-за чего придется переписать половину кода: не используй функции и переменные уровня пакетов.

    Функции и переменные уровня пакетов, это:

    • Тоже самое, что статические классы и свойства в жаве\шарпе.
    • Это делает твой код нетестируемым.
    • Приводит к бек-сайд эффектам
    • Потоконебезопасны
    • Тоже самое, что глобальные переменные в жаваскрипте.
    • Выворачивание потрохов реализации (нарушение инкапсуляции)
    • Позволяет манки-патчить, т.к. глобальные переменные перезаписываемые.

Короче, как и в шарпе\жаве, переменные\функции уровня пакетов, не должны хранить или изменять состояние сущностей, только передавать.

nikolnik ★★★ ()
Последнее исправление: nikolnik (всего исправлений: 1)
Ответ на: комментарий от nikolnik

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

А можно пример? И почему это плохо?

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

lochness ()

Спасибо за ревью всем. Это очень сэкономило моё время.

lochness ()
Ответ на: комментарий от lochness

http://wiki.c2.com/?GlobalVariablesAreBad

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

Глобальные переменные в го используются как замена енумов (через const), но не примитивы не могут быть const в го, в таких случаях и приходится обходиться глобальными значениями, которые могут быть изменены, но никогда не надо это делать.

Функции уровня пакетов можно и нужно использовать. Только эти функции не должны изменять значения переменных уровня пакета и хранить состояние. Типичный пример ф-ии уровня пакетов - это конструктор анонимной структуры.

В коде у тебя не видел, но так же избегай функции init() уровня пакета, это тоже самое, что статический конструктор.

https://peter.bourgon.org/blog/2017/06/09/theory-of-modern-go.html

nikolnik ★★★ ()
Вы не можете добавлять комментарии в эту тему. Тема перемещена в архив.