marijnh / Eloquent-JavaScript

The sources for the Eloquent JavaScript book
https://eloquentjavascript.net
3.01k stars 793 forks source link

Code suggestion for Robot chapter #455

Closed EvanCarroll closed 6 years ago

EvanCarroll commented 6 years ago

In CH7, you have

function buildGraph(edges) {
  let graph = Object.create(null);
  function addEdge(from, to) {
    if (graph[from] == null) {
      graph[from] = [to];
    } else {
      graph[from].push(to);
    }
  }
  for (let [from, to] of edges.map(r => r.split("-"))) {
    addEdge(from, to);
    addEdge(to, from);
  }
  return graph;
}

However, I don't see any of the edges lacking a - or having satisfying graph[from] == null. Perhaps that condition can be yanked. I also think this is awkward syntax

  for (let [from, to] of edges.map(r => r.split("-"))) {

I think that is better written as,

edges.forEach( road => {
  let [from,to] = road.split("-");
} );

Or, if you want to be more clever than that you can follow it through with a more traditional functional pipeline

edges.map( road => road.split("-") ).forEach( [k,v] => ...

All of these are more idiomatic js.

This chapter could definitely use some disambiguation too on the names.

For example

function buildGraph(edges) {

Should really be

function buildGraph(roads) {

The roads don't become edges until you extract directionality out of them,

roads.forEach( road => {
  let [from,to] = road.split("-");
  addEdge(from, to);
  addEdge(to, from);
} );

Would be far better for explaining.

marijnh commented 6 years ago

However, I don't see any of the edges lacking a - or having satisfying graph[from] == null

That's not what that tests. Try removing it and see it break. It handles the first time a node name is found, at which point it doesn't have an array of neighboring nodes yet.

I also think this is awkward syntax

'Awkward' is mostly a matter of what you're used to. I definitely prefer this form to the two you proposed.

The roads don't become edges until you extract directionality out of them,

The function works for any graph, so at this point edges is an appropriate word.