validation-challenge.md

Lets have an imaginary node core function that throws on invalid input:

function crop(image, x1, y1, x2, y2) {
  // throws if x1, y1, x2, y2 out of range for the image.
}

We wrote the following classes using this function:

var Transforms = {};

class ImageCropper() {
  constructor(range) {
    this._range = range
  }
  
  // Checks whether the range passed to the constructor above is out of bounds
  isValidOn(image) {
    var range = this._range;
    return 0 <= range.x1 && range.x1 < range.x2 && range.x2 <= image.width &&
           0 <= range.y1 && range.y1 < range.y2 && range.y2 <= image.height
  }
  applyTo(image) {
    if (!this.isValidOn(image)) {
      throw new RangeError("Crop coordinates out of bounds")
    }
    var r = this._range
    // checks input before passing it to the node function.
    return crop(image, r.x1, r.y1, r.x2, r.y2)
  }
}
Transforms.ImageCropper = ImageCropper;

class FacedetectCropper extends ImageCropper {
  autoDetectFace(image) {
    // runs costly algorithm and returns face range
  }
  applyTo(image) {
    // Assume autoDetectFace always comes up with a valid range
    // so we don't need to check the input.
    this._range = this.autoDetectFace(image);
    return super.applyTo(image);
  }
}

Transforms.FacedetectCropper = FacedetectCropper;

We also have the following function that applies multiple transforms, but doesn't check the inputs before passing them to the applyTo methods. We might need to fix this function:

function applyTransforms(transforms, image) {
  for (var k = 0; k < transforms.length; ++k) {
    image = transforms[k].applyTo(image);
  }
  return image;
}

And finally, we have a function that sends a http response, applying a face detection crop first, then an additional user-defined crop range

edit: We have a function that responds to a user request to transform an image

// Assume that we already passed through basic request validator for request.body params
// So all transforms are of valid type and have valid options for that route.
function runUserTransforms(request, response) {
  var transforms = request.body.transforms.map(t => new Transforms[t.type](t.options)));
  if ( ... write missing validation here ...) {
    response.answer(400, "Invalid combination of transforms specified!");
  }
  else response.answer(200, {..content type}, applyTransforms(transforms, request.body.image))
}

The challenge is:

• Write the code to check the inputs in runUserTransforms before passing them to applyTransforms

Assume that FacedetectCropper always detects valid coordinates

Points to consider:

• If the request specifies [FacedetectCropper, ImageCropper(range)], FacedetectCropper might crop the image to be too small for the subsequent ImageCropper range
• FacedetectCropper needs an actual image as input, even just to run the detection and produce coordinates. Consider the implications of that on a request like this: [ImageCropper(range), FacedetectCropper, ImageCropper(range)],

@bmeck but do you validate by calling the expensive faceDetection? and then calculate the intermediate image's dimensions, and then call isValidOn for the other transform again? Is that how its supposed to work? And also you would have to add that check to every single other layer you build on top of it? Each layer doing face detection over and over again?

In the general case:

function isValidOnAll(transforms, initialImage) {
  // we basically need to run all transforms to actually validate them.
  // more accurately, we need to run all previous transforms to validate the next one.
  return !! transforms.reduce((image, transform) => transform.isValidOn(image) && transform.applyTo(image), initialImage);
}

So the other idea is to stop pre-validating and switch to an {err, value} pair. My question is then at what point do we switch to this? Do we switch immediately in ImageCropper, or only when its impossible to continue validating and re-validating without actually doing the work (i.e. in applyTransform)?

Spion, adding a test for expected errors should occur at any step. Performing pre-application validation can prevent that. In the current design you don't have a means to safely cache the results since you are reusing the same object. If isValidOn was changed to getValidatedImage or similar that returned a new data stracuture you could save the isValidOn by producing a safe data structure rather than risking the need to recalculate / using a mutating data structure.

@spion , though yes, in all these cases you need to calculate the costly operation at least once, prior to application, since that costly operation is what determines if the input is valid or not. Since this can be cached/saved in various ways and it must be performed prior to full application anyway, this isn't really a problem.

@bmeck by all means, do let me know if you come up with such a design.

When I try to come up with one, I get stuck at the fact that I can't write applyTransforms anymore, even if I cache all calculations, because there is no way to know whether some of the transforms wont like the output from the previous transform. My only idea is to write a validator for every pair of transforms which knows what output from each transform may affect the input of another. Thats still n^2 validators. They would also have to cache calculation parameters then pass them to the transforms. Another alternative is to write a layer of N validators that "simulates" the image properties (i.e. mock image).

But I suspect both of these have similar characteristics to the halting problem: just as you can't determine whether a program will halt without running it in the general case, you also can't determine if it will produce invalid input for another program without running it (again, in the general case). The "mock image" of sorts will have to be actually real. For example, you cannot get output coordinates out of FacedetectTransform without providing an actual image on which the algorithm will run. Therefore, if any transforms precedes it, we'll have to run them all to get that image.

By the way, my alternative is https://gist.github.com/spion/816baa575454e9d77f81 but that doesn't qualify as "check your inputs before you pass them" - its more of an exception-like approach (in fact its pretty much identical to try-catch). Which is why I have problem with node throwing for input validation in the first place, or rather, why I'm okay with promises catching all thrown errors in node.

Edited the challenge to make it make more sense. The user specifies the list of transforms now.

Sorry about the many changes, I thought I could simplify things originally. Turns out this last version pretty much captures what a real application would want to do, and the example cannot be reduced further :)