[jbosstools-issues] [JBoss JIRA] (JBIDE-21149) Improve implementation of EGitUtils.getDefaultRemoteRepo()

[Viacheslav Kabanovich (JIRA)]

     [ https://issues.jboss.org/browse/JBIDE-21149?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Viacheslav Kabanovich updated JBIDE-21149:
------------------------------------------
    Description: 
When a class provides for some collection property methods "get all", "get first", "has some" it is not a good pattern to 'reuse' "get all" to implement the other methods. For large collections that pattern ends up with bad performance. For small collections the performance is not critical, but it is a good idea to keep to right patterns to avoid an accidental applying a wrong one to a critical case.

Method EGitUtils.getRemoteGitRepos(IProject) uses advanced API of Java 1.8 for handling collections, but EGitUtils.getDefaultRemoteRepo() just reuses it to get the first element of the list. Java 1.8 Stream operations allow preparing query that is processed on applying a consumer operation. So, if "get first" and "get all" may use the same stream query, we can privately prepare it to be reused in "get first" with Stream.findFirst() and in "get all" with Stream.collect(). [~adietish] please take a look at the attached pull request.

Method getRemoteGitRepos(IProject) is now used just once in TemplateListPageValidator.validate() only to check that the list is not empty. So it could instead call getDefaultRemoteRepo() and check that it is not null. But while getDefaultRemoteRepo() is implemented with a call to getRemoteGitRepos(), it does not matter what method to call at that point. Thus, a wrong pattern in implementing API (EGitUtils) is followed by a wrong pattern in using it.

  was:
When a class provides for some collection property methods "get all", "get first", "has some" it is not a good pattern to 'reuse' "get all" to implement the other methods. For large collections that pattern ends up with bad performance. For small collections the performance is not critical, but it a good idea to keep to right patterns to avoid an accidental applying a wrong pattern to a critical case.

Method EGitUtils.getRemoteGitRepos(IProject) uses advanced API of Java 1.8 for handling collections, but EGitUtils.getDefaultRemoteRepo() just reuses it to get the first element of the list. Java 1.8 Stream operations allow preparing query that is processed on applying a consumer operation. So, if "get first" and "get all" may use the same stream query, we can privately prepare it to be reused in "get first" with Stream.findFirst() and in "get all" with Stream.collect(). [~adietish] please take a look at the attached pull request.

Method getRemoteGitRepos(IProject) is now used just once in TemplateListPageValidator.validate() only to check that the list is not empty. So it could instead call getDefaultRemoteRepo() and check that it is not null. But while getDefaultRemoteRepo() is implemented with a call to getRemoteGitRepos(), it does not matter what method to call at that point. Thus, a wrong pattern in implementing API (EGitUtils) is followed by a wrong pattern in using it.



> Improve implementation of EGitUtils.getDefaultRemoteRepo()
> ----------------------------------------------------------
>
>                 Key: JBIDE-21149
>                 URL: https://issues.jboss.org/browse/JBIDE-21149
>             Project: Tools (JBoss Tools)
>          Issue Type: Enhancement
>          Components: openshift
>    Affects Versions: 4.4.0.Alpha1
>            Reporter: Viacheslav Kabanovich
>            Assignee: Viacheslav Kabanovich
>              Labels: code_quality
>
> When a class provides for some collection property methods "get all", "get first", "has some" it is not a good pattern to 'reuse' "get all" to implement the other methods. For large collections that pattern ends up with bad performance. For small collections the performance is not critical, but it is a good idea to keep to right patterns to avoid an accidental applying a wrong one to a critical case.
> Method EGitUtils.getRemoteGitRepos(IProject) uses advanced API of Java 1.8 for handling collections, but EGitUtils.getDefaultRemoteRepo() just reuses it to get the first element of the list. Java 1.8 Stream operations allow preparing query that is processed on applying a consumer operation. So, if "get first" and "get all" may use the same stream query, we can privately prepare it to be reused in "get first" with Stream.findFirst() and in "get all" with Stream.collect(). [~adietish] please take a look at the attached pull request.
> Method getRemoteGitRepos(IProject) is now used just once in TemplateListPageValidator.validate() only to check that the list is not empty. So it could instead call getDefaultRemoteRepo() and check that it is not null. But while getDefaultRemoteRepo() is implemented with a call to getRemoteGitRepos(), it does not matter what method to call at that point. Thus, a wrong pattern in implementing API (EGitUtils) is followed by a wrong pattern in using it.



--
This message was sent by Atlassian JIRA
(v6.4.11#64026)