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

WIP refactor: code inspection - fix redundant local variable #2377

Closed
wants to merge 7 commits into from
Closed

WIP refactor: code inspection - fix redundant local variable #2377

wants to merge 7 commits into from

Conversation

zielint0
Copy link
Contributor

@zielint0 zielint0 commented Aug 15, 2018

WIP
DO NOT MERGE

@@ -85,8 +85,7 @@ public void accept(CtReference t) {
@Override
public ScanningMode enter(CtElement element) {
if (ignoredParent != null && element instanceof CtElement) {
CtElement ele = (CtElement) element;
if (ele.hasParent(ignoredParent)) {
if (((CtElement) element).hasParent(ignoredParent)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I really don't agree with this change: generally I'm the one extracting those casts in a local variable. I find it really more readable, and it's really useful when debugging.

Now here I even don't understand why a cast is needed: element is already a CtElement, so we just need to remove the cast, don't we?

Copy link
Contributor Author

@zielint0 zielint0 Aug 19, 2018

Choose a reason for hiding this comment

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

@surli

I really don't agree with this change: generally I'm the one extracting those casts in a local variable. I find it really more readable, and it's really useful when debugging.

Restored.

Now here I even don't understand why a cast is needed: element is already a CtElement, so we just need to remove the cast, don't we?

We do :-)

BTW. There are many redundant casts. I will take a look and try to fix them in separate PR.

Set<ModifierKind> backup = EnumSet.noneOf(ModifierKind.class);
backup.addAll(workCopy.getModifiers());
workCopy.removeModifier(ModifierKind.PUBLIC);
backup.addAll(c.getModifiers());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok with the remove of workCopy but then could you rename the argument of the method: c is really not something explicit... workType maybe or whatever.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@surli
Renamed.

if (ignoredParent != null && element instanceof CtElement) {
CtElement ele = (CtElement) element;
if (ele.hasParent(ignoredParent)) {
CtElement ele = element;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't understand the purpose of local variable ele here: IMHO you can just remove this variable and the first if.

Copy link
Contributor Author

@zielint0 zielint0 Aug 20, 2018

Choose a reason for hiding this comment

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

@surli
Done. Make sure 8094440 is what you meant.

@zielint0 zielint0 changed the title refactor: code inspection - fix redundant local variable WIP refactor: code inspection - fix redundant local variable Aug 23, 2018
@zielint0
Copy link
Contributor Author

WIP
DO NOT MERGE

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