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

Epo 7766 #24

Merged
merged 22 commits into from
Mar 22, 2023
Merged

Epo 7766 #24

merged 22 commits into from
Mar 22, 2023

Conversation

ericdrosas87
Copy link
Contributor

No description provided.

@ericdrosas87 ericdrosas87 requested a review from khalwat March 10, 2023 16:39
@ericdrosas87
Copy link
Contributor Author

I did not refactor much of the Asset and Element service classes, as they likely will be going away, nor did I refactor out much of the logic from the controller as it will likely change quite a bit.

@ericdrosas87
Copy link
Contributor Author

I also didn't both to change the name of the custom table from "universal*" to "canto*" since this table will be going away.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Usually I add this to my project or system-wide .gitignore:

.idea

...unless you really want the PhpStorm meta attributes checked into your project.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense

composer.json Show resolved Hide resolved
composer.lock Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

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

For inherited methods, typically the block comment should just be:

/**
 * @inheritdoc
 */

Which tells the IDE/documentation parser to use the docs from the parent method

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a way to automatically tell PhpStorm to do this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

In general, you may find your code is more easily digestible if you use the "happy path" methodology:

https://lamp-dev.com/a-quick-guide-to-psr-coding-style-recommendations/1887#happy-path

Also sometimes called "early return", the idea is to reduce cognitive load by having your methods return early for exceptional conditions.

Similar to the section below this at the same URL in terms of avoiding if/else

Probably not worth a refactor for working code, just noting it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is an interesting comment to me - when working primarily with Java in a large enterprise environment we were always told to never "short circuit" a method/function and to only ever have a single return statement. Perhaps that's because of the monolithic methods/functions that exist in enterprise codebases?

Copy link
Collaborator

@khalwat khalwat left a comment

Choose a reason for hiding this comment

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

Overall looks good, as you said, mostly PhpStorm doing it's thing. I probably should have mentioned this ahead of time, but PhpStorm does help you automate moving namespaces:

https://www.jetbrains.com/help/phpstorm/move-refactorings.html#moving-php-namespace

Probably I can show you a few other tips to make using PhpStorm more efficient for you, but it looks like it's helping to clean things up already!

@ericdrosas87 ericdrosas87 merged commit e8ee05e into main Mar 22, 2023
@ericdrosas87 ericdrosas87 deleted the EPO-7766 branch April 12, 2023 20:42
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.

2 participants