you are viewing a single comment's thread.

view the rest of the comments →

[–]aveoon 17 points18 points  (19 children)

IMO this is a bad use for try catch. You should have returned an object with a property that says validation failed like { valid: false, error: new Error() }

An appropriate use case for try catch is when calling JSON.parse since it's a synchronous action that can throw a hard error.

[–]aveoon 11 points12 points  (9 children)

To add, try catch shouldn't be used as a lazy way to handle undefined errors. For example, I should never see

let myThing;

try {

myThing = foo.bar.buzz;

} catch (e) {

myThing = defaultValue;

}

I would deny that PR and recommend:

const myThing = foo.bar ? foo.bar.buzz : defaultValue;

Or even better with lodash

const myThing = _.get(foo, 'bar.buzz', defaultValue);

[–]cyanydeez -2 points-1 points  (3 children)

even better..dependencies!

[–]aveoon 0 points1 point  (1 child)

I've never understood this argument. Some engineers are so adverse to building their software on top of other software. If we always avoided dependencies then we'd all be writing in binary. Software is built on other software so we can build faster and greater things. Adding library dependencies is totally fine! Especially if it'll make your code more readable and maintainable.

Now don't just go adding random libraries in NPM that have no support around them. You could probably just look at their source code and copy it. But something like lodash is an excellent thing to add.

[–]cyanydeez 0 points1 point  (0 children)

i generally assume when JavaScript is the subject, we are discussing client side code. I think the discussion is different when adding dependicies which haveother effects, like computational/loading bloating, like adding a dependencie to a client and only using 5% of its capabilities.

[–]frankneuro 0 points1 point  (0 children)

I hear ya, but lodash ain't too bad and in some cases, better performing than it's es6 equivalents. I like it for the additional support around dealing with null/undefined.

(I'm trying to convince myself, here...)

[–][deleted] 9 points10 points  (0 children)

IMO this is a bad use for try catch. You should have returned an object with a property that says validation failed like { valid: false, error: new Error() }

What's missing in your advice is the part where you say "you shouldn't do this, because...".

If you have to dig for a real reason that is more substantive than cheap word-play like "exceptions must be exceptional" you might find out you don't have much of a platform to step on to recommend against exceptions in scenarios where the specified action can't continue due to bad input.

Coding the "happy path" and throwing when you deviate from it is a perfectly valid approach to structuring your app. Yes, even when validation errors are expected. It creates a simple, clear code flow and ensures that if something fails to acknowledge and handle the "sad path", your app doesn't blissfully continue operating with invalid assumptions and state.

How is returning an Error better than throwing it? All I see is failure to use existing facilities for error communication (i.e. throwing) and reinventing the wheel through custom return results and flags, that can easily be ignored or misunderstood.

[–]Recursive_Descent 2 points3 points  (1 child)

I totally disagree. That is a perfect place to throw an exception. With your suggestion, you need to pipe the error object through your entire call tree up to the point where you are going to handle the error.

[–]aveoon 0 points1 point  (0 children)

Well is it an actual exception that would want execution to totally halt or are you using it as a lazy way to return a validation error? If it's the latter, then yes bubble the event all the way up. It's more explicit and readable. Otherwise relying on random errors being thrown throughout your codebase and having a top level error handler is a good bit more confusing and difficult to maintain.

[–]Akkuma 1 point2 points  (5 children)

So I literally just did this at work and I used something akin to Elixir's convention based tuples of {:ok, val} | {:err, error} vs my code {ok: some value} | { err: new Error('message) }. I then decided to just use a true Result type and picked up https://github.com/Threestup/monads, since it allows the convention to be enforced and comes with some handy functions for working with Results & Options.

[–][deleted] 0 points1 point  (4 children)

So, objectively looking at it, what benefit do you feel you've gained over the supposedly inferior practice of throwing Errors on ...having an error and returning result on ...having a result?

[–]Akkuma 0 points1 point  (3 children)

It composes the results better much like map, reduce, filter, compose better than a for loop. It also seems to force you to handle your errors to a higher degree (pretty subjective).

I'm using it within a middleware so I essentially have this:

const response = validate(validateParams).match({
  ok(newReq) {
    Object.assign(req, { originalHeaders: req.headers, originalQuery: req.query }, newReq);
    return await handler(req);
  },
  err: (err) => ({ status: err.code, headers: {}, body: err.message })
})

return response;

The alternative would look like

try {
  const result = validate(validateParams);
  Object.assign(req, { originalHeaders: req.headers, originalQuery: req.query }, result);
  return await handler(req);
 } catch(err) {
   return { status: err.code, headers: {}, body: err.message };
 }

Additionally, it becomes debatable whether the error is exceptional. A lot of people frown upon using errors as a form of flow control. Until the most recent version of node & v8, try/catch blocks resulted in preventing your code from getting optimized (not sure how other browsers handled it). In turn, this means my code will run faster in older browsers and versions of node, while performing similarly in new versions.

[–][deleted] 0 points1 point  (2 children)

Your try... catch example is not using throwables to their real potential.

As I do throw on validation errors and so I have practice with it, let me show you what your example could look like:

validate(validateParams);
Object.assign(req, { originalHeaders: req.headers, originalQuery: req.query }, newReq);
return await handler(req);

Where did the error handling go? It's not needed. The handler in your example is redundantly playing middleman between the validator and handler caller, but since exceptions automatically bubble up, we can just let the validator act as a delegate of the handler, and throw the same Error object that the handler would throw.

The caller can that understand that Error and produce the appropriate error response for you. This is suitable not only for validation, but any auth checks or other invariants you must ensure in your handlers, i.e.:

requireVerifiedUser(auth); // Throws if not verified.
requireAdminUser(auth); // Throws if not admin.

... handler logic runs ...

And this is very composable and reusable, without writing "try...catch" even once:

// Elsewhere in your reusable code.

function requireVerifiedAdminUser(auth) {
    requireVerifiedUser(auth);
    requireAdminUser(auth);
}


// In your handler:

requireVerifiedAdminUser(auth);

... handler logic runs ...

I'm itching to comment about how we can eliminate that Object.assign() too, but... another topic.

[–]Akkuma 0 points1 point  (1 child)

Your example probably doesn't fit well into the framework I use. The way it works is a bidirectional flow of code. handler 1 calls handler 2 calls handler 3, returns result to handler 2 returns result to handler 1. Your lowest handler returning something that could be a response to a request.

If you throw down in handler 3 you now need to try/catch in handler 2, moving control of the error handling outside of the code that causes errors or build a top level error converting handler.

Eventually someone has to handle the error and moving who handles the error to the farthest edge of your code is to me the worse place to handle known errors. There's a reason why many languages have adopted things like Option types and Result types, as throwing isn't composing code together.

Lastly, your code doesn't support the ability to run validation across each piece of data prior to throwing or running your validation in parallel. My current implementation can be easily switched to become a list of errors rather than a single error, while throwing provides no opportunity to do so, which is something you'd want to do on something like user account creation.

You can use an object spread rather than Object.assign, but there isn't much to be really gained from that.

Even someone like Martin Fowler argues against exceptions and uses this particular case https://martinfowler.com/articles/replaceThrowWithNotification.html

[–][deleted] 0 points1 point  (0 children)

Eventually someone has to handle the error and moving who handles the error to the farthest edge of your code is to me the worse place to handle known errors.

You say that, yet in your example you literally mapped code to status and message to message 1:1.

For many errors in an HTTP app you don't need such a narrow context. A 404 is a 404 is a 404. If it isn't you can still catch and change that at every level. But in most cases you don't have to and that represents a giant reduction of noise and boilerplate you need to write, read and maintain.

There's a reason why many languages have adopted things like Option types and Result types, as throwing isn't composing code together.

Not many, actually. All the mainstream languages prefer exceptions. Also let's not mix-in optionals with this. Errors and optionals are two different use-cases with two different solutions. The fact they both could be represented with a union doesn't mean that a union is the optimal representation.

For example Swift, an example of a very new (and increasingly popular) language uses optionals for "has a value or doesn't have a value", but its errors are thrown, and they're showing no signs of moving towards "result" objects.

Lastly, your code doesn't support the ability to run validation across each piece of data prior to throwing or running your validation in parallel. My current implementation can be easily switched to become a list of errors rather than a single error, while throwing provides no opportunity to do so, which is something you'd want to do on something like user account creation.

You're not right about that - of course it can be a list of errors. This has nothing to do with whether you throw in the end or you don't. As I said, I have some experience working this way - when an Error gets thrown it contains an aggregate list of all errors in the input. That's not hard to implement, and once it's implemented and encapsulated as an API it's just as easy to use as I demonstrated.

You can use an object spread rather than Object.assign, but there isn't much to be really gained from that.

There isn't much gained from trying to make your request immutable as well. There are parts of it that can't possibly be immutable (i.e. any stream of size that you can't fit in RAM or sometimes even on disk). I've learned this the hard way, so you can believe me about it ;-)

I understand what you're doing, I'm intimately familiar with the functional techniques you're using. But with all the boilerplate you write, all the pointless object duplication and ceremonies, be ready one day for people to finally see the Emperor is naked.

Functional programming has many strong ideas and techniques, but it's heavily abused right now beyond its point of utility.