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

Chore 2733 design level final refactor #2738

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

snehit221
Copy link
Contributor

@snehit221 snehit221 commented Nov 18, 2023

Checklist
Description of change

@snehit221
Copy link
Contributor Author

Hi @ChristianMurphy ,
May I request your review on this PR as well please?
Thanks

Comment on lines +227 to +244
this.authorizationService.canPrincipalBrowse(
this.authorizationPrincipal, portletDefinition);
break;
case CONFIGURE:
given(
this.authorizationService.canPrincipalConfigure(
this.authorizationPrincipal, portletDefinitionStringId))
.willReturn(true);
this.authorizationService.canPrincipalConfigure(
this.authorizationPrincipal, portletDefinitionStringId);
break;
case MANAGE:
given(
this.authorizationService.canPrincipalManage(
this.authorizationPrincipal, portletDefinitionStringId))
.willReturn(true);
this.authorizationService.canPrincipalManage(
this.authorizationPrincipal, portletDefinitionStringId);
break;
case SUBSCRIBE:
given(
this.authorizationService.canPrincipalSubscribe(
this.authorizationPrincipal, portletDefinitionStringId))
.willReturn(true);
this.authorizationService.canPrincipalSubscribe(
this.authorizationPrincipal, portletDefinitionStringId);
break;
case RENDER:
given(
this.authorizationService.canPrincipalRender(
this.authorizationPrincipal, portletDefinitionStringId))
.willReturn(true);
this.authorizationService.canPrincipalRender(
this.authorizationPrincipal, portletDefinitionStringId);
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't these tests assert something?
Why has the testing part of the test been removed?

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 was previously handled in a different way as this same switch case with method calls was used in the class. Now since that is now handled and replaced with a single polymorphic call, I did not need this any more in my test. Hence, I modified the existing flow to test whether the my polymorphism methods are being invoked properly.

Comment on lines -38 to -60
@Override
public int hashCode() {
int h = hash;
if (h == 0) {
h = internalHashCode();
hash = h;
}
return h;
}

private int internalHashCode() {
final int prime = 31;
int result = 1;
result =
prime * result
+ ((this.getPortletWindowId() == null)
? 0
: this.getPortletWindowId().hashCode());
result =
prime * result
+ ((this.getEventType() == null) ? 0 : this.getEventType().hashCode());
return result;
}
Copy link
Member

Choose a reason for hiding this comment

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

What has this been replaced by?
Is this inherited? Is there a lombok annotations I'm not able to see in the diff? Something else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All such implementations have been pulled up in the parent class to remove code duplicates.
Since all such methods were common to the 4/5 inheritors, I have applied pulled up refactor of the method and variable.

I can explore how I can replace the single implementation of this method ( now in the parent class) through Lombok in the next iteration.

Comment on lines -38 to -60
@Override
public int hashCode() {
int h = hash;
if (h == 0) {
h = internalHashCode();
hash = h;
}
return h;
}

private int internalHashCode() {
final int prime = 31;
int result = 1;
result =
prime * result
+ ((this.getPortletWindowId() == null)
? 0
: this.getPortletWindowId().hashCode());
result =
prime * result
+ ((this.getEventType() == null) ? 0 : this.getEventType().hashCode());
return result;
}
Copy link
Member

Choose a reason for hiding this comment

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

same here, where has this implementation been moved to?

Comment on lines +37 to +44
public int hashCode() {
int h = hash;
if (h == 0) {
h = internalHashCode();
hash = h;
}
return h;
}
Copy link
Member

Choose a reason for hiding this comment

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

Could this be handled through Lombok?

Comment on lines +60 to +67
@Override
public String toString() {
return this.getClass().getSimpleName()
+ " ["
+ "portletWindowId="
+ this.getPortletWindowId()
+ "]";
}
Copy link
Member

Choose a reason for hiding this comment

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

could this be handled through Lombok?

Copy link
Member

@bjagg bjagg left a comment

Choose a reason for hiding this comment

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

Looks like there are two refactors going on in this PR. One is pulling up hashCode() into a parent class. That seems reasonable given that these hashCode() methods seem to be special, along with the toString() method. The other refactor is to replace a switch statement with handlers; however, the switch statement just moves into another class with more code to create the classes. I don't see the benefit here.

I request that this be converted into two separate PRs -- one for the hashCode and toString methods and another for the switch changes. The former is likely to be merged, but the switch changes are borderline.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants