Skip to main content
Code Review

Return to Answer

added 131 characters in body
Source Link
r3dst0rm
r3dst0rm
  • 406
  • 3
  • 7

EDIT: As discussed in the comments, it is nicer to merge the private isInstance function into the static of function.

EDIT: As discussed in the comments, it is nicer to merge the private isInstance function into the static of function.

added 1453 characters in body
Source Link
r3dst0rm
r3dst0rm
  • 406
  • 3
  • 7

Alternative solution to eliminate the InternalPredicate and to support functions which are booleans:

type PredicateType<T> = (x: T) => boolean;

class Predicate<T> {
        constructor(private condition: PredicateType<T>) {}
        
        private static isInstance = <T>(input: Predicate<T> | PredicateType<T>): Predicate<T> => (input instanceof Predicate) ? input : Predicate.of(input);

        public static of = <T>(condition: PredicateType<T>) => new Predicate(condition);

        public and = (input: Predicate<T> | PredicateType<T>): Predicate<T> =>
          Predicate.of((x: T) => this.apply(x) && Predicate.isInstance(input).apply(x));

        public or = (input: Predicate<T> | PredicateType<T>): Predicate<T> =>
          Predicate.of((x: T) => this.apply(x) || Predicate.isInstance(input).apply(x));

        public not = (): Predicate<T> =>
          Predicate.of((x: T) => !this.apply(x));

        public apply = (x: T): boolean => this.condition(x);
}

It comes pretty close the the extractCondition approach, but the main difference here is, that a new instance is created if the input is a raw function. This adds up to be more robust to changes and being less error prone. Another advantage is, we don't care about how the predicate internally works, we're just in charge of returning a predicate based on the input. This clears up the use case of the function a bit more.

Alternative solution to eliminate the InternalPredicate and to support functions which are booleans:

type PredicateType<T> = (x: T) => boolean;

class Predicate<T> {
        constructor(private condition: PredicateType<T>) {}
        
        private static isInstance = <T>(input: Predicate<T> | PredicateType<T>): Predicate<T> => (input instanceof Predicate) ? input : Predicate.of(input);

        public static of = <T>(condition: PredicateType<T>) => new Predicate(condition);

        public and = (input: Predicate<T> | PredicateType<T>): Predicate<T> =>
          Predicate.of((x: T) => this.apply(x) && Predicate.isInstance(input).apply(x));

        public or = (input: Predicate<T> | PredicateType<T>): Predicate<T> =>
          Predicate.of((x: T) => this.apply(x) || Predicate.isInstance(input).apply(x));

        public not = (): Predicate<T> =>
          Predicate.of((x: T) => !this.apply(x));

        public apply = (x: T): boolean => this.condition(x);
}

It comes pretty close the the extractCondition approach, but the main difference here is, that a new instance is created if the input is a raw function. This adds up to be more robust to changes and being less error prone. Another advantage is, we don't care about how the predicate internally works, we're just in charge of returning a predicate based on the input. This clears up the use case of the function a bit more.

Source Link
r3dst0rm
r3dst0rm
  • 406
  • 3
  • 7

As pointed out as a comment, I was unhappy about the extractCondition function. This function handles, variables which do conform your Predicate<T> type. Which is fine, but I think this doesn't belong here. Because a Predicate should have an accept or, if we talk about the Java implementation, test function. This is not given when we can pass on any random function which do return a boolean. But does not conform our design of a Predicate.

Next naming, the PredicateBuilder seems okay-ish, but it feels wrong. As it isn't a builder in the classical sense. Furthermore we have to work around functional interfaces, and this is I think the main pain point. To achieve this I would propose to rename the Predicate<T> interface, as it should be describing what a Predicate holds (a function which takes a parameter and returns a boolean). Therefore the name Predicate is available again and should be used instead of PredicateBuilder.

The repetition regarding and, or, etc... feels also some kind off. But I think that's because you always do extractCondition and other stuff (I've eliminated this in my suggestion, as the Java Predicate implementation does basically the same). Plus I've create a static method Predicate#of(...) to easier "get" a new Predicate. This reduces the mess a bit, but changing the operands is also still a bit clumsy, in my opinion.

Oh and btw. I've felt free to use arrow functions :-)

interface InternalPredicate<T> {
  (x: T) : boolean
}

class Predicate<T> {
  constructor(private condition: InternalPredicate<T>) {}

  public static of = <T>(condition: InternalPredicate<T>) => {
    return new Predicate(condition);
  }

  public and = (input: Predicate<T>): Predicate<T> =>
    Predicate.of((x: T) => this.apply(x) && input.apply(x));

  public or = (input: Predicate<T>): Predicate<T> =>
    Predicate.of((x: T) => this.apply(x) || input.apply(x));

  public not = (): Predicate<T> =>
    Predicate.of((x: T) => !this.apply(x));

  public apply = (x: T): boolean => this.condition(x);
}

Those are my two cents to the Predicate implementation, feel free to discuss them with me.