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

Refactor TypeSpecifier::specifyTypesInCondition() for better readability #3705

Closed
wants to merge 1 commit into from

Conversation

thg2k
Copy link
Contributor

@thg2k thg2k commented Dec 3, 2024

This breaks the long if/elseif chain into separate ifs, as almost every case ends with return.

I believe this is a great improvement in code readability. Please let me know if you accept this so I can prepare the PR with the merged 2.0.x beforehand.

@ondrejmirtes
Copy link
Member

I'm sorry, I don't think this is much of an improvement. There are several classes in PHPStan that need to handle all expression types in PHPStan:

  • NodeScopeResolver (6k+ LoC)
  • MutatingScope (almost 6k LoC)
  • TypeSpecifier (almost 2.5k LoC)
  • InitializerExprTypeResolver (2k+ LoC)

I'd like to refactor them so that all code related for example to Coalesce is in a single class in 3-4 different methods presumably. That would allow to break these into many smaller classes. But of course that's a much bigger project. This PR does not get any close to that.

@thg2k
Copy link
Contributor Author

thg2k commented Dec 9, 2024

Well my follow up idea was to replace all ifs() with methods named like resolveExpr_Coalesce() and invoke them using the name of the expression class, much like the pretty printer class from php-parser, from there it would be easier to go towards your idea of a class-per-expression instead of method-per-expression, but it seems a bit hard to go there in one step. The idea of this PR was to get there in much smaller steps.. and taken by itself it's already an improvement to break the if/elseifs chain.

About the other three classes you mentioned, I had similar PRs ready, but they are based on the 1.11 as I experimented with it a while ago. I submitted only this one to check the reaction before investing the time to rebase them.

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