Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

I am following this article.

/**
 * The group name should contain a minimum of 3 and a maximum of 12 capital letters.
 * No space, number, or special characters are allowed
 */

console.clear();

class Group {
  constructor(name) {
    this.name = name;
  }

  getName() {
    return this.name;
  }

  getLength() {
    return this.name.length;
  }

  add(name) {}

  remove(name) {}

  toString() {
    return `Group: ${this.name}`;
  }
}

class DefaultGroup {
  constructor(group) {
    this.origin = group;
  }

  getName() {
    return this.origin.getName();
  }

  getLength() {
    return this.origin.getLength();
  }

  validate() {
    return true;
  }
}

class OnlyUpCaseGroup extends DefaultGroup {
  constructor(origin) {
    super(origin);

    if (!this.validate()) {
      alert('Invalid group name ' + this.getName());
      throw Error('Invalid group name');
    }
  }

  validate() {
    let REGEX_UP_CASE = /^[A-Z]*$/g;
    return super.validate() && REGEX_UP_CASE.test(this.getName());
  }

  toString() {
    return `Group: ${this.getName()}`;
  }
}

class ValidLengthGroup extends DefaultGroup {
  constructor(origin) {
    super(origin);

    if (!this.validate()) {
      alert('Length must be between 3 and 12');
      throw Error('Invalid group name');
    }
  }

  validate() {
    let len = this.getLength();
    return super.validate() && len >= 3 && len <= 12;
  }

  toString() {
    return `Group: ${this.getName()}`;
  }
}

let g = new Group('FOOOBAAAR');
//console.log(new OnlyUpCaseGroup(new Group('vivek')));
//console.log(new OnlyUpCaseGroup(new DefaultGroup(g)));
//console.log(new ValidLengthGroup(new DefaultGroup(g)));
console.log(new ValidLengthGroup(new OnlyUpCaseGroup(new DefaultGroup(g))));

Questions:

  1. I need to know if I am following the pattern properly?
  2. Can I do same with less verbose code?

Note:

Maintainability and flexibility is prime here. Keep in mind that there can be more validations.

share|improve this question
up vote 1 down vote accepted

Unfortunately does not have support for interfaces yet. What this means is that Group does not quite make sense and that DefaultGroup should be your Group instead. No matter how I look at it I don't really see any kind of usefulness on having both of those classes existing.

If we extrapolate the decorator pattern to one of the most common examples in C# present in Stream classes (StreamReader, BufferedStream, ...) you will clearly see that there is no DefaultStream.

It doesn't even exist a common interface. I am not saying that is the best and that is always what you should do. What you should do is to think about what makes sense on your language and in your requisites.


Keep going with this conversation you are doing your architecture. But did you reach to the conclusion that decorator pattern was really the best choice?

Could strategy pattern, together with composite pattern be a better alternative?

If I had to answer this question I would probably answer: Yes it would.

Let me explain why:

The decorator exists in order to add some kind of functionality that your class didn't have before. It's true that in your use case you are adding validation, a thing that you didn't have before. However what you really want to do is to have multiple kind of validations (strategies). You then could use all strategies you wanted by putting them on a list, iterating over them and call the validation method (this is the composite pattern)

Not to say that Group now becomes ignorant about which kind of validation is being made. Which might be a good thing or a bad thing. If you really want your group to be aware of validation it should take a parameter on the constructor to tell him how the validation should be made.


I could keep going by asking why you have hard-coded values for the minimum and maximum length (3 and 12 respectively). And also why do you have a getLength method that does getName().length?

In case you were wondering all of this that I said would translate in the following implementation

class Group {
  constructor(name) {
    this.name = name;
  }

  getName() {
    return this.name;
  }

  toString() {
    return `Group: ${this.name}`;
  }
}

class Validation {
  constructor() {
  }

  validate(value) {
    return true;
  }
}

class OnlyUpCaseValidaton extends Validation {
  constructor() {
    super();
  }

  validate(value) {
    let REGEX_UP_CASE = /^[A-Z]*$/g;
    return REGEX_UP_CASE.test(value);
  }
}

class LengthValidation extends Validation {
  constructor(minLength, maxLength) {
    super();
    this.minLength = minLength || 3;
    this.maxLength = maxLength || 12;
  }

  validate(value) {
    let len = value.length;
    return len >= this.minLength && len <= this.maxLength;
  }
}

let g = new Group('FOOOBAAAR');
//poor man's composite...
var validations = [new LengthValidation(), new OnlyUpCaseValidaton()];
console.log(validations.every(function(validation){
    return validation.validate(g.getName())
}));
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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