LINUX.ORG.RU

Оптимизация куска кода

 , ,


0

1

Доброе утро, ЛОР.

Задача: Плагин авторизации для Moodle. Плагин проверяет на внешней системе идентификационные данные пользователя. Если результат true, авторизует пользователя, иначе отклоняет авторизацию.

Особенность: Moodle работает только с локальными профилями. Если авторизация разрешена, а локальный профиль отсутствует, пользователя потребуется создать.

В начале был такой код:

public function user_login($username, $password) {
    if (!auth_billing::check_user($username, $password)) {
        return false;
    }

    if (!$user = get_complete_user_data('email', $username)) {
        if (!auth_billing::create_user($username)) {
            return false;
        }

        $user = get_complete_user_data('email', $username);
    }

    complete_user_login($user);
    self::redirect();
}

Авторизация происходит по адресу электронной почты и паролю пользователя. На версии Moodle 3.5+ код работает, как и ожидается. На старых версиях Moodle всплыл нюанс: В функцию передаётся адрес эл. почты только тогда, когда локальный профиль отсутствует. В остальных случаях, передаётся логин существующего пользователя.

Функция была переписана таким образом:

public function user_login($username, $password) {
    if (!validate_email($username)) {
        if (!$user = get_complete_user_data('username', $username)) {
            return false;
        }

        if (!auth_billing::check_user($user->email, $password)) {
            return false;
        }
    } else {
        if (!auth_billing::check_user($username, $password)) {
            return false;
        }

        if (!$user = get_complete_user_data('email', $username)) {
            if (!auth_billing::create_user($username)) {
                return false;
            }

            $user = get_complete_user_data('email', $username);
        }
    }

    complete_user_login($user);
    self::redirect();
}

Вопрос: Это работает и выполняет возложенную задачу. Не устраивает количество условий и уровень вложенности. Существуют способы оптимизации этого кода?

Ну поменяй

if (!lalala) {
     return false;
}
на
if (!lalala) return false;
Убери else там, где он не нужен
    if (!$user = get_complete_user_data('email', $username)) {
        if (!auth_billing::create_user($username)) {
            return false;
        }

        $user = get_complete_user_data('email', $username);
    }
Вот тут не знаю, нужно ли присваивать $user второй раз, по идее присвоение должно произойти в условии. Тогда второе условие можно просто прицепить в первое (&&) или сперва присвоить $user, а потом уже плясать. Вроде в php есть еще конструкции типа $condition or return false;
В целом, не понимаю в чём проблема. Какая разница сколько там if...else. Такой код пишется один раз, а потом его никто никогда не открывает.

crutch_master ★★★★★ ()
Последнее исправление: crutch_master (всего исправлений: 4)

А вообще

public function user_login($username, $password) {
    if (!validate_email($username)) {
        if (!$user = get_complete_user_data('username', $username)) {
            return false;
        }

        if (!auth_billing::check_user($user->email, $password)) {
            return false;
        }
    } else {
        if (!auth_billing::check_user($username, $password)) {
            return false;
        }

        if (!$user = get_complete_user_data('email', $username)) {
            if (!auth_billing::create_user($username)) {
                return false;
            }

            $user = get_complete_user_data('email', $username);
        }
    }

    complete_user_login($user);
    self::redirect();
}
== (not tested)
public function user_login($username, $password) {
    $user = get_complete_user_data('username', $username)
    if (!validate_email($username) && 
         (!$user || 
          !auth_billing::check_user($user->email, $password))
       ) {
            return false;
    }
 
    if (!auth_billing::check_user($username, $password) ||
        !$user || !auth_billing::create_user($username)
        ) {
            return false;
    }
    complete_user_login($user);
    self::redirect();
}
что в свою очередь ==
public function user_login($username, $password) {
    $user = get_complete_user_data('username', $username);
    if (!$user || 
        !auth_billing::check_user($username, $password) ||
        !auth_billing::create_user($username)) {
        return false;
    }
    complete_user_login($user);
    self::redirect();
}
Какой-то бред или там все функции на сайд-эффектах, тогда бессмысленно что-то придумывать - всё равно дерьмо получится.

crutch_master ★★★★★ ()
Последнее исправление: crutch_master (всего исправлений: 2)

В функцию передаётся адрес эл. почты только тогда, когда локальный профиль отсутствует. В остальных случаях, передаётся логин существующего пользователя.

Мда. Крайне костыльное поделие. Не могли $email добавить в параметры.

crutch_master ★★★★★ ()

Не устраивает количество условий и уровень вложенности. Существуют способы оптимизации этого кода?

Короче. Напиши комментарии, что этот код делает и оставь так. Можно поменять

if ($c1) {
   if ($c2) return false;
   op();
}
на
if ($c1 && $c2) return false;
op();
Но так - лучше читается, кмк.
Ну или if ($c1) {op();} на $c1 or op();.

crutch_master ★★★★★ ()
Последнее исправление: crutch_master (всего исправлений: 2)

auth_billing::create_user - твой метод? Возвращай из него объект user, тогда не понадобится по новой подгружать данные после создания.

А остальное вроде всё по делу. Если для создания учётки тебе обязательно нужен email, а по логину нельзя.

Ivan_qrt ★★★★★ ()

Я б разбил на два метода. На уровне главное if/else.

anonymous ()

Можно ещё так попробовать.

public function user_login($username, $password) {
    $mail = $username;
    if (!validate_email($username)) {
        if (!$user = get_complete_user_data('username', $username)) {
            return false;
        }
        $mail = $user->email;
    }
    if (!auth_billing::check_user($mail, $password)) {
        return false;
    }

    if (!$user && !$user = get_complete_user_data('email', $mail)) {
        if (!$user = auth_billing::create_user($username)) {
            return false;
        }
    }

    complete_user_login($user);
    self::redirect();
}
Ivan_qrt ★★★★★ ()
Ответ на: комментарий от anonymous

auth_billing::create_user - твой метод?

Да, мой.

Можно ещё так попробовать.

Хороший пример, нет глубокой вложенности, спасибо.

Adeptus-Mechanicus ()
Ответ на: комментарий от crutch_master

Какой-то бред или там все функции на сайд-эффектах, тогда бессмысленно что-то придумывать - всё равно дерьмо получится.

Спасибо за варианты, человек-костыль. Теперь уверен, что не упустил нюансы.

Adeptus-Mechanicus ()
Ответ на: комментарий от Adeptus-Mechanicus

Спасибо за варианты, человек-костыль.

Обращайся, если что.

crutch_master ★★★★★ ()

Читать «Совершенный код». А конкретно правило про 1 точку выхода из функции. Вообще, это правило не обязятельно, но в вашем случае только оно спасет.

anonymous ()

Существуют способы оптимизации этого кода?

Забей, архитектура moodle это просто кусок говна. Но если очень хочется, то перепиши этот код с применением конечного автомата.

no-such-file ★★★★★ ()
Последнее исправление: no-such-file (всего исправлений: 2)
Ответ на: комментарий от anonymous

А конкретно правило про 1 точку выхода из функции

У ТС в функции 1 точка выхода. Ты бы сам сначала какие-нибудь мурзилки почитал.

no-such-file ★★★★★ ()
Ответ на: комментарий от no-such-file

Опа, знаток по мурзилкам подъехал! грепни return в коде из ОП и посчитай кол-во точек выхода.

Вот как это делается.

protected function validateUsernamePassword($username, $password) {
    return validateUsername($username) && auth_billing::check_user($username, $password);
}
public function completeUserLogin($username, $password) {
    return $this->validateUsernamePassword($username, $password) && completeUserLogin($user);
}

anonymous ()

Количество if возможно уменьшить через throw и/или assert

Нагромождение многоуровневых имен вида get_complete_user_data, validate_email тоже имеет смысл уменьшить. Можно упростить через fluent-интерфейсы, ленивую загрузку, пространства имен.

Игноры ооп не дают особых плюсов.

Повышайте ценность (качество) кода - есть потенциал.

use https://github.com/squizlabs/PHP_CodeSniffer Имена классов принято писать с заглавной.

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

return в коде из ОП и посчитай кол-во точек выхода

return это не точка выхода из функции, дурака ты кусок. Точка выхода (или иначе, точка перехода) - это куда передаётся управление. И такая точка у ТС одна - это точка вызова функции.

Вот как это делается

Так это делается только наивными школьниками. По-хорошему, вложенные условия нужно преобразовать в стейт-машину (хотя бы на switch, но лучше на объектах). Потому что завтра тебе приспичит проверять ещё что-то и в итоге получится такая говнолапша от которой Дийкстра вспотеет в могиле. И ты мне ещё тут рассказываешь про «1 точку выхода»? Или читай «войну и мир» или что там сейчас на лето задают.

no-such-file ★★★★★ ()
Ответ на: комментарий от no-such-file

Я знал, что оно точно что-то нарушает и так люди не пишут, но это надо для себя, так что кому какая разница.

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

но это надо для себя, так что кому какая разница

Ну в принципе, да. Там весь moodle одно большое нарушение всего. Хуже не будет, наверное.

no-such-file ★★★★★ ()
Ответ на: комментарий от crutch_master

Это не эквивалентное изменение. В первом случае при с1-с2 равных false-false функция op() не вызывается, а во втором случае — вызывается.

i-rinat ★★★★★ ()

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

Изучив исходный код Moodle и почитав документацию, выяснил следующий момент: Если функция user_login вернула истину, а пользователь не был найден, создаётся новый локальный пользователь и вызывается функция get_userinfo, чтобы заполнить поля профиля.

В итоге, код выглядит следующим образом:

/**
 * Истинно, если пользователь идентифицирован.
 *
 * @param   string  $username   Логин пользователя
 * @param   string  $password   Пароль пользователя
 * @return  boolean             Результат проверки
 */
public function user_login($username, $password) {
    return auth_billing::check_user($username, $password);
}

/**
 * Получение информации о пользователе из внешней системы.
 *
 * @param   string  $username   Логин пользователя
 * @return  array               Пользовательская информация
 */
public function get_userinfo($username) {
    return auth_billing::create_profile($username);
}

Спасибо всем откликнувшимся за решения и советы.

Adeptus-Mechanicus ()
Ответ на: комментарий от anonymous

use https://github.com/squizlabs/PHP_CodeSniffer Имена классов принято писать с заглавной.

Использую, спасибо.

Moodle используют свои правила оформления кода и требуют этого от всех сторонних разработчиков, иначе плагин не проходит проверку и тестирование.

Подробности: https://docs.moodle.org/dev/Coding_style

Adeptus-Mechanicus ()
Ответ на: комментарий от no-such-file

return это не точка выхода из функции, дурака ты кусок. Точка выхода (или иначе, точка перехода) - это куда передаётся управление.

интересное определение, сам придумал? что по твоему делает return? «точка перехода»... клоун. сразу видно вебмакаку, ни разу не читавшего макконела

По-хорошему, вложенные условия нужно преобразовать в стейт-машину

по-хорошему, вложенность должна быть 1-уровневая

Или читай «войну и мир» или что там сейчас на лето задают.

Не знаю, что там у вас в дурке задают, но художественный кал читай сам, твою голову уже сильнее не засрать.

anonymous ()
Ответ на: комментарий от no-such-file

return это не точка выхода из функции
Точка выхода (или иначе, точка перехода)
такая точка у ТС одна - это точка вызова функции

facepalm

помечу тебя что бы вдруг случайно за умного не принять.

Noob_Linux ★★★ ()
Ответ на: комментарий от anonymous

что по твоему делает return?

По-моему return формально делает goto в конец функции. В ту самую «одну точку выхода».

по-хорошему, вложенность должна быть 1-уровневая

Стейт-машина и обеспечивает вложенность 1 уровня при этом не заставляя писать лапшу из кучи && ||.

художественный кал читай сам

Ути-пути, ловите нигилиста.

ни разу не читавшего макконела

Что, петушара, решил на авторитетов съехать? Типичные замашки школоло «а в учебнике написано». И что, что там написано. Ты ж нихрена не понял, что там написано и в каком контексте. Прочитал «1 точка выхода» и лепишь везде совершенно не к месту. А про что это и к чему сказано - вообще понятия не имеешь. Мартышка и очки в натуре, а да, ты ж художественную литературу не читаешь.

no-such-file ★★★★★ ()
Ответ на: комментарий от no-such-file

По-моему return формально делает goto в конец функции. В ту самую «одну точку выхода».

Ты ошибаешься. Это лечится С/С++ внутривенно, заедая дизассемблерными листингами.

Что, петушара, решил на авторитетов съехать?

Вообще-то мое первое сообщение в треде содержало рекомендацию прочитать «Совершенный код» Макконела, на книгу котрого я и упираюсь при написании кода. Так что никуда я не съезжаю.

Ты ж нихрена не понял, что там написано и в каком контексте.

Нет, это ты не понял.

Прочитал «1 точка выхода» и лепишь везде совершенно не к месту.

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

ты ж художественную литературу не читаешь

Не читаю и никому не советую, т.к. пользы от неё нет.

anonymous ()
Ответ на: комментарий от no-such-file

По-моему return формально делает goto в конец функции. В ту самую «одну точку выхода».

Восстановление стек фрейма действительно происходит в одном месте, но понятие выхода из функции имеет совершенно другой смысл.

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

имею многолетний опыт реверс инжиниринга

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

Это лечится С/С++ внутривенно

Ох ты ж ёперный театр, да тут у нас шпециалист в треде. Я эти твои Си с ассемблерами вертел на одном месте ещё лет 20 назад. Можешь скукожить свою пипку обратно, у меня всё равно длиннее.

Восстановление стек фрейма действительно происходит в одном месте

Поциент начал что-то подозревать...

но понятие выхода из функции имеет совершенно другой смысл

Ну ты уже как-то определись, goto end формально не является выходом из функции и в твоей трактовке «1 точка выхода» соблюдается. При этом goto end полностью эквивалентно return. Я понимаю что у тебя шаблон трещит, но иногда стоит прислушаться к тому, что тебе говорят, а не хером трясти.

Эту тему с «1 точкой выхода» стали поднимать в бородатые времена потому что вместо

begin: ...
...
res = sub();
if (!res) goto begin;
...
sub:
...
if (!test) return false;
...
return true;
тогда было принято фигачить
begin: ...
...
sub();
...
sub:
...
if (!test) goto begin;
...
return;
И т.п. лапшекод, когда sub мог выйти куда угодно и нужно было следить за тем что куда переходит и блок-схемы рисовать. Отдельной фишкой было заходить в подпрограмму по разным адресам, пропуская часть кода. И тогда последователи «1 точки выхода» стали пилить процедурные/структурные языки в которых _невозможно_ получить не 1 точку выхода, потому что goto за пределы процедуры/функции запрещён. Идея «1 точки выхода» в том, чтобы иметь «процедуру», как независимый от остального кода кирпичик с 1 входом куда подаются данные и 1 выходом, откуда данные забираются. И блджад к количеству return это вообще никаким боком не относится, потому что даже 100500 return не нарушают эту абстракцию. Наоборот return это средство соблюдения данной абстракции без необходимости явного перехода в конец функции. Это всё равно что знак «земля» на электросхеме, который используют чтобы не рисовать 100500 соединений. Так что утверждать что return-ы это разные точки выхода, всё равно что утверждать что каждый значок «земля» это разные, не соединяющиеся «земли».

PS: Ска, ну совсем уже на пальцах объясняю, специально для семиклассников «реверсинжиниров на C++». Надеюсь дошло?

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