1
\$\begingroup\$

Why did I make this? I'm building an interactive data structure visualization, and when certain errors are thrown from certain parts of the code, they are displayed on screen for the user. I have not included that displaying logic.

The if checks seem somewhat smelly to me, but they might be simple enough to the point that they're fine. I'm interested in best practices, ANY criticism is welcome.

export class OutOfBoundsError extends AppError { 
// AppError code is not included, but it is just an extremely simple
//abstract class I needed so I could instanceof to check whether a caught
// error was thrown by my own code
// If you want, pretend I'm extending JS's RangeError instead

constructor(valueName: 'length' | 'index' | 'position', value: number,
              bounds?: {
                lowerBoundInclusive?: number;
                upperBoundInclusive?: number
              }) {
    super();
    const lower = bounds?.lowerBoundInclusive;
    const upper = bounds?.upperBoundInclusive;
    if (lower === undefined) {
      if (upper === undefined) {
        this.message = `${valueName} of ${value} is out of bounds`;
      } else {
        this.message = `${valueName} must be less than or equal to ${upper} but\
         ${value} was given`;
      }
    } else {
      if (upper === undefined) {
        this.message = `${valueName} must be greater than or equal to ${lower}\
         but ${value} was given`;
      } else {
        this.message = `${valueName} must be between ${lower} and ${upper} but\
         ${value} was given`;
      }
    }
  }
}
```
\$\endgroup\$
2
\$\begingroup\$

This seems good.
I noticed a couple of things, I will try to explain down below.

  1. Lack of type definition
    For the sake of readability, it would be great to have some type definitions to make the code look easier to read.
type ValueName = 'length' | 'index' | 'position';

interface Bounds {
    lowerBoundInclusive?: number;
    upperBoundInclusive?: number;
}

export class OutOfBoundsError extends AppError {
    // ...

    constructor(name: ValueName, value: number, bounds?: Bounds) {
        // ...
    }
}

Since Bounds is an structured type, it deserves to be an interface.
The point of defining types is not just the code itself, helps in case of documentation, code hints (code completion, parameters information, ...) and reusability.
In case of reusability, types should be separated from implementations.


  1. message internal property misplace
    Inside your constructor, you are setting the value of message. Unlike Javascript, Typescript needs placeholders for internal properties.
    Maybe you had, indeed. Your code does not compile, otherwise.
    Anyways, internal properties must be declared between the class definition and its constructor.
export class OutOfBoundsError extends AppError {
    private message: string;

    constructor(name: ValueName, value: number, bounds?: Bounds) {
        // ...
    }
}

  1. Truthy and falsy assertions
    Avoid checking with explicit undefined, use if (value) instead.
    If value is a number type and in fact you are expecting a 0 to be a valid value, Javascript will coerce it into a false. In that case, check your value like so
if (value != null) {
   // ...
}

By checking with null with a single equal symbol, you are checking for undefined and null, as well.


  1. Two or more depth levels
    Functions should not have more than one depth level. Depth levels are done by using if, for, while and switch.
    Depth levels don't apply to inner functions, object or array instantiations, ...
interface ErrorMessageArgs {
    value: number;
    upper?: number;
    lower?: number;
}

export class OutOfBoundsError extends AppError {
    private message: string = ``;

    constructor(private name: ValueName, value: number, bounds?: Bounds) {
        super();

        const lower = bounds?.lowerBoundInclusive;
        const upper = bounds?.upperBoundInclusive;

        if (lower != null) {
          this.handleTruthyLower({value, upper});
        } else {
          this.handleFalsyLower({value, upper, lower});
        }
    }

    private handleFalsyLower = (args: ErrorMessageArgs) => {
        const { value, upper } = args;
        // ...
    }

    private handleTruthyLower = (args: ErrorMessageArgs) => {
        const { value, upper, lower } = args;
        // ...
    }
}

I decide to define yet another interface to wrap the error message arguments. By doing this, I can define both functions as monadic functions (just because of Robert).

The ideal number of arguments for a function is zero (niladic). Next comes one (monadic), followed closely by two (dyadic). Three arguments (triadic) should be avoided where possible. More than three (polyadic) requires very special justification — and then shouldn’t be used anyway.
Robert C. Martin


  1. message property protection
    I assume that a third party which manipulates this class is not allowed to modify the error message. It doesn't make any sense, in my opinion.
    That is why it may define a read-only access.
export class OutOfBoundsError extends AppError {
    private _message: string;

    constructor(private name: ValueName, value: number, bounds?: Bounds) {
        super();
        // ...
    }

    private handleFalsyLower = (args: ErrorMessageArgs) => {
        const { value, upper } = args;
        // ... message is filled
    }

    private handleTruthyLower = (args: ErrorMessageArgs) => {
        const { value, upper, lower } = args;
        // ... message is filled
    }

    get message() {
        return this._message;
    }
}

Maybe, for more consistency, _message should be protected if OutOfBoundsError is not a sealed class.
And yes, I know that in some case you defined this message property inside the AppError class. I just mention, just in case.


  1. Ternary operation
    This is just a preference. When there is only one line to write inside the if and else statements, I just prefer a ternary operation. It seems cleaner to me.
export class OutOfBoundsError extends AppError {
    protected _message = ``;

    constructor(private name: ValueName, value: number, bounds?: Bounds) {
        super();

        const lower = bounds?.lowerBoundInclusive;
        const upper = bounds?.upperBoundInclusive;

        lower != null
            ? this.handleTruthyLower({value, upper})
            : this.handleFalsyLower({value, upper, lower});
    }

    private handleFalsyLower = (args: ErrorMessageArgs) => {
        const { value, upper } = args;
        this._message = upper != null
                        ? `${this.name} must be less than or equal to ${upper} but\
                            ${value} was given`
                        : `${this.name} of ${value} is out of bounds`;
    }

    private handleTruthyLower = (args: ErrorMessageArgs) => {
        const { value, upper, lower } = args;
        this._message = upper != null
                        ? `${this.name} must be between ${lower} and ${upper} but\
                            ${value} was given`
                        : `${this.name} must be greater than or equal to ${lower}\
                            but ${value} was given`;
    }

    get message() {
        return this._message;
    }
}

Even for void function calls.

Hope it helps and any suggestions are welcome.

\$\endgroup\$
5
  • \$\begingroup\$ Thanks a lot for the thorough answer. Commenting on each of your points: - 1: I think its arguable whether the additional type/interface are necessary. It is already pretty readable/understandable as is, due to adequate variable naming, and the type/interface seem like over engineering and unnecessary boilerplate. I would agree if it were the case that they were used in any other place in the app (except calls to this constructor), which they are not. \$\endgroup\$ – habit May 24 '20 at 7:37
  • \$\begingroup\$ - 2: As you hinted at in point 5, the message property does come from AppError (in fact AppError inherits it from JS's Error Class), so I didn't need to define it as you described. - 3: I don't need to check for null, as I am trusting TS's type system to not allow null to be passed in to those properties (they are not nullable). Checking only for undefined makes this more explicit and makes possible bugs wherein null is somehow passed to this function not fail silently. \$\endgroup\$ – habit May 24 '20 at 7:38
  • \$\begingroup\$ - 4: I agree with this point, and like the idea of reducing the depth level; it does indeed make it a lot easier to follow. - 5: As previously mentioned, the message comes from JS's Error class, and I don't see a good reason to interfere with its public access. - 6: I agree with this one. Given point number 4 and the creation of additional methods, I agree that the use of ternaries makes it cleaner. I will mention that without point 4, the double with two depth levels would be quite harder to follow. \$\endgroup\$ – habit May 24 '20 at 7:38
  • \$\begingroup\$ Finally, even though I currently only agree with points 4 and 6, they really are quite useful to me. Thanks a lot. \$\endgroup\$ – habit May 24 '20 at 7:39
  • \$\begingroup\$ Edit to my comment that starts with -4: ...double TERNARY with two depth... \$\endgroup\$ – habit May 24 '20 at 19:32

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.