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

[Improve] Improve streampark-console core package module service name base on [3.1 Naming Style] #3295

Merged
merged 5 commits into from
Nov 1, 2023

Conversation

VampireAchao
Copy link
Contributor

What changes were proposed in this pull request

Issue Number: see #3064

Brief change log

Improve streampark-console core package module service name base on [3.1 Naming Style]

  1. Just query method
  2. Just in service

Verifying this change

This change is a trivial rework / code cleanup without any test coverage.

Yes (or)

This change is already covered by existing tests, such as (please describe tests).

No (or)

This change added tests and can be verified as follows:

Does this pull request potentially affect one of the following parts

  • Dependencies (does it add or upgrade a dependency): No

@wolfboys
Copy link
Member

Thanks very much for your contribution. There are some check style issues need to be resolved first.
mvn spotless:apply

Copy link
Contributor

@RocMarshal RocMarshal left a comment

Choose a reason for hiding this comment

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

Thx for the contribution.
I left a few of comments. PTAL in your free time.

@VampireAchao
Copy link
Contributor Author

Great!I'll fix it ASAP.

@caicancai
Copy link
Member

cc @RocMarshal

@VampireAchao
Copy link
Contributor Author

cc  抄送@RocMarshal

Thx

@@ -27,7 +27,7 @@

public interface YarnQueueService extends IService<YarnQueue> {

IPage<YarnQueue> findYarnQueues(YarnQueue yarnQueue, RestRequest restRequest);
IPage<YarnQueue> getPage(YarnQueue yarnQueue, RestRequest restRequest);
Copy link
Member

Choose a reason for hiding this comment

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

getYarnQueuesPage?

Copy link
Contributor Author

@VampireAchao VampireAchao Nov 1, 2023

Choose a reason for hiding this comment

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

yarnQueueService.getPage already mentioned yarnQueue


boolean checkExists(Project project);
boolean exists(Project project);
Copy link
Member

Choose a reason for hiding this comment

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

I think checkExists better

Copy link
Contributor Author

@VampireAchao VampireAchao Nov 1, 2023

Choose a reason for hiding this comment

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

Difference:

void checkExists(Project project) {
    if (notExists) {
        throw new ProjectNotExistsException();
    }
}

As a result:

if (!exists(project)) {
    throw new ProjectNotExistsException();
}

Alternatively, it could be simplified as:

checkExist(project);

This approach is preferred since the "exists" method merely returns a boolean value, which enhances clarity and conciseness.

Copy link
Contributor

@RocMarshal RocMarshal left a comment

Choose a reason for hiding this comment

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

Thanks @VampireAchao for the improvement and @caicancai for the review.
LGTM +1~

@RocMarshal RocMarshal merged commit 57b9b78 into apache:dev Nov 1, 2023
6 checks passed
@VampireAchao VampireAchao changed the title [WIP] [Improve] Improve streampark-console core package module service name base on [3.1 Naming Style] [Improve] Improve streampark-console core package module service name base on [3.1 Naming Style] Nov 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants