2

I was looking at the angular documentation and noticed this code:

export class QuestionBase<T> {
  value: T;
  key: string;
  label: string;
  required: boolean;
  order: number;
  controlType: string;
  type: string;
  options: {key: string, value: string}[];

  constructor(options: {
      value?: T,
      key?: string,
      label?: string,
      required?: boolean,
      order?: number,
      controlType?: string,
      type?: string
    } = {}) {
    this.value = options.value;
    this.key = options.key || '';
    this.label = options.label || '';
    this.required = !!options.required;
    this.order = options.order === undefined ? 1 : options.order;
    this.controlType = options.controlType || '';
    this.type = options.type || '';
  }
}

They use this as the base class for different types of questions such as "DropdownQuestion", "TextboxQuestion", etc. I've copied the usage they included in the docs below:

export class DropdownQuestion extends QuestionBase<string> {
  controlType = 'dropdown';
  options: {key: string, value: string}[] = [];

  constructor(options: {} = {}) {
    super(options);
    this.options = options['options'] || [];
  }
}
 //in some other file...
  new DropdownQuestion({
    key: 'brave',
    label: 'Bravery Rating',
    options: [
      {key: 'solid',  value: 'Solid'},
      {key: 'great',  value: 'Great'},
      {key: 'good',   value: 'Good'},
      {key: 'unproven', value: 'Unproven'}
    ],
    order: 3
  }),

  new TextboxQuestion({
    key: 'firstName',
    label: 'First name',
    value: 'Bombasto',
    required: true,
    order: 1
  }),

  new TextboxQuestion({
    key: 'emailAddress',
    label: 'Email',
    type: 'email',
    order: 2
  })

As you can see, only the DropdownQuestion uses the "options" property, and has it's own property for "options", but they've included it in the base class. From what I can tell, it literally does nothing in the base class, because it's not used in the constructor, and it will never reach the prototype. What is the point of doing that? Shouldn't it just be included in the DropdownQuestion?

Slightly unrelated, but making the "key" and "label" properties optional when they seem to be required in all questions also seems like an odd thing to do.

Can anyone provide some clarification on what is ideal to do?

5

Having a property in the base class which is used in only one of several subclasses is a common antipattern. This can easily happen when creating the superclass, by either pulling too many things into the superclass when creating it or forgetting to pull the property into the new subclass (depending on how the initial refactoring happened).

Better naming might have made this flaw clearer. In this case, the "dropdown" part of DropdownQuestion is misleading: a question with options might be displayed as a single-select list (aka "dropdown"), but might just as easily be a multi-select or a set of radio buttons. If it were instead named (for example) SingleOptionQuestion that would make it obvious that the options property should be in this class.

| improve this answer | |
  • Such an informative response. Glad I wasn’t out of left field with my concerns – p32094 Jul 31 at 3:30
2

Including any public properties in base class is an anti pattern to begin with, or at least it's a code smell. In most cases, you should follow the tell don't ask principle, and the matters of property wouldn't even become an issue in the first place.

In this particular case, the options could simply be a matter of render(), getDefault() and validate() method, which should remove the need to actually have options to be exposed as a property.

| improve this answer | |
  • 1
    Typescript doesn't have real private variables, so not sure as to the applicability of this? – p32094 Jul 31 at 4:51
  • Properties on base classes have the same standing as those on concrete classes. If they're bad in base classes, they're bad in concrete classes; might as well not have them at all. – Robert Harvey Jul 31 at 4:53
  • @RobertHarvey curious what do you mean by "same standing"? – p32094 Jul 31 at 4:57
  • @p32094: it doesn't matter whether a language actually has private variables or not. Properties shouldn't be part of the public contract of a base class whether or not the language enforces them, and the base class should define methods that would allow 95% of its users to never need to access those properties. The 5% code that ignores that and touches the properties are the "consenting adults" anyway, so they should know what they're getting into. – Lie Ryan Jul 31 at 5:00
  • 1
    @p32094 the same general principle of designing classes so that direct use of properties unnecessary still apply for when you're using concrete classes directly, but the problems caused by directly accessing properties from concrete classes are usually much less severe than those that are defined by a base class/interface shared by multiple concrete classes. So it's still a "consenting adults" situation, but use your best judgement on those. – Lie Ryan Jul 31 at 5:49

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.