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.

Sign up for GitHub

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

Jump to bottom

Convert the abstract class DAOs to interfaces. #5462

Merged
merged 1 commit into from
Jun 8, 2021
Merged

Convert the abstract class DAOs to interfaces. #5462

merged 1 commit into from
Jun 8, 2021

Conversation

Isira-Seneviratne
Copy link
Member

What is it?

  • Bugfix (user facing)
  • Feature (user facing)
  • Codebase improvement (dev facing)
  • Meta improvement to the project (dev facing)

Description of the changes in your PR

  • Convert the Java abstract class DAOs to interfaces.

Due diligence

@vkay94
Copy link
Contributor

vkay94 commented Jan 20, 2021

Just of curiosity: I know that since NP uses Java 8 now and therefore because of the default feature the conversion is possible now (without side-effects I think). Are there benefits of switching from abstract classes to interfaces (except of getting rid of public abstract)?

@XiangRongLin XiangRongLin added the codequality Improvements to the codebase to improve the code quality label Jan 20, 2021
@Isira-Seneviratne
Copy link
Member Author

Just of curiosity: I know that since NP uses Java 8 now and therefore because of the default feature the conversion is possible now (without side-effects I think). Are there benefits of switching from abstract classes to interfaces (except of getting rid of public abstract)?

Aside from the change you mentioned, no.

@TobiGr TobiGr force-pushed the dev branch 2 times, most recently from 679bc75 to 2aeccc0 Compare March 16, 2021 08:24
triallax
triallax reviewed May 7, 2021
View reviewed changes
app/src/main/java/org/schabi/newpipe/database/stream/dao/StreamStateDAO.java

@Insert(onConflict = OnConflictStrategy.IGNORE)
abstract void silentInsertInternal(StreamStateEntity streamState);
void silentInsertInternal(StreamStateEntity streamState);
Copy link
Contributor

Choose a reason for hiding this comment

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

The only issue I see with this change is that "internal" methods like this one have to be public (all of an interface's members can be only public).

Copy link
Member Author

Choose a reason for hiding this comment

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

Private interface methods were introduced in Java 9.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Isira-Seneviratne indeed, you're right; I was not aware of that. However, we're using Java 8 here, so we still can't do that.

@Stypox
Copy link
Member

Stypox commented Jun 8, 2021

@TobiGr should this be merged or not?

@TobiGr
Copy link
Member

TobiGr commented Jun 8, 2021

I think it is a reasonable change (in other word: yes)

@Stypox Stypox merged commit 5e2735a into TeamNewPipe:dev Jun 8, 2021
This was referenced Jun 26, 2021
Merged
Closed
@Isira-Seneviratne Isira-Seneviratne deleted the Convert_abstract_classes_to_interfaces branch July 10, 2021 09:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@triallax triallax triallax approved these changes

@Stypox Stypox Stypox approved these changes

Assignees
No one assigned
Labels
codequality Improvements to the codebase to improve the code quality
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Isira-Seneviratne @vkay94 @Stypox @TobiGr @triallax @XiangRongLin