понедельник, 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? Вот, собственно, и все доказательство.

Отправить комментарий