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

split Guard class into two #3497

Closed
romani opened this Issue Oct 4, 2016 · 5 comments

Comments

Projects
None yet
2 participants
@romani
Member

romani commented Oct 4, 2016

https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/checks/imports/Guard.java

it serves two differend modes that are managed by null values in fields.

Task: create two classes from one to avoid checking for null. Names should be different "Guard" and named close to what they represent from XML.

@romani romani added the approved label Oct 4, 2016

@jochenvdv

This comment has been minimized.

Show comment
Hide comment
@jochenvdv

jochenvdv Oct 21, 2016

Contributor

Hi, I would like to tackle this issue as my first contribution to the checkstyle project as this seems relatively simple to begin with.

As I see it, there should be at least two different classes: a ClassGuard and a PkgGuard.
Should they both inherit from an abstract Guard class? The calculateResult method could be used by both.
The unit test would also have to be split in two.

If there is any other information I would need to implement this, please let me know.

Contributor

jochenvdv commented Oct 21, 2016

Hi, I would like to tackle this issue as my first contribution to the checkstyle project as this seems relatively simple to begin with.

As I see it, there should be at least two different classes: a ClassGuard and a PkgGuard.
Should they both inherit from an abstract Guard class? The calculateResult method could be used by both.
The unit test would also have to be split in two.

If there is any other information I would need to implement this, please let me know.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Oct 22, 2016

Member

Thanks for desire to help, you always welcome. Do not hesitate to ask questions.

Should they both inherit from an abstract Guard class?

Please propose better names. "Guard" is not good. It is better to less in common, overlap in one field is not enough to make an abstract class for them.

The unit test would also have to be split in two.

Yes

If there is any other information I would need to implement this, please let me know.

Please do PR , and we will see what to change. You can skip uts for first review

Member

romani commented Oct 22, 2016

Thanks for desire to help, you always welcome. Do not hesitate to ask questions.

Should they both inherit from an abstract Guard class?

Please propose better names. "Guard" is not good. It is better to less in common, overlap in one field is not enough to make an abstract class for them.

The unit test would also have to be split in two.

Yes

If there is any other information I would need to implement this, please let me know.

Please do PR , and we will see what to change. You can skip uts for first review

@jochenvdv

This comment has been minimized.

Show comment
Hide comment
@jochenvdv

jochenvdv Oct 22, 2016

Contributor

Please propose better names. "Guard" is not good.

What do you think of the names ClassImportRule and PkgImportRule? Or maybe ClassImportAccessRule and PkgImportAccessRule?

It is better to less in common, overlap in one field is not enough to make an abstract class for them.

Ok, but unless we create an abstract class or an interface we cannot operate on the general concept of a 'Guard' anymore from the PkgControl class.
(For example, we would have to provide two methods instead of one addGuard)

Right now all Guard objects are stored in a single Deque (LinkedList), do you want them to be stored and handled separately?. We could also store both of them in a Deque(Object) and then use instanceof and typecasting to work with them.

I think storing them in separate fields is better, we have to make two separate methods for adding them anyway.

Contributor

jochenvdv commented Oct 22, 2016

Please propose better names. "Guard" is not good.

What do you think of the names ClassImportRule and PkgImportRule? Or maybe ClassImportAccessRule and PkgImportAccessRule?

It is better to less in common, overlap in one field is not enough to make an abstract class for them.

Ok, but unless we create an abstract class or an interface we cannot operate on the general concept of a 'Guard' anymore from the PkgControl class.
(For example, we would have to provide two methods instead of one addGuard)

Right now all Guard objects are stored in a single Deque (LinkedList), do you want them to be stored and handled separately?. We could also store both of them in a Deque(Object) and then use instanceof and typecasting to work with them.

I think storing them in separate fields is better, we have to make two separate methods for adding them anyway.

jochenvdv added a commit to jochenvdv/checkstyle that referenced this issue Oct 22, 2016

jochenvdv added a commit to jochenvdv/checkstyle that referenced this issue Oct 22, 2016

@jochenvdv

This comment has been minimized.

Show comment
Hide comment
@jochenvdv

jochenvdv Oct 22, 2016

Contributor

I have started working on this.

  • GuardTest.java contained some tests for AccessResult, I move them into its own test file
  • I think PkgControl should also be renamed to ImportControl, some attributes/parameters should also be renamed to reflect that class names can also be worked with.
  • The unit tests for PkgControl only tests for packages, so I will maybe also add some tests for classes
  • Not using an abstract class will lead to unnecessary code duplication in PkgControl

I'm gonna do what I think is best for now. I will make a PR once references to Guard and PkgControl have been replaced, I can then still change things if needed.

Contributor

jochenvdv commented Oct 22, 2016

I have started working on this.

  • GuardTest.java contained some tests for AccessResult, I move them into its own test file
  • I think PkgControl should also be renamed to ImportControl, some attributes/parameters should also be renamed to reflect that class names can also be worked with.
  • The unit tests for PkgControl only tests for packages, so I will maybe also add some tests for classes
  • Not using an abstract class will lead to unnecessary code duplication in PkgControl

I'm gonna do what I think is best for now. I will make a PR once references to Guard and PkgControl have been replaced, I can then still change things if needed.

jochenvdv added a commit to jochenvdv/checkstyle that referenced this issue Oct 22, 2016

jochenvdv added a commit to jochenvdv/checkstyle that referenced this issue Oct 22, 2016

jochenvdv added a commit to jochenvdv/checkstyle that referenced this issue Oct 22, 2016

jochenvdv added a commit to jochenvdv/checkstyle that referenced this issue Oct 23, 2016

jochenvdv added a commit to jochenvdv/checkstyle that referenced this issue Oct 23, 2016

romani added a commit that referenced this issue Oct 23, 2016

@romani romani added this to the 7.2 milestone Oct 23, 2016

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Oct 23, 2016

Member

fix is mered.

Member

romani commented Oct 23, 2016

fix is mered.

@romani romani closed this Oct 23, 2016

agcuda added a commit to agcuda/checkstyle that referenced this issue Oct 30, 2016

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