0
\$\begingroup\$

If I have a one liner method that looks like this:

str.toLowerCase.replaceAll("[^\\w']", " ").split(" ").filter(word => word.trim.length >= 1).groupBy(word => word).mapValues(_.size)

What is the best way to avoid long lines of code and split it into multiple lines? Here are the options that I see:

  1. Assign part of the methods chain to a val and start from line 2 with val.(rest of method chain)
  2. Move some parts to own methods with meaningful names (onlyChars, trimmedText, ...)
  3. Just split the lines and make the second line nested in the first one
\$\endgroup\$
6
  • \$\begingroup\$ The desire to improve code is implied for all questions on this site. Question titles should reflect the purpose of the code, not how you wish to have it reworked. See How to Ask. \$\endgroup\$
    – Jamal
    Jun 28, 2015 at 21:23
  • \$\begingroup\$ I've updated the title, is it good enough now? \$\endgroup\$
    – HeeL
    Jun 28, 2015 at 21:40
  • \$\begingroup\$ You've only removed a few words and it still means the same thing. Be sure to read that link for more information. \$\endgroup\$
    – Jamal
    Jun 28, 2015 at 21:42
  • \$\begingroup\$ I've changed the title and the body of the question to make it concrete rather than hypothetical. \$\endgroup\$ Jun 29, 2015 at 6:49
  • 1
    \$\begingroup\$ Since it appears that you are asking about generic beat practices rather than requesting a code review of that particular one-liner, I have reverted my edit and closed this question again. \$\endgroup\$ Jun 29, 2015 at 13:12

2 Answers 2

5
\$\begingroup\$

You can split the line as @maaartinus suggested. Long chains like this are not unusual in Scala.

Alternatively, you could assign parts of the expression to variables, as you yourself suggested, for example:

val words = str.toLowerCase.replaceAll("[^\\w']+", " ").trim().split(" ")
words.groupBy(identity).mapValues(_.length)

Some of the items in your chain can be simpler and shorter too:

  • Instead of: .groupBy(word => word)
    • You can do: .groupBy(identity)
  • Instead of s.length >= 1 it's more idiomatic to write s.nonEmpty
  • Instead of replacing [\\w'] by space, trimming each item and check if not empty, it's simpler to replace [\\w']* and trim before the split, so that there will be no empty elements and no single space

Lastly, it's not about shortening, but instead of .mapValues(_.size), it's recommended to use .mapValues(_.length) with strings.

\$\endgroup\$
0
3
\$\begingroup\$

While I really hate long split lines, this one

str.toLowerCase.replaceAll("[^\\w']", " ").split(" ").filter(word => word.trim.length >= 1).groupBy(word => word).mapValues(_.size)

is just fine when written as

str.toLowerCase
    .replaceAll("[^\\w']", " ")
    .split(" ")
    .filter(word => word.trim.length >= 1)
    .groupBy(word => word)
    .mapValues(_.size)

This is surely better than methods doing close to nothing or local variables storing meaningless results. I'd consider them only when it gets really long or when there's a part worth extracting.


I hardly know Scala and there may be some automatic semicolon insertion rules prohibiting this. Then you could probably use the Javascript-like trick with dots placed on the previous line

str.toLowerCase.
    replaceAll("[^\\w']", " ").
    split(" ").
    ...
\$\endgroup\$

Not the answer you're looking for? Browse other questions tagged or ask your own question.