-
Notifications
You must be signed in to change notification settings - Fork 21
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
Comments
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 Remaining steps (suggested commits):
|
This issue relates somewhat to recently opened issue #1446. |
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. |
In regards to the following code:
When there are required parameters to an object, such as
Population*
andPopulationPool*
, 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 nevernullptr
, 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 markedconst
. Potentially the change isn't as simple as I think.Target design:
The text was updated successfully, but these errors were encountered: