10

When I'm trying to create an interface for a specific program I'm generally trying to avoid throwing exceptions that depend on non-validated input.

So what often happens is that I've thought of a piece of code like this (this is just an example for the sake of an example, don't mind the function it performs, example in Java):

public static String padToEvenOriginal(int evenSize, String string) {
    if (evenSize % 2 == 1) {
        throw new IllegalArgumentException("evenSize argument is not even");
    }

    if (string.length() >= evenSize) {
        return string;
    }

    StringBuilder sb = new StringBuilder(evenSize);
    sb.append(string);
    for (int i = string.length(); i < evenSize; i++) {
        sb.append(' ');
    }
    return sb.toString();
}

OK, so say that evenSize is actually derived from user input. So I'm not sure that it is even. But I don't want to call this method with the possibility that an exception is thrown. So I make the following function:

public static boolean isEven(int evenSize) {
    return evenSize % 2 == 0;
}

but now I've got two checks that perform the same input validation: the expression in the if statement and the explicit check in isEven. Duplicate code, not nice, so let's refactor:

public static String padToEvenWithIsEven(int evenSize, String string) {
    if (!isEven(evenSize)) { // to avoid duplicate code
        throw new IllegalArgumentException("evenSize argument is not even");
    }

    if (string.length() >= evenSize) {
        return string;
    }

    StringBuilder sb = new StringBuilder(evenSize);
    sb.append(string);
    for (int i = string.length(); i < evenSize; i++) {
        sb.append(' ');
    }
    return sb.toString();
}

OK, that solved it, but now we get into the following situation:

String test = "123";
int size;
do {
    size = getSizeFromInput();
} while (!isEven(size)); // checks if it is even
String evenTest = padToEvenWithIsEven(size, test);
System.out.println(evenTest); // checks if it is even (redundant)

now we've got a redundant check: we already know that the value is even, but padToEvenWithIsEven still performs the parameter check, which will always return true, as we already called this function.

Now for isEven of course doesn't pose a problem, but if the parameter check is more cumbersome then this may incur too much cost. Besides that, performing a redundant call simply doesn't feel right.

Sometimes we can work around this by introducing a "validated type" or by creating a function where this issue cannot occur:

public static String padToEvenSmarter(int numberOfBigrams, String string) {
    int size = numberOfBigrams * 2;
    if (string.length() >= size) {
        return string;
    }

    StringBuilder sb = new StringBuilder(size);
    sb.append(string);
    for (int i = string.length(); i < size; i++) {
        sb.append('x');
    }
    return sb.toString();
}

but this requires some smart thinking and quite a large refactor.

Is there a (more) generic way in which we can avoid the redundant calls to isEven and performing double parameter checking? I'd like the solution not to actually call padToEven with an invalid parameter, triggering the exception.


With no exceptions I do not mean exception-free programming, I mean that user input doesn't trigger an exception by design, while the generic function itself still contains the parameter check (if just to protect against programming errors).

  • Are you just trying to remove the exception? You can do that by making an assumption about what the actual size should be. For example, if you pass in 13, pad to either 12 or 14 and just avoid the check altogether. If you cannot make one of those assumptions, then you're stuck with the exception because the parameter is unusable and your function cannot continue. – Robert Harvey Jan 14 '17 at 15:53
  • @RobertHarvey That - in my opinion - goes straight against the principle of least surprise as well as against the principle of fail fast. It's much like returning null if the input parameter is null (and then forgetting to handle the result correctly, of course, hello unexplained NPE). – Maarten Bodewes Jan 14 '17 at 15:57
  • Ergo, the exception. Right? – Robert Harvey Jan 14 '17 at 15:58
  • 2
    Wait, what? You did come here for advice, right? What I'm saying (in my apparently not so subtle way), is that those exceptions are by design, so if you want to eliminate them, you probably ought to have a good reason. By the way, I agree with amon: you probably shouldn't have a function that can't accept odd numbers for a parameter. – Robert Harvey Jan 14 '17 at 16:07
  • 1
    @MaartenBodewes: You have to remember that padToEvenWithIsEven does not perform validation of user input. It performs a validity check on its input to protect itself against programming errors in the calling code. How extensive this validation needs to be depends on a cost/risk analysis where you put the cost of the check against the risk that the person writing the calling code passes in the wrong parameter. – Bart van Ingen Schenau Jan 15 '17 at 10:10
7

In the case of your example, the best solution is to use a more general padding function; if the caller wants to pad to an even size then they can check that themselves.

public static String padString(int size, String string) {
    if (string.length() >= size) {
        return string;
    }

    StringBuilder sb = new StringBuilder(size);
    sb.append(string);
    for (int i = string.length(); i < size; i++) {
        sb.append(' ');
    }
    return sb.toString();
}

If you repeatedly perform the same validation on a value, or want to only allow a subset of values of a type, then microtypes / tiny types can be helpful. For general purpose utilities like padding this is not a good idea, but if your value plays some particular role in your domain model, using a dedicated type instead of primitive values can be a great step forward. Here, you might define:

final class EvenInteger {
  public final int value;

  public EvenInteger(int value) {
    if (!(value % 2 == 0))
      throw new IllegalArgumentException("EvenInteger(" + value + ") is not even");
    this.value = value;
  }
}

Now you can declare

public static String padStringToEven(EvenInteger evenSize, String string)
    ...

and don't have to do any inside validation. For such a simple test wrapping an int inside an object will likely be more expensive in terms of run time performance, but using the type system to your advantage can reduce bugs and clarify your design.

Using such tiny types can even be useful even when they perform no validation, e.g. to disambiguate a string representing a FirstName from a LastName. I frequently use this pattern in statically typed languages.

  • Doesn't your second function still thrown an exception in some cases? – Robert Harvey Jan 14 '17 at 16:05
  • 1
    @RobertHarvey Yes, for example in case of null pointers. But now the main check – that the number is even – is forced out of the function into the responsibility of the caller. They can then deal with the exception in a manner they see fit. I think the answer focusses less on getting rid of all exceptions than getting rid of code duplication in validation code. – amon Jan 14 '17 at 16:08
  • 1
    Great answer. Of course in Java the penalty for creating data classes is quite high (a lot of additional code) but that's more a problem of the language than the idea you've presented. I guess that you would not use this solution for all of the use cases where my problem would occur, but it is a good solution against overusing primitives or for edge cases where the cost of the parameter check is exceptionally hard. – Maarten Bodewes Jan 14 '17 at 16:25
  • 1
    @MaartenBodewes If you want to do validation, you can't get rid of something like exceptions. Well, the alternative is to use a static function that returns a validated object or null on failure, but that has basically no advantages. But by moving the validation out of the function into the constructor, we can now call the function without the possibility of a validation error. That gives the caller all the control. E.g.: EvenInteger size; while (size == null) { try { size = new EvenInteger(getSizeFromInput()); } catch(...){}} String result = padStringToEven(size,...); – amon Jan 15 '17 at 15:19
  • 1
    @MaartenBodewes Duplicate validation happens all the time. For instance, before trying to close a door your may check if it's already closed, but the door itself will also not allow to be re-closed if it already is. There is just no way out of this, except if you always trust that the caller doesn't perform any invalid operations. In your case you may have a unsafePadStringToEven operation that doesn't perform any checks, but it seems like a bad idea just to avoid validation. – plalx Jan 17 '17 at 3:31
8

As an extension to @amon's answer, we can combine his EvenInteger with what the functional programming community might call a "Smart Constructor" -- a function that wraps the dumb constructor and makes sure that the object is in a valid state (we make the dumb constructor class or in non-class based languages module/package private to make sure only the smart constructors are used). The trick is to return an Optional (or equivalent) to make the function more composable.

public final class EvenInteger {
    private final int value;

    private EvenInteger(value) {
        this.value = value;
    }

    public static Optional<EvenInteger> of(final int value) {
        if (value % 2 == 0) {
            return Optional.of(new EvenInteger(value));
        }
        return Optional.empty();
    }

    public int getValue() {
        return this.value;
    }
}

We can then easily use standard Optional methods to write the input logic:

class GetEvenInput {
    public Optional<EvenInteger> askOnce() {
        int size = getSizeFromInput();
        return EvenInteger.of(size);
    }

    public EvenInteger keepAsking() {
        return askOnce().orElseGet(() -> keepAsking());
    }
}

You could also write keepAsking() in a more idiomatic Java style with a do-while loop.

Optional<EvenInteger> result;
do {
    result = askOnce();
} while (!result.isPresent());

return result.get();

Then in the rest of your code you can depend on EvenInteger with certitude that it really will be even and our even check was only ever written once, in EvenInteger::of.

  • Optional in general represents potentially invalid data quite well. This is analogous to a plethora of TryParse methods, which return both data and a flag indicating input validity. With complie time checks. – Basilevs Jan 17 '17 at 4:54
0

Doing validation twice is a problem if the result of the validation is the same AND the validation is being done in the same class. That is not your example. In your refactored code, the first isEven check that is done is an input validation, failure results in a new input being requested. The second check is completely independent of the first, as it is on a public method padToEvenWithEven which can be called from outside of the class and has a different result (the exception).

Your problem is similar to the problem of accidentally identical code being confused for non-dry. You are confusing the implementation, with the design. They aren't the same, and just because you have one or a dozen lines that are the same, doesn't mean that they are serving the same purpose and can forever be consided interchangeable. Also, probably having your class do too much, but skip that as this is probably just a toy example...

If this is a performance issue, you can solve the problem by creating a private method, that doesn't do any validation, which your public padToEvenWithEven called after doing it's validation and which your other method would call instead. If it's not a performance issue, then let your different methods do the checks that they require to do their assigned tasks.

  • OP stated that the input validation is done to prevent the function throwing. So the checks are completely dependent and intentionally the same. – D Drmmr May 11 '18 at 21:54
  • @DDrmmr: no, they aren't dependent. The function throws because that is part of it's contract. As I said, the answer is to create a private method does everything except throw the exception and then have the public method call the private method. The public method retains the check, where it serves a different purpose--to handle unvalidated input. – jmoreno May 12 '18 at 0:42

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.