Рефакторинг / верстка функционала Scala - PullRequest
12 голосов
/ 21 ноября 2010

Этот лайнер ...

 Console.println(io.Source.fromFile("names.txt").getLines.mkString.split(",").map{x:String => x.slice(1, x.length -1)}.sortBy { x => x}.zipWithIndex.map{t =>{ (t._2 +1)*(t._1.map{_.toChar - "A"(0).toChar + 1}.sum)}}.sum);

... это мое решение Project Euler, проблема 22 * ​​1005 *. Кажется, это работает и написано в (моей попытке) функциональном стиле.

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

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

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

Для примера, который я привел, как лучше написать и представить?

(Как и все вопросы о стиле, этот вопрос субъективен. Впрочем, нет причин для спора!)

Ответы [ 4 ]

19 голосов
/ 21 ноября 2010

Первая простая попытка привести его в порядок - просто удалить начальный Console. завершающий ; и явный тип :String - все это не нужно - добавить отступы и импортировать io.Source:

import io.Source
println(
  Source.fromFile("names.txt").getLines.mkString.split(",").map{
    x => x.slice(1, x.length -1)
  }.sortBy {x => x}.zipWithIndex.map{
    t =>{ (t._2 +1)*(t._1.map{_.toChar - "A"(0).toChar + 1}.sum)}
  }.sum
)

Следующий шаг - это немного его очистить, использовать сопоставление с шаблоном при отображении списка кортежей и identity вместо x=>x. toChar также не требуется для символов, и одинарные кавычки могут использоваться для представления символьных литералов.

import io.Source
println(
  Source.fromFile("names.txt").getLines.mkString.split(",").map {
    x => x.slice(1, x.length -1)
  }.sortBy(identity).zipWithIndex.map {
    case (v, idx) =>{ (idx+1)*(v.map{_ - 'A' + 1}.sum)}
  }.sum
)

Еще несколько изменений также помогут сделать код более понятным:

import io.Source
println(
  Source.fromFile("names.txt").getLines.mkString.split(",")
  .map { _.stripPrefix("\"").stripSuffix("\"") }
  .sortBy(identity)
  .map { _.map{_ - 'A' + 1}.sum }
  .zipWithIndex
  .map { case (v, idx) => (idx+1) * v }
  .sum
)

Следующий шаг, чтобы сделать его более «функциональным», состоит в том, чтобы разбить его на «функции» ( подлый, да? ). В идеале каждая функция должна иметь имя, четко выражающее ее назначение, и оно должно быть коротким (стремитесь к тому, чтобы это было одно выражение, поэтому фигурные скобки не требуются):

import io.Source

def unquote(s:String) = s.stripPrefix("\"").stripSuffix("\"")

def wordsFrom(fname:String) =
  Source.fromFile(fname).getLines.mkString.split(",").map(unquote)

def letterPos(c:Char) = c - 'A' + 1

println(
  wordsFrom("names.txt")
  .sortBy(identity)
  .map { _.map(letterPos).sum }
  .zipWithIndex
  .map { case (v, idx) => (idx+1) * v }
  .sum
)

wordsFrom является очевидным 1-строчным, но я разбил его для упрощения форматирования в stackOverflow

4 голосов
/ 22 ноября 2010

Говоря строго о том, как форматировать этот код, без внесения каких-либо структурных изменений, я бы сделал это так:

Console println (
  (
    io.Source
    fromFile "names.txt"
    getLines ()
    mkString ""
    split ","
    map (x => x.slice(1, x.length - 1))
    sortBy (x => x)
    zipWithIndex
  )
  map (t => (t._2 + 1) * (t._1 map (_.toChar - "A"(0).toChar + 1) sum) )
  sum
)

Или, возможно, чтобы обойти методы без параметров, я бы сделал это:

Console println (
  io.Source
  .fromFile("names.txt")
  .getLines
  .mkString
  .split(",")
  .map(x => x.slice(1, x.length - 1))
  .sortBy(x => x)
  .zipWithIndex
  .map(t => (t._2 + 1) * (t._1 map (_.toChar - "A"(0).toChar + 1) sum) )
  .sum
)

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

Теперь некоторые вещи, которые я бы сделал по-другому:

println ( // instead of Console println
  Source // put import elsewhere
  .fromFile("names.txt")
  .mkString // Unless you want to get rid of /n, which is unnecessary here
  .split(",")
  .map(x => x.slice(1, x.length - 1))
  .sorted // instead of sortBy
  .zipWithIndex
  .map { // use { only for multiple statements and, as in this case, pattern matching
    case (c, index) => (index + 1) * (c map (_ - 'A' + 1) sum) // chars are chars
  }
  .sum
)

Я бы тоже не делал суммирование и умножение на одном шаге, поэтому:

  .sorted
  .map(_ map (_ - 'A' + 1) sum)
  .zipWithIndex
  .map { case (av, index) => av * (index + 1) }
  .sum

Наконец, мне не очень нравится изменение размера строки, поэтому я мог бы вместо этого прибегнуть к регулярному выражению. Добавьте немного рефакторинга, и это то, что я, вероятно, напишу:

  import scala.io.Source
  def names = Source fromFile "names.txt" mkString

  def wordExtractor = """"(.*?)"""".r
  def words = for {
    m <- wordExtractor findAllIn names matchData
  } yield m group 1

  def alphabeticValue(s: String) = s map (_ - 'A' + 1) sum
  def wordsAV = words.toList.sorted map alphabeticValue

  def multByIndex(t: (Int, Int)) = t match {
    case (av, index) => av * (index + 1)
  }
  def wordsAVByIndex = wordsAV.zipWithIndex map multByIndex

  println(wordsAVByIndex.sum)

EDIT

Следующим шагом будет переименование рефакторинга - выбор имен, которые лучше передают то, что делает каждая часть кода. Кен предложил в комментариях более подходящие имена, и я бы присвоил их еще одному варианту (он также показывает, насколько лучше наименование улучшает читабельность).

import scala.io.Source
def rawData = Source fromFile "names.txt" mkString

// I'd rather write "match" than "m" in the next snippet, but
// the former is a keyword in Scala, so "m" has become more
// common in my code than "i". Also, make the return type of
// getWordsOf clear, because iterators can be tricky.
// Returning a list, however, makes a much less cleaner
// definition.

def wordExtractor = """"(.*?)"""".r
def getWordsOf(input: String): Iterator[String] = for {
  m <- wordExtractor findAllIn input matchData
} yield m group 1
def wordList = getWordsOf(rawData).toList

// I stole letterPosition from Kevin's solution. There, I said it. :-)

def letterPosition(c: Char) = c.toUpper - 'A' + 1 // .toUpper isn't necessary
def alphabeticValueOfWord(word: String) = word map letterPosition sum
def alphabeticValues = wordList.sorted map alphabeticValueOfWord

// I don't like multAVByIndex, but I haven't decided on a better
// option yet. I'm not very fond of declaring methods that return
// functions either, but I find that better than explicitly handling
// tuples (oh, for the day tuples/arguments are unified!).

def multAVByIndex = (alphabeticValue: Int, index: Int) => 
  alphabeticValue * (index + 1)
def scores = alphabeticValues.zipWithIndex map multAVByIndex.tupled

println(scores sum)
4 голосов
/ 21 ноября 2010

Вот то, что я думаю, может быть лучшим способом изложить это:

Console.println(
    io.Source.fromFile("names.txt")
    .getLines.mkString.split(",")
    .map{x:String => x.slice(1, x.length -1)}
    .sortBy { x => x}
    .zipWithIndex
    .map{t =>{ (t._2 +1)*(t._1.map{_.toChar - "A"(0).toChar + 1}.sum)}}
    .sum); 

Мне кажется, что в глубине моего мозга есть какой-то алгоритм, который принимает решения о компромиссах компоновки кода между горизонтальным и вертикальным пространством, но у меня, кажется, нет прямого доступа, чтобы позволить мне сформулировать это. :)

Что касается введения имен, а не использования лямбд, я думаю, что обычно я делаю, если у меня возникает соблазн написать короткий комментарий, описывающий назначение кода, но хорошее имя идентификатора может сделать то же самое, тогда я могу одноразовая лямбда в именованную функцию, чтобы получить удобство чтения имени идентификатора. Строка с вызовами toChar - единственная выше, которая выглядит мне кандидатом. (Для ясности, я бы учел (частично) лямбду внутри map, но сам вызов map.) В качестве альтернативы, введение вертикального пробела дает вам возможность повесить //comment, который является альтернатива введению имени идентификатора.

(Отказ от ответственности: я не пишу Scala, поэтому, если что-то, что я говорю, противоречит соглашениям о стиле, то игнорируйте меня :), но я думаю, что многие из этих советов в основном не зависят от языка.)

1 голос
/ 22 ноября 2010

В дополнение к решению Кевина,

ключ заключается в четком и аккуратном разделении функций с учетом возможности повторного использования и читаемости.

Чтобы сделать код еще короче и чище, онкажется, что можно использовать выражение for.


def inputString(inputFile: String) = io.Source.fromFile(inputFile).getLines.mkString

def inputWords(input: String) = input.split("[,\"]").filter("" != )

Console.println {
    (for { (s, idx) <- inputWords(inputString("names.txt")).sorted.zipWithIndex }
        yield s.map(_ - 'A' + 1).sum * (idx + 1)).sum
}

Часть s.map (_- 'A' + 1) может быть дополнительно помещена в функцию, скажем, wordSum, если вы хотите, чтобы она была большеразличны.

...