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