stevekane / promise-it-wont-hurt

A Workshopper module that teaches you to use promises in javascript
737 stars 219 forks source link

Is this a better/worse solution for "fetch json"? #24

Open CamiloMM opened 10 years ago

CamiloMM commented 10 years ago

I solved "fetch json" like this:

var qhttp = require('q-io/http');
qhttp.read('http://localhost:1337')
.then(JSON.parse)
.then(console.log)
.done();

Is it more straightforward? Or a bad practice?

For comparsion, here's the official solution:

var qhttp = require('q-io/http');

qhttp.read("http://localhost:1337")
.then(function (json) {
  console.log(JSON.parse(json));
})
.then(null, console.error)
.done()
StevenACoffman commented 10 years ago

I find yours very clever and elegant. It really exemplifies how promises can simplify code.

One thing I noticed is that you don't have any error handling in yours (not that this artificial situation is going to throw an error), so I'd like this better for the official answer:

var qhttp = require('q-io/http');
qhttp.read('http://localhost:1337')
  .then(JSON.parse)
  .then(console.log)
  .then(null, console.error)
  .done();

I would also note that the node style is to avoid using semicolons (I prefer to use them).

stevekane commented 10 years ago

Steven,

The solution as it was originally written was exactly as you have indicated. However, there is a bug in both our code snippets which is that the passing of "console.log" and "console.error" to higher-order-functions will cause the context of console to be lost. As a result, there is a run-time error when my code hits your success branch and either of our failure branches.

In seeing this, my standard solution would be to do the following:

var qhttp = require('q-io/http')
var log = console.log.bind(console)
var logErr = console.error.bind(console)

qhttp.read('http://localhost:1337')
  .then(JSON.parse)
  .then(log)
  .then(null, logErr)
  .done();

In seeing this, I elected not to do this for the official solution so as not to introduce irrelevant (debatable for sure) details into the solution which might confuse a reader and might also necessitate an explanation.

Additionally, I think using promise chaining as a poor man's synchronous function composition is an anti-pattern. I would much prefer that .then semantics be used ONLY when an operation in the chain is fundamentally async. The solution you propose would wait an additional process.nextTick between the parsing and logging steps (as mandated by the promises A+ spec) which is an unneeded performance hit.

Thus, the ideal solution in my preferred style of programming would be as follows:

var qhttp = require('q-io/http')
var compose = require("lodash.compose") 
var log = console.log.bind(console)
var logErr = console.error.bind(console)
var parse = JSON.parse.bind(JSON)
var parseAndLog = compose(log, parse)

qhttp.read('http://localhost:1337')
  .then(parseAndLog)
  .then(null, logErr)
  .done();

I prefer this solution as a functional programmer who deeply believes in building complexity out of simple parts. However, it's too many unrelated philosophical components for the canonical solution to this promises tutorial.

With all this in mind, I will update my solution to avoid the bug I indicated exists above.

Thanks for the issue report it's very helpful.

StevenACoffman commented 10 years ago

Thanks for a great set of exercises, and for your responsiveness. I don't notice any errors when I run the proposed solution, so I assume you're talking about in later more_functional or in other applications. Thanks again.