Skip to content

[Improvement][API] Simplify the Check of Result by introducing several new methods.#5666

Merged
CalvinKirs merged 11 commits intoapache:devfrom
echohlne:refactor_Result
Jun 21, 2021
Merged

[Improvement][API] Simplify the Check of Result by introducing several new methods.#5666
CalvinKirs merged 11 commits intoapache:devfrom
echohlne:refactor_Result

Conversation

@echohlne
Copy link
Contributor

Purpose of the pull request

Try to introduce several methods like below in Result:

    public boolean isSuccess() {
        return this.isStatus(Status.SUCCESS);
    }

    public boolean isFailed() {
        return !this.isSuccess();
    }

    public boolean isStatus(Status status) {
        return this.code != null && this.code.equals(status.getCode());
    }

The purpose of this is to reduce some tedious template code:

// can be replaced by isConnection.isFailed()
if (Status.SUCCESS.getCode() != isConnection.getCode()) {
  ....
}

// can be replaced by isConnection.isSuccess()
if (Status.SUCCESS.getCode() == result.getCode()) {
  ....
}

// can be replaced by Assert.assertTrue(result.isStatus(Status.CREATE_ACCESS_TOKEN_ERROR));
Assert.assertEquals(Status.CREATE_ACCESS_TOKEN_ERROR.getCode(), result.getCode().intValue());

The

Brief change log

No new features, using existing UTs.

@echohlne echohlne changed the title Refactor result [Improvement][API] Simplify the Check of Result by introducing several new methods. Jun 20, 2021
check code style has some errors.
@codecov-commenter
Copy link

codecov-commenter commented Jun 20, 2021

Codecov Report

Merging #5666 (7125c01) into dev (e2eed1f) will increase coverage by 0.05%.
The diff coverage is 56.52%.

Impacted file tree graph

@@             Coverage Diff              @@
##                dev    #5666      +/-   ##
============================================
+ Coverage     45.29%   45.34%   +0.05%     
- Complexity     3672     3681       +9     
============================================
  Files           607      607              
  Lines         24794    24797       +3     
  Branches       2803     2803              
============================================
+ Hits          11230    11245      +15     
+ Misses        12493    12479      -14     
- Partials       1071     1073       +2     
Impacted Files Coverage Δ
...lphinscheduler/api/controller/LoginController.java 4.54% <0.00%> (ø)
...eduler/api/service/impl/DataSourceServiceImpl.java 44.76% <0.00%> (ø)
...r/api/service/impl/ProcessInstanceServiceImpl.java 73.29% <0.00%> (ø)
...heduler/api/service/impl/ResourcesServiceImpl.java 52.19% <60.00%> (ø)
.../org/apache/dolphinscheduler/api/utils/Result.java 93.54% <80.00%> (-2.89%) ⬇️
...inscheduler/common/task/sqoop/SqoopParameters.java 74.00% <0.00%> (ø)
...er/master/dispatch/host/assign/RandomSelector.java 83.33% <0.00%> (+5.55%) ⬆️
...olphinscheduler/rpc/remote/NettyClientHandler.java 51.16% <0.00%> (+9.30%) ⬆️
...he/dolphinscheduler/common/enums/SqoopJobType.java 88.88% <0.00%> (+88.88%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e2eed1f...7125c01. Read the comment docs.

return new Result<>(data);
}

public boolean isSuccess() {
Copy link
Member

Choose a reason for hiding this comment

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

Hi, to be honest, I don't recommend modifying this class, this may be related to programming habits :).
I think this class should be kept as simple as possible, he shouldn't care about the status judgements.
Another point is that if we add more status judgements in the future, we need to modify this class again.
For example, if we add a new status of result called unknown, then we need to modify this class and add a new method, am I right?

But you are right, right now there is a lot of duplicate code in many placements, once we need to change the rule of status judgement, we need to modify many placements. I may prefer to use a new class to take care of the status judgement, rather than modified the current class.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks@ruanwenjun I'll close this pr.

Copy link
Member

Choose a reason for hiding this comment

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

@Kyoty This is my own opinion, you can open this pr and discuss with more people.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ruanwenjun thanks, what you said is very valuable, I will collect other's opinions to decide what to do next.

Copy link
Member

Choose a reason for hiding this comment

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

In my opinion, this is a public method, and I think it can be done so.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @CalvinKirs , do you think this pr makes sense? Or is there a better way to deal with it?

@echohlne echohlne closed this Jun 20, 2021
@echohlne echohlne reopened this Jun 20, 2021
@CalvinKirs CalvinKirs added the improvement make more easy to user or prompt friendly label Jun 21, 2021
Copy link
Member

@CalvinKirs CalvinKirs left a comment

Choose a reason for hiding this comment

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

LGTM

@CalvinKirs
Copy link
Member

please resolve the conflicts file.thx

@echohlne
Copy link
Contributor Author

@CalvinKirs solve it!

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

75.0% 75.0% Coverage
1.0% 1.0% Duplication

@CalvinKirs CalvinKirs merged commit e628e4e into apache:dev Jun 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

improvement make more easy to user or prompt friendly

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants