Wandering water ways

An entry for the August 2016 Community Challenge

I slightly modified the input and output to suit the snippet tool, and also because I really like the mapped output with the basins.

I have been golfing a lot recently, which always impacts my coding style. You will find a number of possibly too smart one-liners. I am looking for more gracious alternatives.



//1 liner helper functions
const $ = id => { return document.getElementById(id); },
trim = s => { return s.trim(); },
splitNumbersOnSpace = s => { return s.split(' ').map(Number); },
join = a => { return a.join('') };

//DOM elements
const input = $('input'),
output = $('output'),
output2 = $('output2'),
go = $('go');

go.addEventListener('click', () => {

const depths = input.value.split('\n').map(trim).map(splitNumbersOnSpace),
basins = Array(depths.length).fill().map(()=>{return [];}),
basinDepths = new Map();
let nextBasin = 1;

//compare a spot with the lowest neighbour thus far
function compareNeighbour( lowestSpotSoFar, attemptRow, attemptCol ){
try {
if( depths[attemptRow][attemptCol]
#output, #input {
font-family: monospace;
}

1 0 2 5 8
2 3 4 7 9
3 5 7 8 9
1 2 5 4 3
3 3 5 2 1

Analyze


Output




`

I've been looking at this code for a good long while now, and I must admit I don't have much in terms of a review. It's all nicely done!

What few quibbles I have are minor:

-
output and output2 aren't really the best names, are they?

-
Also, basinDepths appears to actually hold basin sizes. The code already deals with "actual" depths, so the name's needlessly confusing.

-
Speaking of, "depth" is perhaps a slightly confusing term in this case. It's actually altitudes, and the rain flows from high to low. By calling it "depths", one would expect it to be the other way around: Water flowing to greater depth values.

-
The one-liners aren't all that useful. $ is fine (though maybe using document.querySelector would be nicer, as one might expect $ to behave jQuery-ish), but the rest are really only called once, as far as I can tell. So I would argue that this:

const depths = input.value
  .split('\n')
  .map(line => line.trim().split(' ').map(Number));

would be as clear as using extracted methods. Yes, it's nice to name and encapsulate the operations being performed, and the original line really reads quite well. Still... eh, just seems excessive when there's no re-use of said operations.

Conversely, if you want to go all-in on English-like legibility, a lines function should perhaps take the place of split('\n')?

-
Your use of whitespace is... eclectic? I'm used to reproaching people for not using enough whitespace, but here it's the opposite. Maybe you're over-correcting after too much code golf? :)

I just find it curious that a line like this:

function markPath( path , basin ){

has tons of space in the argument list, but nothing between ){ (esp. since you do use a space for x => { y } declarations... except in that place where you don't).

Or that your else-if looks like this:

}else if (...){

when most would probably write:

} else if(...) {

instead. Indeed you do write if(...) { in other places.

Again, it's not a disaster, but it is inconsistent and odd.

StackExchange Code Review Q#136752, answer score: 5