Skip to content

[TASK] Drop redundant constructor code #932

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

Merged
merged 2 commits into from
Feb 16, 2025

Conversation

oliverklee
Copy link
Collaborator

  • use default values for properties instead of setting them in the constructor
  • drop constructors that are redundant to the constructor of the parent class

@coveralls
Copy link

coveralls commented Feb 15, 2025

Coverage Status

coverage: 50.453% (-0.05%) from 50.498%
when pulling b7989b0 on cleanup/drop-redundant-constructors
into e1fa3b6 on main.

Copy link
Collaborator

@JakeQZ JakeQZ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only had a brief look at this. Initializing properties to null is not necessary since that is the default, and by convention this is not normally explicitly done.

I found https://stackoverflow.com/questions/13114602/are-parent-constructors-called-if-a-child-class-does-not-define-a-constructor#13114603 which confirms constructors can be removed provided the base class defines one.

- use default values for properties instead of setting them in
  the constructor
- drop constructors that are redundant to the constructor of the
  parent class
@oliverklee oliverklee force-pushed the cleanup/drop-redundant-constructors branch from bdf04f4 to b7989b0 Compare February 15, 2025 12:18
@oliverklee
Copy link
Collaborator Author

Done and repushed.

@oliverklee oliverklee requested a review from JakeQZ February 15, 2025 12:18
Copy link
Collaborator

@JakeQZ JakeQZ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this works now.

IIRC, the PHP language issue with undefined constructors is when there is a base class with no constructor defined:

  • the derived class cannot call parent::__construct();
  • if the base class is later modified to add a constructor to perform an important initialization task, it won't be implicitly called;
  • to mitigate against this, the base class should have an empty constructor from the outset, so that the derived class can call it, should it later need to do something.
  • (C++ does not have this problem because default constructors are always implicitly called.)

The issue doesn't apply here, since all the removed constructors are in derived classes for which there is a already a constructor in the base class.

@JakeQZ JakeQZ merged commit 15f7a93 into main Feb 16, 2025
21 checks passed
@JakeQZ JakeQZ deleted the cleanup/drop-redundant-constructors branch February 16, 2025 02:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants