|
| 1 | +## 1. Hygienic Rules |
| 2 | + |
| 3 | +<img src="https://raw.githubusercontent.com/monifu/scala-best-practices/master/assets/scala-logo-256.png" align="right" width="128" height="128" /> |
| 4 | + |
| 5 | +These are general purpose hygienic rules that transcend the language |
| 6 | +or platform rules. Programming language is a form of communication, |
| 7 | +targeting computer systems, but also your colleagues and your future |
| 8 | +self, so respecting these rules is just like washing your hands after |
| 9 | +going to the bathroom. |
| 10 | + |
| 11 | +### 1.1. SHOULD enforce a reasonable line length |
| 12 | + |
| 13 | +There's a whole science on typography which says that people lose |
| 14 | +their focus when the line of text is too wide, a long line makes it |
| 15 | +difficult to gauge where the line starts or ends and it makes it |
| 16 | +difficult to continue on the next line below it, as your eyes have to |
| 17 | +move a lot from the right to the left. It also makes it difficult to |
| 18 | +scan for important details. |
| 19 | + |
| 20 | +In typography, the optimal line length is considered to be somewhere |
| 21 | +between 50 and 70 chars. |
| 22 | + |
| 23 | +In programming, we've got indentation, so it's not feasible to impose |
| 24 | +a 60 chars length for lines. 80 chars is usually acceptable, but not |
| 25 | +in Scala, because in Scala we use a lot of closures and if you want |
| 26 | +long and descriptive names for functions and classes, well, 80 chars |
| 27 | +is way too short. |
| 28 | + |
| 29 | +120 chars, as IntelliJ IDEA is configured by default, may be too wide |
| 30 | +on the other hand. Yes, I know that we've got 16:9 wide monitors, but |
| 31 | +this doesn't help readability and with shorter lines we can put these |
| 32 | +wide monitors to good use when doing side-by-side diffs. And with long |
| 33 | +lines it takes effort to notice important details that happen at the |
| 34 | +end of those lines. |
| 35 | + |
| 36 | +So as a balance: |
| 37 | + |
| 38 | +- strive for 80 chars as the soft limit and if it gets ugly, |
| 39 | +- then 100 chars is enough, except for ... |
| 40 | +- function signatures, which can get really ugly if limited |
| 41 | + |
| 42 | +On the other hand, anything that goes beyond 120 chars is an abomination. |
| 43 | + |
| 44 | +### 1.2. MUST NOT rely on a SBT or IDE plugin to do the formatting for you |
| 45 | + |
| 46 | +IDEs and SBT plugins can be of great help, however if you're thinking |
| 47 | +about using one to automatically format your code, beware. |
| 48 | + |
| 49 | +You won’t find a plugin that is able to infer the developer’s intent, |
| 50 | +since that’s an NP-Complete problem (i.e. it requires human-like |
| 51 | +intelligence). The purpose of proper indentation and formatting isn't |
| 52 | +to follow some rigid rules set upon you in a cargo-cult way, but to |
| 53 | +make the code more logical, more readable, more |
| 54 | +approachable. Indentation is actually an art form, which is not awful |
| 55 | +since all you need is a nose for awful code and the urge of fixing |
| 56 | +it. And it is in the developer's job description to make sure that his |
| 57 | +code doesn't stink. |
| 58 | + |
| 59 | +So automated means are fine, BUT BE CAREFUL to not ruin other people's |
| 60 | +carefully formatted code, otherwise I'll slap you in prose. |
| 61 | + |
| 62 | +Lets think about what I said - if the line is too long, how is a |
| 63 | +plugin supposed to break it? Lets talk about this line (real code): |
| 64 | + |
| 65 | +```scala |
| 66 | + val dp = new DispatchPlan(new Set(filteredAssets), start = startDate, end = endDate, product, scheduleMap, availabilityMap, Set(activationIntervals.get), contractRepository, priceRepository) |
| 67 | +``` |
| 68 | + |
| 69 | +In most cases, a plugin will just do truncation and I've seen a lot of these in practice: |
| 70 | + |
| 71 | +```scala |
| 72 | + val dp = new DispatchPlan(Set(filteredAssets), start = |
| 73 | + startDate, end = endDate, product, scheduleMap, availabilityMap, |
| 74 | + Set(activationIntervals), contractRepository, priceRepository) |
| 75 | +``` |
| 76 | + |
| 77 | +Now that's not readable, is it? I mean, seriously, that looks like |
| 78 | +barf. And that's exactly the kind of output I see coming from people |
| 79 | +relying on plugins to work. We could also have this version: |
| 80 | + |
| 81 | +```scala |
| 82 | + val dp = new DispatchPlan( |
| 83 | + Set(filteredAssets), |
| 84 | + startDate, |
| 85 | + endDate, |
| 86 | + product, |
| 87 | + scheduleMap, |
| 88 | + availabilityMap, |
| 89 | + Set(activationIntervals), |
| 90 | + contractRepository, |
| 91 | + priceRepository |
| 92 | + ) |
| 93 | +``` |
| 94 | + |
| 95 | +Looks much better. But truth is, this isn't so good in other |
| 96 | +instances. Like say we've got a line that we want to break: |
| 97 | + |
| 98 | +```scala |
| 99 | + val result = service.something(param1, param2, param3, param4).map(x => transform(x)) |
| 100 | +``` |
| 101 | + |
| 102 | +Now placing those parameters on their own line is awful, no matter how you deal with it: |
| 103 | + |
| 104 | +```scala |
| 105 | + // awful because that transform call is not visible |
| 106 | + val result = service.something( |
| 107 | + param1, |
| 108 | + param2, |
| 109 | + param3, |
| 110 | + param4).map(x => transform(x)) |
| 111 | + |
| 112 | + / awful because it breaks the logical flow |
| 113 | + val result = service.something( |
| 114 | + param1, |
| 115 | + param2, |
| 116 | + param3, |
| 117 | + param4 |
| 118 | + ).map(x => transform(x)) |
| 119 | +``` |
| 120 | + |
| 121 | +This would be much better: |
| 122 | + |
| 123 | +```scala |
| 124 | + val result = service |
| 125 | + .something(param1, param2, param3, param4) |
| 126 | + .map(x => transform(x)) |
| 127 | +``` |
| 128 | + |
| 129 | +Now that's better, isn't it? Of course, sometimes that call is so long |
| 130 | +that this doesn't cut it. So you need to resort to a temporary value |
| 131 | +of some sort, e.g... |
| 132 | + |
| 133 | +```scala |
| 134 | + val result = { |
| 135 | + val instance = |
| 136 | + object.something( |
| 137 | + myAwesomeParam1, |
| 138 | + otherParam2, |
| 139 | + someSeriousParam3, |
| 140 | + anEvenMoreSoParam4, |
| 141 | + lonelyParam5, |
| 142 | + catchSomeFn6, |
| 143 | + startDate7 |
| 144 | + ) |
| 145 | + |
| 146 | + for (x <- instance) yield |
| 147 | + transform(x) |
| 148 | + } |
| 149 | +``` |
| 150 | + |
| 151 | +Of course, sometimes if the code stinks so badly, you need to get into |
| 152 | +refactoring - as in, maybe too many parameters are too much for a |
| 153 | +function ;-) |
| 154 | + |
| 155 | +And we are talking strictly about line lengths - once we get into |
| 156 | +other issues, things get even more complicated. So really, you won't |
| 157 | +find a plugin that does this analysis and that can make the right |
| 158 | +decision for you. |
| 159 | + |
| 160 | +### 1.3. SHOULD break long functions |
| 161 | + |
| 162 | +Ideally functions should only be a couple of lines long. If the lines |
| 163 | +get too big, then we need to break them into smaller functions and |
| 164 | +give them a name. |
| 165 | + |
| 166 | +Note that in Scala we don't necessarily have to make such intermediate |
| 167 | +functions available in other scopes, the purpose here is to primarily |
| 168 | +aid readability, so in Scala we can do inner-functions to break logic |
| 169 | +into pieces. |
| 170 | + |
| 171 | +### 1.4. MUST NOT introduce spelling errors in names and comments |
| 172 | + |
| 173 | +Spelling errors are freakishly annoying, interrupting a reader's flow. |
| 174 | +Use a spell-checker. Intelligent IDEs have built-in |
| 175 | +spell-checkers. Note the underlined spelling warnings and fix them. |
| 176 | + |
| 177 | +### 1.5. Names MUST be meaningful |
| 178 | + |
| 179 | +*"There are only two hard things in Computer Science: cache |
| 180 | +invalidation and naming things."* -- Phil Karlton |
| 181 | + |
| 182 | +We've got three guidelines here: |
| 183 | + |
| 184 | +1. give descriptive names, but don't go overboard, four words is a |
| 185 | + little too much already |
| 186 | +2. you can be terse in naming if the type / purpose can be easily |
| 187 | + inferred from the immediate context, or if there's already an |
| 188 | + established convention |
| 189 | +3. if going the descriptive route, don't do bullshit words that are |
| 190 | + meaningless |
| 191 | + |
| 192 | +For example this is acceptable: |
| 193 | + |
| 194 | +```scala |
| 195 | +for (p <- people) yield |
| 196 | + transformed(p) |
| 197 | +``` |
| 198 | + |
| 199 | +We can see that `p` is a person from the immediate context, so a short |
| 200 | +one letter name is OK. This is also acceptable because `i` is an |
| 201 | +established convention to use as an index: |
| 202 | + |
| 203 | +```scala |
| 204 | +for (i <- 0 until limit) yield ??? |
| 205 | +``` |
| 206 | + |
| 207 | +This is in general not acceptable, because usually with tuples the |
| 208 | +naming of the collection doesn't reflect well what's contained (if you |
| 209 | +haven't given those elements a name, then as a consequence the |
| 210 | +collection itself is going to have a bad name): |
| 211 | + |
| 212 | +``` |
| 213 | +someCollection.map(_._2) |
| 214 | +``` |
| 215 | + |
| 216 | +Implicit parameters on the other hand are OK with short names, because |
| 217 | +being passed implicitly, we don't care about them unless they are |
| 218 | +missing: |
| 219 | + |
| 220 | +```scala |
| 221 | +def query(id: Long)(implicit ec: ExecutionContext, c: WSClient): Future[Response] |
| 222 | +``` |
| 223 | + |
| 224 | +This is not acceptable because the name is utterly meaningless, even |
| 225 | +if there's a clear attempt at being descriptive: |
| 226 | + |
| 227 | +```scala |
| 228 | +def processItems(people: Seq[Person]) = ??? |
| 229 | +``` |
| 230 | + |
| 231 | +It's not acceptable because the naming of this function indicates a |
| 232 | +side-effect (`process` is a verb indicating a command), yet it doesn't |
| 233 | +describe what we are doing with those `people`. The `Items` suffix is |
| 234 | +meaningless, because we might have said `processThingy`, |
| 235 | +`processRows`, `processStuff` and it would still say exactly the same |
| 236 | +thing - absolutely nothing. It also increases visual clutter, as more |
| 237 | +words is more text to read and meaningless words are just noise. |
| 238 | + |
| 239 | +Properly chosen descriptive names - good. Bullshit names - bad. |
| 240 | + |
| 241 | + |
0 commit comments