OutpostUniverse / OPHD

OutpostHD - Open source remake of Sierra On-Line's Outpost
BSD 3-Clause "New" or "Revised" License
110 stars 21 forks source link

`PopulationPanel` should take required arguments as constructor parameters #1442

Open DanRStevens opened 9 months ago

DanRStevens commented 9 months ago

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 commented 9 months ago

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):

DanRStevens commented 8 months ago

This issue relates somewhat to recently opened issue #1446.

ldicker83 commented 8 months ago

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.