This seems good.
I noticed a couple of things, I will try to explain down below.
- 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.
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) {
// ...
}
}
- 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.
- 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
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.
- 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.
