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

Tidy up util.php #38

Merged
merged 5 commits into from
Feb 20, 2018
Merged

Tidy up util.php #38

merged 5 commits into from
Feb 20, 2018

Conversation

janopae
Copy link
Member

@janopae janopae commented Feb 15, 2018

util.php only contains unused functions + the connect function, which is only used exactly one time in the SlimdumpCommand.php file, and I think, this is where the function belongs.

@coveralls
Copy link

coveralls commented Feb 15, 2018

Coverage Status

Coverage decreased (-2.3%) to 67.647% when pulling f227b13 on TidyUpUtil.php into ecacd6f on master.

@mpdude
Copy link
Member

mpdude commented Feb 15, 2018

Seems reasonable. Are you sure that the other functions are really unused? If so, I'm fine with this.

@MalteWunsch
Copy link
Member

From an object oriented point of view, I don't think the command should have the responsibility to create a database connection. For me, the command is merely the bridge between the console and the start of the application. Creating a database connection seems to be some other infrastructure problem. (Looking into the connect method, it does not seem to be very worthy anyway.) I think the dump method should be extracted to a task class as well.

If you think the advantage of getting rid of the util.php outweighs this problem, I'd suggest to move the method into the lines below the other protected methods, because a) this follows the execution and reading flow (read the class like a new article), b) the other protected methods are overwriting their heritage and c) the connect method should be private.

@janopae
Copy link
Member Author

janopae commented Feb 15, 2018

Being already home of the dump()-function, SlimdumpCommand.php seemed to be the best place for connect in the current structure of the application, at least it's much better than having it in a separate file outside the src-directory inside the public namespace, where it has been originally.

Putting all functions that really do something into another class looks like a good idea, but practically, I don't see the need for that at the moment, as the SlimdumpCommand-class is easy to read and works well.

@MalteWunsch
Copy link
Member

220px-mestlin_kulturhaus_zerbrochenes_fenster_2012-09-05_069

@polarbirke
Copy link
Member

@janopae
Copy link
Member Author

janopae commented Feb 15, 2018

Okay, yeah, you convinced me, we should definitively do that, but we should do it in a separate pull request.

@janopae janopae merged commit 479c290 into master Feb 20, 2018
@mpdude mpdude deleted the TidyUpUtil.php branch June 27, 2018 20:45
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

Successfully merging this pull request may close these issues.

None yet

5 participants