RFR: 1256: Use stream api to simplify the csr issue lookup [v2]

Erik Joelsson erikj at openjdk.java.net
Tue Nov 30 23:24:52 UTC 2021


On Tue, 30 Nov 2021 16:07:10 GMT, Guoxiong Li <gli at openjdk.org> wrote:

>> Hi all,
>> 
>> This patch adds the common methods `Issue#linksWithRelationships`, `Issue#csrLink` and `Issue#csrIssue` and refactors the code to reduce the loop nested statements by using these common methods.
>> 
>> All the existing tests passed.
>> 
>> Best Regards,
>> -- Guoxiong
>
> Guoxiong Li has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Use stream api instead of the encapsulated method.

I think this kind of refactoring is good enough for now. I still think the Issue interface should get refactored along the lines I described at some point, so that concepts like different types of links can be handled by a single appropriate class/interface. The clash with the other "Issue" class which represents something completely different is very unfortunate as well.

bots/pr/src/main/java/org/openjdk/skara/bots/pr/CSRCommand.java line 165:

> 163:         }
> 164: 
> 165:         var csr = jbsIssue.get().links().stream()

This particular pattern now occurs twice in this file. Perhaps worth moving to a private method at least, to reduce some duplication.

bots/pr/src/main/java/org/openjdk/skara/bots/pr/CheckRun.java line 142:

> 140:                 .filter(link -> link.relationship().isPresent() && "csr for".equals(link.relationship().get()))
> 141:                 .findAny().flatMap(Link::issue).orElse(null);
> 142:         if (csr == null) {

I just noticed this pattern being used a lot. Instead of .orElse(null), I would prefer if you let `csr` be an optional and used `csr.isPresent()` in the if statement. You will then need to use `csr.get()` to actually get the value, but this is safe as it's protected by the condition on isPresent(). If the variable in question is used a lot, you can choose resolve it with .get() into a new variable at the top of the if/else block.

This applies to several places where the same pattern is used.

-------------

PR: https://git.openjdk.java.net/skara/pull/1248


More information about the skara-dev mailing list