2010-11-21 10 views
12

Ésta forro ...Refactoring/diseño de Scala funcional

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); 

... es mi solución a Project Euler problem 22. Parece que funciona, y está escrito en (intento de) estilo funcional.

Este ejemplo es un poco extremo, pero mi pregunta es un poco más general: ¿cómo prefiere escribir/formatear/comentar el código de estilo funcional? El enfoque funcional parece alentar una secuencia de llamadas a métodos, que, en mi opinión, puede ser ilegible, y tampoco deja nada obvio para poner comentarios.

Además, cuando escribo código de procedimiento, encuentro que escribo métodos pequeños, cada uno con un propósito y con nombres significativos. Cuando escribo código funcional, parece que estoy desarrollando un hábito que produce líneas un poco como la anterior, donde (para mí) el significado es difícil de descifrar, y también los cálculos individuales son difíciles de reutilizar en otro lugar. Muchos de los ejemplos de código funcional que veo en la web son igualmente escuetos (para mí) oscuros.

¿Qué debería estar haciendo? ¿Escribir pequeñas funciones para cada parte del cálculo con nombres significativos en el contexto actual? (incluso si son poco más que un contenedor para el mapa, digamos?)

Por el ejemplo que he dado, ¿cuáles son las mejores formas de escribirlo y presentarlo?

(Al igual que todas las cuestiones de estilo, éste es subjetiva No hay ninguna razón por la que debe recibir argumentativo, sin embargo.!)

+0

No tengo suficiente experiencia con Scala para darle buenos consejos, pero podría ser útil si dividiera cada 'mapa' en una línea diferente? Me interesa saber qué tienen que decir los Scala-it reales sobre esto. –

+0

Una regla muy importante e independiente del lenguaje/paradigma no es hacer líneas de más de 80 caracteres (u otro valor razonable). 190 caracteres es una locura (y si tiene código de golf, al menos baje a 130 o ¡nunca le ganará a Perl!;)). – delnan

+1

De acuerdo, es tan largo por razones pedagógicas. Pero mi pregunta es, en parte, sobre la mejor forma de dividirla en trozos más pequeños, aparte de elegir los descansos solo para que las líneas sean <80 caracteres. No estoy jugando al golf, pero estoy tratando de mejorar mi programación funcional ... –

Respuesta

19

Un trivial primer intento de poner en orden operativa es simplemente quitar el líder de la Console. detrás ; y la explícita tipo :String - todos los cuales son innecesarias - añadir un poco de sangrado e importación 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 
) 

el siguiente paso es limpiarlo un poco, use la coincidencia de patrones cuando mapee sobre la lista de tuplas a nd identity en lugar de x=>x. toChar también es innecesario para los caracteres, y las comillas simples se pueden usar para representar los literales de caracteres.

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 
) 

Unos cuantos cambios también ayudan a que la intención del código mucho más claro:

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 
) 

El siguiente paso, para que sea más "funcional", es para dividirla en "funciones" (furtivo, ¿eh?). Idealmente, cada función tiene un nombre que expresa claramente su propósito, y que será corto (objetivo para que sea una sola expresión, por lo que no son necesarios apoyos):

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 es una obvia 1-liner, pero me separé de él para facilitar el formato de Stackoverflow

+0

Gracias. Todavía soy un neófito en Scala, así que esto es realmente útil. La refactorización es más o menos lo que habría hecho. De alguna manera he ganado la opinión (probablemente errónea) de que este enfoque de función de expresión única no era la forma de hacer las cosas. –

+0

Tuve que volver a bajar un par de muescas después de ver el código en pantalla completa, no se veía mucho mejor que el original después de descomponer * todo * en funciones :) –

+2

+1 Esto es bastante parecido a lo que consideraría una respuesta perfecta a esta pregunta. Minor nitpick: ¿no sería 'sorted' preferible a' sortBy (identity) '? –

4

Esto es lo que creo que podría ser una mejor manera de ponerlo hacia fuera:

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); 

Siento que en lo más profundo de mi cerebro hay algún algoritmo que toma decisiones sobre el reparto de códigos entre el espacio horizontal y el vertical, pero no parece que tenga acceso directo para poder articularlo. :)

En cuanto a la introducción de nombres en lugar de usar lambdas, creo que lo que suelo hacer es, si tengo la tentación de poner un breve comentario que describa la intención del código, pero un buen identificador puede hacer lo mismo, entonces puede factorizar una lambda de una sola vez en una función nombrada para obtener el beneficio de legibilidad del nombre del identificador. La línea con las llamadas toChar es la única de arriba que parece un candidato para mí. (Para ser claros, debería factorizar (parte de) el lambda dentro del map, pero el map se llama a sí mismo). Alternativamente, la introducción del espacio en blanco vertical le da un lugar para colgar un //comment que es una alternativa a la introducción de un nombre identificador .

(Negación:. No escribo Scala, por lo que si todo lo que digo en conflicto con las convenciones de estilo, entonces me ignoran :), pero me imagino una gran cantidad de este consejo es sobre todo-independiente del idioma)

1

Además de solución de Kevin,

la clave es dividir la funcionalidad clara y cuidadosamente, tomando en consideración de la reutilización y la legibilidad.

Para que el código sea aún más corto y más limpio, parece que se puede utilizar la expresión para.


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 
} 

El s.map (_- 'A' + 1) parte se puede poner más en una función, por ejemplo wordSum, si usted quiere que sea más clara.

+0

Gracias. Todo se trata de preferencia personal, por supuesto, pero encuentro la reelaboración de Kevin más clara. –

4

Hablando estrictamente a la forma de formato de ese código, sin hacer cambios estructurales, lo haría así:

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 
) 

O, tal vez, de moverse por los métodos sin parámetros, que había hacer esto:

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 
) 

Tenga en cuenta que hay un montón de espacio para comentarios, pero, en términos generales, lo que se está haciendo suele ser clara. Las personas que no están acostumbradas a ello pueden perderse a veces a la mitad, sin variables para realizar un seguimiento del significado/tipo del valor transformado .

Ahora, algunas cosas que haría de manera diferente son:

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 
) 

yo tampoco hago suma y la multiplicación en el mismo paso, por lo que:

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

Por último, I don' Es muy parecido al cambio de tamaño de la cuerda, así que podría recurrir a Regex en su lugar. Añadir un poco de refactorización, y esto es algo que probablemente escribiría:

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) 

EDITAR

El siguiente paso sería una refactorización de cambio de nombre - la elección de nombres que mejor transmiten lo que cada parte del código está haciendo. Ken sugirió mejores nombres en los comentarios, y los apropiaría para una variante más (también muestra cuánto mejor nombrar a mejora la legibilidad).

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) 
+0

Me gusta este el mejor. Renombraría algunas variables para aclarar un poco las cosas: 'names -> rawData' (reflejar que es una cadena grande),' wordsAV -> alphabeticalValues' (evitar el acrónimo), 'wordsAVByIndex -> scores' (evitar el acrónimo , llámalos puntajes porque eso es lo que el Proyecto Euler les llama) –

+0

@Ken No cambié el nombre de la refactorización. :-) De hecho, me gustan todas sus sugerencias mejor que las variables que actualmente se nombran. Tal vez debería incluso cambiar el código, para hacerlo más legible? –

+0

sí, por favor cámbielo (agregando otra variante a su respuesta) para que los futuros lectores de la página puedan beneficiarse de sus opiniones al respecto. –

Cuestiones relacionadas