3
\$\begingroup\$

A situation has come up in which we have a block of code that will be repeated in multiple files, and this block of code has to:

  1. Create some object based on the values of other controls/properties.
  2. Set a property on another object to be the object from #1.

Here is an example of what this looks like:


Repeated code before refactoring (TypeScript)

public class PageViewModel {
  private _name: TextBox;
  private _type: TextBox;
  private _timestampFormat: RadioButton;

  ..

  // This should generate a data model out of the current state of its controls.
  public generateDataModel(): IDataModel {
    // Here will be some logic to generate a property value based on
    // the radio button value and some other conditions.
    let timestampProperty;
    if (this._radioControl.value() === 1) {
      timestampProperty = { format: "foo" };
    } else if(this._radioControl.value() === 2) {
      timestampProperty = { format: "bar" };
    }

    var dataModelObject = <IDataModel>{
      name: this._name.value(),
      type: this._type.value(),
      timestamp: timestampProperty
    };
    return dataModelObject;
  }

}

Problem

The generateDataModel function will be present on more than one view model, all of which will generate the timestamp field from the radio control with that logic, so code will be repeated.


How it was solved

A helper method was extracted to do both #1 and #2 from above. This method is being called with references to both the control and the IDataModel object being created.

Helpers.ts:

public static setTimestampRadioButton(radioControl: RadioButton, dataModelObject: IDataModel): void {
    let timestampProperty;
    if (radioControl.value() === 1) {
      timestampProperty = { format: "foo" };
    } else if(radioControl.value() === 2) {
      timestampProperty = { format: "bar" };
    }

    dataModelObject.timestamp = timestampProperty;
}

PageViewModel.ts:

public generateDataModel(): IDataModel {
  var dataModelObject = <IDataModel>{
    name: this._name.value(),
    type: this._type.value()
  };

  Helpers.setTimestampRadioButton(this._radioControl, dataModelObject);

  return dataModelObject;
}

I have some concerns with this approach from the point of view of readability, and also because I think Helpers.ts knows too much about IDataModel and how it is used.

My questions:

  1. Is this a good way to solve the problem of repetition here? Why / why not?

    • If not, is there a better way you'd suggest?
  2. What do you think of the pattern static setSomethingOnSomethingElse(thing1, thing2)?

\$\endgroup\$
1
\$\begingroup\$

I would argue that setTimestampRadioButton can be improved in three ways.

  1. Side effects. By mutating the passed dataModelObject, the function immediately makes any function that uses setTimestampRadioButton harder to understand as it magically sets a property name that the calling function did not specify.
  2. Typing. Mutating the argument the function forces you to create a Partial<IDataModel> (or cast an object which is not an IDataModel to IDataModel). This is not ideal.
  3. Alternative property names. Currently only IDataModel uses the format from a RadioButton, what happens when ITimeModel needs the timestamp format stored in a format property? We are now back to the original problem.

I would instead recommend creating a method to get the expected format.

public static getTimestampFormat(radioControl: RadioButton): IFormat {
    if (radioControl.value() === 1) {
      return { format: "foo" };
    } else if(radioControl.value() === 2) {
      return { format: "bar" };
    }
    return { format: 
}

This simplifies your generateDataModel method even more.

public generateDataModel(): IDataModel {
  return {
    name: this._name.value(),
    type: this._type.value(),
    timestamp: Helpers.getTimestampFormat(this._radioControl)
  };
}

Instead of a static helper method, you might also consider adding the method as an instance method of the RadioButton class, or creating a child of the RadioButton class with a TimestampRadioButton class.

A couple other general things:

  • Avoid casting like the plague. It is almost as bad as using the any type as it hides potential problems in a very similar way.
  • I can't think of any instance in which adding Object or data to a variable name makes it more descriptive. dataModelObject doesn't really tell you any more than model.
\$\endgroup\$
  • \$\begingroup\$ Thanks @Gerrit0. To be clear, I changed the variable names in this example to abstract it away, but we don't use names like dataModelObject. Out of interest though, why is casting bad in this case? (I assume you're talking about the <IDataModel> cast before the object literal) \$\endgroup\$ – peter Sep 1 '17 at 16:53
  • \$\begingroup\$ Good question @peter, take a look at this playground example to see. Casting makes the compiler ignore type errors. \$\endgroup\$ – Gerrit0 Sep 1 '17 at 19:37
  • \$\begingroup\$ Thanks for the clarification - great point. I will keep this in mind. \$\endgroup\$ – peter Sep 2 '17 at 20:17

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.