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?

p32094
  • 177
  • 1
  • 6

2 Answers2

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.

l0b0
  • 11,014
  • 2
  • 43
  • 47
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.

Lie Ryan
  • 12,291
  • 1
  • 30
  • 41
  • 1
    Typescript doesn't have real private variables, so not sure as to the applicability of this? – p32094 Jul 31 '20 at 04: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 '20 at 04:53
  • @RobertHarvey curious what do you mean by "same standing"? – p32094 Jul 31 '20 at 04: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 '20 at 05:00
  • @LieRyan what is the "public contract"? Meaning what is exposed to the sub class? And I understand the logic. In this case, it seems the documentation is using polymorphism to prevent the re-implementation of a standard "Question" interface in every class. – p32094 Jul 31 '20 at 05:02
  • Whatever you, as the designer of the basse class API, decides that you want to support for eternity for all subclasses. Generally, it's very hard or often nearly impossible to find a set of properties that will satisfy the needs of all subclasses while at the same time not having any excess properties that aren't used by some. That's why "Tell, don't ask" when designing a base class, as high level methods are a lot easier to define in a way that wouldn't cause inconsistencies or unused methods/attributes between the concrete implementations. – Lie Ryan Jul 31 '20 at 05:10
  • @LieRyan cool, thanks for the response and the resource on tell don't ask! For sub classes, would the same principle apply? – p32094 Jul 31 '20 at 05:14
  • 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 '20 at 05:49