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

Should ProtectedMethodInFinalClass really exist? #901

Closed
victornoel opened this issue May 19, 2018 · 18 comments
Closed

Should ProtectedMethodInFinalClass really exist? #901

victornoel opened this issue May 19, 2018 · 18 comments
Assignees

Comments

@victornoel
Copy link

When implementing a final class that extends an abstract class with protected methods, it feels very wrong to me to mark the implementations of those methods as being public.

But with the rule ProtectedMethodInFinalClassCheck, I am forced to make them public.
This exposes undesired internal behaviour to the outside and thus seem a bad idea.

@0crat
Copy link
Collaborator

0crat commented May 19, 2018

@krzyk/z please, pay attention to this issue

@0crat
Copy link
Collaborator

0crat commented May 19, 2018

@victornoel/z this project will fix the problem faster if you donate a few dollars to it; just click here and pay via Stripe, it's very fast, convenient and appreciated; thanks a lot!

@krzyk
Copy link
Collaborator

krzyk commented Jul 28, 2018

@victornoel why would you use protected in such class? Doesn't default also work for the cases that you need?
protected adds only gives subclasses visibility (which is pointless in final class) in comparison to the default scope:
image

@victornoel
Copy link
Author

@krzyk see the following example:

public abstract class AbstractClass {
    protected abstract void method();
}

// here qulice complains with ProtectedMethodInFinalClassCheck
public final class FinalClass extends AbstractClass {
    @Override
    protected void method() {
        // implem
    }
}

// here javac complains with "Cannot reduce the visibility of the inherited method from AbstractClass"
public final class FinalClass extends AbstractClass {
    @Override
    void method() {
        // implem
    }
}

// here we made a method not meant to be exposed public
public final class FinalClass extends AbstractClass {
    @Override
    public void method() {
        // implem
    }
}

To answer you question: default does not work in that case, so I have to make it public, which I think is worst design than having a protected method in a final class (or it should only be for non-overridden methods).

@krzyk
Copy link
Collaborator

krzyk commented Jul 28, 2018

@victornoel Oh, in that case it should be excluded in case of @Override

@krzyk
Copy link
Collaborator

krzyk commented Jul 28, 2018

@0crat in

@krzyk
Copy link
Collaborator

krzyk commented Jul 28, 2018

@0crat assign me

@0crat 0crat added the scope label Jul 28, 2018
@0crat
Copy link
Collaborator

0crat commented Jul 28, 2018

@0crat in (here)

@krzyk Job #901 is now in scope, role is DEV

@0crat
Copy link
Collaborator

0crat commented Jul 28, 2018

@0crat assign me (here)

@krzyk The job #901 assigned to @krzyk/z, here is why; the budget is 30 minutes, see §4; please, read §8 and §9; if the task is not clear, read this and this; there will be no monetary reward for this job

@0crat
Copy link
Collaborator

0crat commented Jul 28, 2018

Bug was reported, see §29: +15 point(s) just awarded to @victornoel/z

@0crat
Copy link
Collaborator

0crat commented Jul 28, 2018

Manual assignment of issues is discouraged, see §19: -5 point(s) just awarded to @krzyk/z

@krzyk
Copy link
Collaborator

krzyk commented Jul 28, 2018

@victornoel fixed in #915, it will be available with next qulice release. Please close this issue.

@victornoel
Copy link
Author

@krzyk don't you think the rule should be stricter and only allow it in case there is an override AND the superclass method is protected?

@krzyk
Copy link
Collaborator

krzyk commented Jul 28, 2018

@victornoel but this will be quite a narrow case when someone widens the scope from default to protected, and it has to be in a class that has visibility of that default method (so in the same package). Please create a new bug for this (we will probably need to convert this to PMD check in that case).

@victornoel
Copy link
Author

@krzyk yep, I understand, this issue is fixed anyway :)

@0crat
Copy link
Collaborator

0crat commented Jul 28, 2018

Job was finished in 3 hours, bonus for fast delivery is possible (see §36)

@0crat 0crat removed the scope label Jul 28, 2018
@0crat
Copy link
Collaborator

0crat commented Jul 28, 2018

Order was finished: +35 point(s) just awarded to @krzyk/z

@0crat
Copy link
Collaborator

0crat commented Jul 28, 2018

The job #901 is now out of scope

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

No branches or pull requests

3 participants