Ayende @ Rahien

Hi!
My name is Oren Eini
Founder of Hibernating Rhinos LTD and RavenDB.
You can reach me by email or phone:

ayende@ayende.com

+972 52-548-6969

, @ Q c

Posts: 6,421 | Comments: 47,496

filter by tags archive

PR ReviewIs your error handling required?

time to read 1 min | 129 words

During reviewing a PR I run into what seemed like a strange thing. Take a look at this change:

image

This came with its own exception class, and left me pretty confused. Why would I want to have something like that?

Here we have some error handling code that doesn’t seem to add any additional value. Everything in the error here can be deducted from the details of the exception that will be thrown if we did nothing.

The fact we throw a specialized exception might be meaningful, but looking at the code, this isn’t actually used for anything.

Like all code, error handling needs to justify itself, and this one doesn’t pass the bar.

PR Reviewavoid too many parameters

time to read 2 min | 228 words

During code review I run into these two sections, which raised a flag. Can you tell why?

image

image

The problem with this type of code is two fold. First, we add optional parameters, to reduce the number of breaking changes we have. The problem with that is that we already have parameters on the call, and eventually you’ll get to something like this:

image

Which is the queen of optional parameters method, and you can probably guess how it looks internally.

In the first case, we can add the new optional parameter to the… options variable that we are already sending this method. This way, we don’t have to worry about breaking changes, and we already have a way to setup options, determine defaults, etc.

In the second case, we are passing two bools to the method, and there isn’t a preexisting parameters object. Instead of creating one, we can use a Flags enum, whose bits we can set to determine what exactly the behavior of this method should be. That is generally much easier to maintain in the long run.

PR Reviewthe errors should be nurtured

time to read 2 min | 202 words

I run into the following in a PR I recently reviewed. Take a look and try to figure out why I’m pointing this bits out:

image_thumb[2]

image_thumb[3]

Those are bad errors. In the sense that they are hiding information. Sure, in 90% of the cases the user just put it the backup location or the restore location, so they know what the application is referring to. But the other 10% is looking at the exception from the logs, having “I got an error” support call or all sort of weird stuff.

In particular, one of the most annoying things is when the user and the application disagree on a relative path. If the user believes that the relative path is based on one location and the application another, hilarity ensues. But not in a fun way.

A better way to prevent all of those would be to just include the actual locations (fully resolved, mind) in the error. It doesn’t prevent support calls, but it does make them a lot easier to solve.

FUTURE POSTS

  1. Building a query parser over a weekend: Part II - 2 hours from now
  2. Let the Merge Games begin! - about one day from now
  3. Keeping track on long running branches - 2 days from now
  4. Error handling belongs at Layer 7 (policy) - 3 days from now
  5. Practice makes perfect - 6 days from now

And 2 more posts are pending...

There are posts all the way to Aug 02, 2017

RECENT SERIES

  1. Production postmortem (17):
    23 Aug 2016 - The insidious cost of managed memory
  2. Building a query parser over a weekend (2):
    24 Jul 2017 - Part I
  3. PR Review (3):
    21 Jul 2017 - Is your error handling required?
  4. Reviewing Resin (6):
    20 Jul 2017 - Part VI – Analyzing I/O and being unfair
  5. Inside RavenDB 4.0 (2):
    17 Jul 2017 - Chapter 6 is done
View all series

Syndication

Main feed Feed Stats
Comments feed   Comments Feed Stats