понедельник, 24 января 2011 г.

[prog.flame] Попробую отрефакторить чужой Scala-код

На LOR-е прошло обсуждение присуждения гранта команде разработчиков Scala (об этом я уже писал). Свою лопату дерьма на вентилятор в этом обсуждении я мастерски вбросил и запасся попкорном. В процессе споров один из защитников Scala поделился с общественностью примером своего кода. Который, по мнению автора, демонстрирует преимущества лаконичности Scala вообще и повышения надежности за счет Option[T] и паттерн-матчинга.

Лично же мне кажется, что это обычный говнокод. Который я в данной заметке попробую отрефакторить. Чтобы получилось так, как привык писать я сам. И как я требую поступать от своих подчиненных. Т.е. не факт, что получится что-то принципиально иное. Но, будет оформлено более привычно для меня :)

Кроме того, в процессе упомянутого LOR-овского флейма мне стало более очевидно, почему на платформе JVM языку Scala не суждено стать мейнстримом. Надеюсь, что в конце заметки я смогу это продемонстрировать.

Букв будет много, поэтому продолжение под катом.

Итак, исходный код:

@rest.Method(httpMethod = Array(POST))
def login(request: rest.Request,
          @rest.Param(name = "login") login: String,
          @rest.Param(name = "password") password: String) = {
        debug("accessing login... " + login)

        if( login == null || password == null ) respond(UNAUTHORIZED, "You should provide login and password")
        else AccountsStorage.find(login) match {
                case None => respond(UNAUTHORIZED, "User not found")
                case Some(user) if user.inactive => respond(UNAUTHORIZED, "Account is inactive")
                case Some(user) if user.authScheme == "PETRIVKA" =>
                        if( user.passwordMatches(password) ) request.session("user") = user
                        else respond(UNAUTHORIZED, "Authentication failed")
                case Some(user) => AccessStorage.access.auth_configs.find(_.key == user.authScheme) match {
                        case None => respond(FORBIDDEN, "Unknown authentication scheme: " + user.authScheme)
                        case Some(scheme) =>
                                log.debug("authenticating with " + scheme.command)
                                val exec = Runtime.getRuntime.exec(scheme.command.replace("{login}", login).replace("{password}", password))
                                if( exec.waitFor == 0 ) request.session("user") = user
                                else respond(UNAUTHORIZED, "Authentication within " + scheme.key + " failed")
                }
                case _ => respond(UNAUTHORIZED, "Authentication failed")
        }
}

Что он делает по своей сути? Сначала проверяет, что аргументы login и password отличны от null и, в зависимости от результатов проверки, предпринимает одно из двух действий – либо выдает ошибку, либо пытается осуществить аутентификацию пользователя. Значит именно так и следует написать тело метода login:

@rest.Method(httpMethod = Array(POST))
def login(request: rest.Request,
    @rest.Param(name = "login") login: String,
    @rest.Param(name = "password") password: String) = {
  debug("accessing login... " + login)

  if( login == null || password == null )
    respond(UNAUTHORIZED, "You should provide login and password")
  else
    doLogin(request, login, password)
}

Теперь посмотрим, что же нужно сделать для аутентификации? Сперва найти описание пользователя. Если оно найдено, то нужно предпринимать дальнейшие действия, в противном случае нужно сообщать, что пользователь неизвестен. Т.е. деление очень простое “или-или”.

Однако, в исходном коде все это разделено на несколько вариантов – пользователь неизвестен; пользователь известен, но деактивирован; пользователь известен, но у него хитрая схема аутентификации; и просто пользователь известен. Все это свалено в одну большую кучу, что мне совершенно не нравится. Поэтому я бы расписал все проще:

def doLogin(request: rest.Request, login: String, password: String) = {
  AccountsStorage.find(login) match {
    case None => handleUnknownUserLogin
    case Some(user) => handleKnownUserLogin(request, login, password, user)
  }
}

def handleUnknownUserLogin() = respond(UNAUTHORIZED, "User not found")

Можно было бы не делать отдельный метод-однострочник handleUnknownUserLogin, но я предпочел его написать. Поскольку с ним реализация doLogin оказывается более высокоуровневой – читатель не видит низкоуровневых подробностей каждого из вариантов. Видно только, что это разные варианты и обрабатываются различными методами. Ну а то, что сейчас handleUnknownUserLogin является однострочником, так это частная деталь текущей реализации.

Теперь нужно перейти к handleKnownUserLogin. В принципе, теперь это уже не сложно:

def handleKnownUserLogin(
    request: rest.Request,
    login: String,
    password: String,
    user: UserInfo) = {
  if user.inactive
    handleInactiveUserLogin
  else
    if user.authScheme == "PETRIVKA"
      handlePetrivkaAuthSchemeLogin(request, login, password, user)
    else
      handleUsualAuthSchemeLogin(request, login, password, user)
}

def handleInactiveUserLogin =
  respond(UNAUTHORIZED, "Account is inactive")

def handlePetrivkaAuthSchemeLogin(
    request: rest.Request,
    login: String,
    password: String,
    user: UserInfo) =
  if( user.passwordMatches(password) )
    setupSuccessfulAuthResult(request, user)
  else
    respond(UNAUTHORIZED, "Authentication failed")

def handleUsualAuthSchemeLogin(
    request: rest.Request,
    login: String,
    password: String,
    user: UserInfo) = {
  AccessStorage.access.auth_configs.find(_.key == user.authScheme) match {
    case None => respond(FORBIDDEN, "Unknown authentication scheme: " + user.authScheme)
    case Some(scheme) =>
      log.debug("authenticating with " + scheme.command)
      val exec = Runtime.getRuntime.exec(
          scheme.command.replace("{login}", login).replace("{password}",
          password))
      if( exec.waitFor == 0 )
        setupSuccessfulAuthResult(request, user)
      else
        respond(UNAUTHORIZED, "Authentication within " + scheme.key + " failed")
  }
}

def setupSuccessfulAuthResult(request: rest.Request, user: UserInfo) =
  request.session("user") = user

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

Хотя я бы пошел в рефакторинге еще дальше. Например, из моего кода видно, что изрядный объем занимают декларации методов handle* – за счет перечня параметров request, login, password и user. Поэтому я бы для обработки логина выделил бы специальный класс, в котором данные параметры были бы атрибутами. И в этот класс бы переместил методы handle.

В результате получилось бы так:

@rest.Method(httpMethod = Array(POST))
def login(request: rest.Request,
          @rest.Param(name = "login") login: String,
          @rest.Param(name = "password") password: String) = {
        debug("accessing login... " + login)

        if( login == null || password == null ) respond(UNAUTHORIZED, "You should provide login and password")
        else AccountsStorage.find(login) match {
                case None => respond(UNAUTHORIZED, "User not found")
                case Some(user) if user.inactive => respond(UNAUTHORIZED, "Account is inactive")
                case Some(user) if user.authScheme == "PETRIVKA" =>
                        if( user.passwordMatches(password) ) request.session("user") = user
                        else respond(UNAUTHORIZED, "Authentication failed")
                case Some(user) => AccessStorage.access.auth_configs.find(_.key == user.authScheme) match {
                        case None => respond(FORBIDDEN, "Unknown authentication scheme: " + user.authScheme)
                        case Some(scheme) =>
                                log.debug("authenticating with " + scheme.command)
                                val exec = Runtime.getRuntime.exec(scheme.command.replace("{login}", login).replace("{password}", password))
                                if( exec.waitFor == 0 ) request.session("user") = user
                                else respond(UNAUTHORIZED, "Authentication within " + scheme.key + " failed")
                }
                case _ => respond(UNAUTHORIZED, "Authentication failed")
        }
}
@rest.Method(httpMethod = Array(POST))
def login(request: rest.Request,
    @rest.Param(name = "login") login: String,
    @rest.Param(name = "password") password: String) = {
  debug("accessing login... " + login)

  if( login == null || password == null )
    respond(UNAUTHORIZED, "You should provide login and password")
  else
    doLogin(request, login, password)
}

def doLogin(request: rest.Request, login: String, password: String) = {
  AccountsStorage.find(login) match {
    case None => handleUnknownUserLogin
    case Some(user) => KnownUserLoginHandler(request, login, password, user).handle
  }
}

def handleUnknownUserLogin() = respond(UNAUTHORIZED, "User not found")

class KnownUserLoginHandler(
    request: rest.Request,
    login: String,
    password: String,
    user: UserInfo) {
  def handle() =
    if user.inactive
      handleInactiveUserLogin
    else
      if user.authScheme == "PETRIVKA"
        handlePetrivkaAuthSchemeLogin
      else
        handleUsualAuthSchemeLogin

  def handleInactiveUserLogin =
    respond(UNAUTHORIZED, "Account is inactive")

  def handlePetrivkaAuthSchemeLogin() =
    if( user.passwordMatches(password) )
      setupSuccessfulAuthResult
    else
      respond(UNAUTHORIZED, "Authentication failed")

  def handleUsualAuthSchemeLogin() =
    AccessStorage.access.auth_configs.find(_.key == user.authScheme) match {
      case None => respond(FORBIDDEN, "Unknown authentication scheme: " + user.authScheme)
      case Some(scheme) =>
        log.debug("authenticating with " + scheme.command)
        val exec = Runtime.getRuntime.exec(
            scheme.command.replace("{login}", login).replace("{password}",
            password))
        if( exec.waitFor == 0 )
          setupSuccessfulAuthResult
        else
          respond(UNAUTHORIZED, "Authentication within " + scheme.key + " failed")
    }
  }

  def setupSuccessfulAuthResult() =
    request.session("user") = user
}

А теперь немного о том, почему Scala не станет мейнстримом вместо Java. Достаточно попытаться представить, как мог бы выглядеть это же решение на Java. И сравнить (слева Java, справа Scala):

@rest.Method(httpMethod = Array(POST))
public void login(rest.Request request,
      @rest.Param(name = "login") String login,
      @rest.Param(name = "password") String password)
{
   debug("accessing login... " + login);

   if( login == null || password == null )
      respond(UNAUTHORIZED, "You should provide login and password");
   else
      doLogin(request, login, password);
}

private void doLogin(rest.Request request, String login, String password)
{
   UserInfo user = AccountsStorage.find(login);
   if( user == null )
      handleUnknownUserLogin();
   else
      (new KnownUserLoginHandler(request, login, password, user)).handle();
}

private class KnownUserLoginHandler
{
   rest.Request request;
   String login;
   String password;
   UserInfo user;

   KnownUserLoginHandler(
         rest.Request request,
         String login,
         String password,
         UserInfo user)
      {
         this.request = request;
         this.login = login;
         this.password = password;
         this.user = user;
      }

   public void handle()
   {
      if( user.inactive )
         handleInactiveUserLogin();
      else
         if( user.authScheme.equals( "PETRIVKA" ) )
            handlePetrivkaAuthSchemeLogin();
         else
            handleUsualAuthSchemeLogin();
   }

   private void handleInactiveUserLogin()
   {
      respond(UNAUTHORIZED, "Account is inactive");
   }

   private void handlePetrivkaAuthSchemeLogin()
   {
      if( user.passwordMatches(password) )
         setupSuccessfulAuthResult();
      else
         respond(UNAUTHORIZED, "Authentication failed");
   }

   private void handleUsualAuthSchemeLogin()
   {
      AuthScheme scheme = findAuthScheme();
      if( scheme == null )
         respond(FORBIDDEN, "Unknown authentication scheme: " + user.authScheme);
      else
      {
         log.debug("authenticating with " + scheme.command)
         SomeExecReturnValue exec = Runtime.getRuntime.exec(
               scheme.command.replace("{login}", login).replace("{password}",
               password));
         if( exec.waitFor == 0 )
            setupSuccessfulAuthResult();
         else
            respond(UNAUTHORIZED, "Authentication within " + scheme.key + " failed");
      }
   }

   private void setupSuccessfulAuthResult()
   {
      request.session("user") = user;
   }
   
   private AuthScheme findAuthScheme()
   {
      // bla-bla-bla
   }
}
@rest.Method(httpMethod = Array(POST))
def login(request: rest.Request,
    @rest.Param(name = "login") login: String,
    @rest.Param(name = "password") password: String) = {
  debug("accessing login... " + login)

  if( login == null || password == null )
    respond(UNAUTHORIZED, "You should provide login and password")
  else
    doLogin(request, login, password)
}

def doLogin(request: rest.Request, login: String, password: String) = {
  AccountsStorage.find(login) match {
    case None => handleUnknownUserLogin
    case Some(user) => KnownUserLoginHandler(request, login, password, user).handle
  }
}

def handleUnknownUserLogin() = respond(UNAUTHORIZED, "User not found")

class KnownUserLoginHandler(
    request: rest.Request,
    login: String,
    password: String,
    user: UserInfo) {
  def handle() =
    if user.inactive
      handleInactiveUserLogin
    else
      if user.authScheme == "PETRIVKA"
        handlePetrivkaAuthSchemeLogin
      else
        handleUsualAuthSchemeLogin

  def handleInactiveUserLogin =
    respond(UNAUTHORIZED, "Account is inactive")

  def handlePetrivkaAuthSchemeLogin() =
    if( user.passwordMatches(password) )
      setupSuccessfulAuthResult
    else
      respond(UNAUTHORIZED, "Authentication failed")

  def handleUsualAuthSchemeLogin() =
    AccessStorage.access.auth_configs.find(_.key == user.authScheme) match {
      case None => respond(FORBIDDEN, "Unknown authentication scheme: " + user.authScheme)
      case Some(scheme) =>
        log.debug("authenticating with " + scheme.command)
        val exec = Runtime.getRuntime.exec(
            scheme.command.replace("{login}", login).replace("{password}",
            password))
        if( exec.waitFor == 0 )
          setupSuccessfulAuthResult
        else
          respond(UNAUTHORIZED, "Authentication within " + scheme.key + " failed")
    }
  }

  def setupSuccessfulAuthResult() =
    request.session("user") = user
}

Не смотря на более длинный код, на Java получились, в сущности те же яйца, только в профиль. Решение на Scala компактнее только за счет небольших синтаксических рюшечек (в первую очередь, за счет возможности автоматической генерации конструктора класса и необязательности скобок вокруг однострочных методов). Но, учитывая продвинутость современных IDE для Java, я не думаю, что разработчик будет писать руками на Java намного больше, чем руками на Scala.

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

13 комментариев:

Rustam комментирует...

А Scala вложенные функции понимает?
Если да, то class KnownUserLoginHandler не нужен, делаем все твои вспомогательные функции вложенными и без лишних параметров, и читабельность хорошая и краткость не пострадает.

Евгений Охотников комментирует...

@Rustam:

вот чего не помню, того уже не помню. С 2007-го ничего длиннее 5 строк на Scala не писал.

Rustam комментирует...

Должна поддерживать иначе какая это функциональщина :)

Евгений Охотников комментирует...

@Rustam:

наверняка должна! :)

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

Ну и, наверное, привычка к C++у сказалась :)

Rustam комментирует...

Да тесты с вложенными функциями посложнее делать.

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

Евгений Охотников комментирует...

@Rustam:

Обилие хардкодинга -- это отдельный вопрос. Меня больше беспокоили логические условия. Поэтому по-моему, как минимум, нужно было в отдельную функцию выносить обработку Some(user) (и уже в этой функции обрабатывать ситуации inactive и authScheme) и Some(scheme) в еще одну. Т.е. как минимум нужно было бы три функции, а не одна.

А когда в одном match-е обрабатывается и None, и Some(user), и Some(user) if... то здесь уже смысла в матчинге не видно. Да и компилятор не в состоянии проверить, все ли варинты обработаны.

CheatEx комментирует...

Здравствуйте. Тема поста хорошо срезонировала с текущей деятельность, так что не смог удержаться и написал развёрнутый ответ: http://programmers-path.blogspot.com/2011/01/blog-post.html

Ну и раз уж дошли руки тут отписаться, хочется поблагодарить за хороший блог. Уже около полугода с удовольствием читаю.

Евгений Охотников комментирует...

@CheatEx:

>написал развёрнутый ответ

Ответил вам там.

>Ну и раз уж дошли руки тут отписаться, хочется поблагодарить за хороший блог. Уже около полугода с удовольствием читаю.

Спасибо на добром слове. Надеюсь, вы останетесь моим читателем не смотря на мою резкость.

Quaker комментирует...

Прочитал http://programmers-path.blogspot.com/2011/01/blog-post.html, но все равно имеется ощущение, что функциональщики занимаются плясками с бубном вокруг одного и того же, только внешний вид другой.

Евгений Охотников комментирует...

@Quaker:

Чуть-чуть на эту тему высказался здесь (см."Во-вторых...")

Rustam комментирует...

@Quaker

А я читаю тут и кусочками флейм на лоре и все больше офигеваю как это Scala столько времени прикидывалась функциональным языком. Даже по сравнению с почти императивными OCaml/F# :)

anonymous комментирует...

ыыы, читаю ту самую тему на ЛОР. Как вы эпично сливаете. Получаю массу удовольствия.

Евгений Охотников комментирует...

@anonymous:

Надеюсь, попкорна вам хватит.