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

PopulationPanel should take required arguments as constructor parameters #1442

Open
DanRStevens opened this issue Feb 7, 2024 · 3 comments

Comments

@DanRStevens
Copy link
Member

In regards to the following code:

	PopulationPanel();

	void population(Population* pop);
	void populationPool(PopulationPool* popPool);

When there are required parameters to an object, such as Population* and PopulationPool*, they should usually be provided as constructor parameters, rather than a separate setter. In particular, much of the code in the class assumes these values are never nullptr, which is their default values. It would make sense to remove their default values and force them to be initialized during object construction.

Actually, if the two fields are being initialized in the constructor, it may also make sense to convert them to reference syntax. A reference would need to be initialized by a constructor init list, so by moving initialization there, it makes it possible to use references. In addition to nicer access syntax, references are generally considered non-nullable, so that would also be consistent with assumptions made in the code.

Seems like the values should also be marked as const, since this should only be for reading and display purposes. Though I wonder if perhaps they need to make use of a member function that hasn't been marked const. Potentially the change isn't as simple as I think.


Target design:

PopulationPanel(const Population& pop, const PopulationPool& popPool);
@DanRStevens
Copy link
Member Author

DanRStevens commented Feb 25, 2024

It's been a few weeks, so I thought maybe I'd lookup some of the comments on Discord and summarize them here for reference.


PR #1444 dealt with marking the needed support functions in PopulationPool as const.


Remaining steps (suggested commits):

  • Add optional constructor parameters, with default values of nullptr. This can be done while keeping a single constructor. Keep pointer syntax for now.
    • PopulationPanel(Population* pop = nullptr, PopulationPool* popPool=nullptr);
  • Update the calling code to use the new constructor arguments.
  • Remove the default nullptr values, and the setter methods (they shouldn't be used at this point).
    • PopulationPanel(Population* pop, PopulationPool* popPool);
  • Convert to reference syntax, including the member field, the constructor parameters, and update the constructor call(s).
    • PopulationPanel(Population& pop, PopulationPool& popPool);
  • Mark the member fields and constructor parameters as const (optionally this could possibly be done first)
    • PopulationPanel(const Population& pop, const PopulationPool& popPool);

@DanRStevens
Copy link
Member Author

This issue relates somewhat to recently opened issue #1446.

@ldicker83
Copy link
Collaborator

In agreement here, that's a pattern I've recently been starting to change by deleting default constructors unless truly needed and opting for c'tor's with required parameters instead of setters.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants