Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

1.3. Выравнивание присвоений переменных и элементов массива #18

Open
peter-gribanov opened this issue Dec 19, 2019 · 5 comments
Milestone

Comments

@peter-gribanov
Copy link

peter-gribanov commented Dec 19, 2019

1.3. Выравнивание присвоений переменных и элементов массива

Довольно спорное решение. Я бы сказал вкусовщина. Я когда-то очень давно применял этот подход и результат мне не понравился. Улучшается читаемость только если название переменных (ключей массива) и присваиваемые им значения имеют примерно одинаковую длину.

$inside_left_floating_width   = 0;
$inside_right_floating_width  = 0;
$outside_left_floating_width  = 0;
$outside_right_floating_width = 0;

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

$varName                            = 'varName';
$secondVariableWithVeryLongNameHere =
    '123456790123456790123456790123456790123456790123456790123456790123456790123456790';

Другой утрированный пример более наглядно демонстрирующий проблему.

$user_access_token    = $user_access_token_storage->get();
$i                    = 1;
$has_promote_articles = false;
$result               = [];
$ids                  = [];
$categories           = [];
foreach ($article_rep->slice($start, self::PER_PAGE) as $article) {
   // ...

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

$choice_translation_domain = 'article';
$total                     = $article_rep->countOf(
    new PublishedArticles(new \DateTimeImmutable()),
    new Cache(self::CACHE_TTL)
);

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

                            $descendant_delimeter = strrpos($query, '::');
                            $isChild              = substr($query, $descendant_delimeter - 5, 5) === 'child';
                            $el                   = substr($query, $descendant_delimeter + 2);
                            $query                = substr($query, 0, strrpos($query, '/')).
                                ($isChild ? '/' : '//').$el;
                            // потребовалось сделать перенос так как уперлись в 120 символов

                            $p   = $i + 1;
                            $nth = trim(mb_substr($selector, $p, strpos($selector, ')', $i) - $p));
                            // другой логический блок переменных который имеет свои отступы и
                            // вступает в диссонанс с предыдущим блоком из-за разницы длинны в отступах

Так диссонанс будет нагляднее

$descendant_delimeter = strrpos($query, '::');
$isChild              = substr($query, $descendant_delimeter - 5, 5) === 'child';
$el                   = substr($query, $descendant_delimeter + 2);
$query                = substr($query, 0, strrpos($query, '/')).($isChild ? '/' : '//').$el;

$p   = $i + 1;
$nth = trim(mb_substr($selector, $p, strpos($selector, ')', $i) - $p));

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

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

@index0h
Copy link
Owner

index0h commented Dec 19, 2019

Спасибо за комментарий, я все больше склоняюсь к тому, что конкретно этот пункт вовсе стоит удалить из рекомендаций. Этот пункт слишком спорен и субъективен. Ваш же пример в двух видах оформления, мне субъективно читать проще с отступами. Физически проще за счет того, что есть 2 колонки, в зависимости от того, что мне надо (переменная/значение) - я читаю правую, или левую колонку. В случае, когда отступов нет - визуально нужно разделять каждую строку. Объективный минус отступов в том, что в коммит попадут помимо изменяемых строк еще и отформатированные, это усложняет code review.

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

$user_access_token    = $user_access_token_storage->get();
$i                    = 1;
$has_promote_articles = false;
$result               = [];
$ids                  = [];
$categories           = [];

$user_access_token = $user_access_token_storage->get();
$i = 1;
$has_promote_articles = false;
$result = [];
$ids = [];
$categories = [];

@VladReshet
Copy link

Я не автор, но попробую поспорить)

$query                = substr($query, 0, strrpos($query, '/')).
                                ($isChild ? '/' : '//').$el;
                            // потребовалось сделать перенос так как уперлись в 120 символов

Тут плохое оформление - дело десятое, тут код: substr($query, 0, strrpos($query, '/')). ($isChild ? '/' : '//').$el - нечитаемый абсолютно. Отсюда и в 120 упёрлись

А вот тут - частично согласен. Не надо принудительно выравнивать сильно разные строки. Лучше наоборот разделить их пустой строкой, чтобы не слипалось визуально

// плохо
$varName                            = 'varName';
$secondVariableWithVeryLongNameHere =
    '123456790123456790123456790123456790123456790123456790123456790123456790123456790';

// лучше
$varName = 'varName';

$secondVariableWithVeryLongNameHere = '123456790123456790123456790123456790123456790123456790123456790123456790123456790';

@index0h
Copy link
Owner

index0h commented Dec 19, 2019

В принципе можно и дополнить этот пункт комментарием @VladReshet , получится что-то типа такого

$user_access_token    = $user_access_token_storage->get();
$has_promote_articles = false;

$result     = [];
$categories = [];

$i   = 1;
$ids = [];

или такого

$user_access_token    = $user_access_token_storage->get();
$has_promote_articles = false;

$i          = 1;
$result     = [];
$ids        = [];
$categories = [];

@peter-gribanov
Copy link
Author

Физически проще за счет того, что есть 2 колонки, в зависимости от того, что мне надо (переменная/значение) - я читаю правую, или левую колонку.

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

image

Аналогичная проблема есть и в классических таблицах и есть вполне распространенные решения этой проблемы.

image
image
image
image

@index0h
Copy link
Owner

index0h commented Dec 19, 2019

@peter-gribanov убедили

@index0h index0h added this to the 2.0 milestone Dec 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants