Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MeanShareAndBalanceModel should be an interface #45

Closed
pixelzoom opened this issue Jun 28, 2022 · 3 comments
Closed

MeanShareAndBalanceModel should be an interface #45

pixelzoom opened this issue Jun 28, 2022 · 3 comments
Assignees

Comments

@pixelzoom
Copy link
Contributor

pixelzoom commented Jun 28, 2022

For code review #41 ...

MeanShareAndBalanceModel looks like this:

export default class MeanShareAndBalanceModel {

  public constructor( providedOptions: MeanShareAndBalanceModelOptions ) {
    // Here for potential future use.
  }

  public syncData(): void {
    // See subclass for implementation
  }

  /**
   * Resets the model.
   */
  public reset(): void {
    // See subclass for implementation
  }

  /**
   * Steps the model.
   * @param dt - time step, in seconds
   */
  public step( dt: number ): void {
    // See subclass for implementation
  }
}

MeanShareAndBalanceModel is not meant to be instantiated directly. It provides no properties, functionality, or default behavior. And (based on what I see in IntroModel) you're expecting the subclasses to override all of those empty methods. So MeanShareAndBalanceModel should be an interface.

As an interace, it would look like this:

interface MeanShareAndBalanceModel {

  syncData(): void;

  /**
   * Resets the model.
   */
  reset(): void;

  /**
   * Steps the model.
   * @param dt - time step, in seconds
   */
  step( dt: number ): void;
}

export default MeanShareAndBalanceModel;

And usages would look like this:

export default class IntroModel implements MeanShareAndBalanceModel {

Interfaces are sometime named with an "I" prefix, like axon's IProperty. But PhET does not have a clear convention on this, so you decide whether it should be MeanShareAndBalanceModel or IMeanShareAndBalanceModel.

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Jun 28, 2022

By the way... If MeanShareAndBalanceModel had properties or provided default behavior, then I'd be recommending that you make it an abstract class. And while that's not the case now, it might become the case as you add screens to this sim.

@marlitas
Copy link
Contributor

marlitas commented Jul 1, 2022

@samreid suggested marking MeanShareAndBalanceModel as an abstract class for now as future screens get developed. We do anticipate adding methods or properties so will make abstract for the moment.

@marlitas
Copy link
Contributor

marlitas commented Jul 1, 2022

MeanShareAndBalanceModel is now an abstract class. Closing.

@marlitas marlitas closed this as completed Jul 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants