opendev/gerrit
Code Issues Proposed changes

WebLinks: Return List<T> from methods rather than FluentIterable<T>

Browse Source 
All the callers of these methods convert the returned FluentIterable to
a List. Also it's inefficient to invoke FluentIterable.isEmpty() because
it has to traverse the whole thing including map and filter calls.

Change it to just return a List.

Rather than converting to use the Java 8 streams API, leave the internal
implementation of the methods unchanged for now. The links are DynamicSet,
which is an implementation of Iterable and does not have the stream()
method. There isn't much readability improvement by rewriting it to use
Spliterator, but we can revisit this later when we upgrade to Guava 21
which has a Streams utility.

Change-Id: I0e2a68f2992f6bcb2ab1ee2ae067975d416e9651
This commit is contained in:
David Pursehouse
2016-12-06 21:27:48 +09:00
parent 9b8be04d27
commit e32ef82ad7
8 changed files with 33 additions and 32 deletions
Download Patch File Download Diff File Expand all files Collapse all files

5
gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/project/ProjectAccessFactory.java
View File

@@ -14,7 +14,6 @@
package com.google.gerrit.httpd.rpc.project;
import com.google.common.collect.FluentIterable;
import com.google.common.collect.Maps;
import com.google.gerrit.common.data.AccessSection;
import com.google.gerrit.common.data.GroupDescription;
@@ -216,10 +215,10 @@ class ProjectAccessFactory extends Handler<ProjectAccess> {
}
private List<WebLinkInfoCommon> getConfigFileLogLinks(String projectName) {
FluentIterable<WebLinkInfoCommon> links =
List<WebLinkInfoCommon> links =
webLinks.getFileHistoryLinks(projectName, RefNames.REFS_CONFIG,
ProjectConfig.PROJECT_CONFIG);
return links.isEmpty() ? null : links.toList();
return links.isEmpty() ? null : links;
}
private Map<AccountGroup.UUID, GroupInfo> buildGroupInfo(List<AccessSection> local) {

25
gerrit-server/src/main/java/com/google/gerrit/server/WebLinks.java
View File

@@ -36,6 +36,8 @@ import com.google.inject.Singleton;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import java.util.List;
@Singleton
public class WebLinks {
private static final Logger log = LoggerFactory.getLogger(WebLinks.class);
@@ -94,7 +96,7 @@ public class WebLinks {
* @param commit SHA1 of commit.
* @return Links for patch sets.
*/
public FluentIterable<WebLinkInfo> getPatchSetLinks(Project.NameKey project,
public List<WebLinkInfo> getPatchSetLinks(Project.NameKey project,
String commit) {
return filterLinks(
patchSetLinks,
@@ -108,7 +110,7 @@ public class WebLinks {
* @param file File name.
* @return Links for files.
*/
public FluentIterable<WebLinkInfo> getFileLinks(String project,
public List<WebLinkInfo> getFileLinks(String project,
String revision, String file) {
return filterLinks(
fileLinks,
@@ -122,7 +124,7 @@ public class WebLinks {
* @param file File name.
* @return Links for file history
*/
public FluentIterable<WebLinkInfoCommon> getFileHistoryLinks(
public List<WebLinkInfoCommon> getFileHistoryLinks(
String project, String revision, String file) {
return FluentIterable
.from(fileHistoryLinks)
@@ -140,7 +142,8 @@ public class WebLinks {
commonInfo.target = info.target;
return commonInfo;
})
.filter(INVALID_WEBLINK_COMMON);
.filter(INVALID_WEBLINK_COMMON)
.toList();
}
/**
@@ -155,7 +158,7 @@ public class WebLinks {
* @param fileB File name of side B.
* @return Links for file diffs.
*/
public FluentIterable<DiffWebLinkInfo> getDiffLinks(final String project, final int changeId,
public List<DiffWebLinkInfo> getDiffLinks(final String project, final int changeId,
final Integer patchSetIdA, final String revisionA, final String fileA,
final int patchSetIdB, final String revisionB, final String fileB) {
return FluentIterable
@@ -164,7 +167,8 @@ public class WebLinks {
webLink.getDiffLink(project, changeId,
patchSetIdA, revisionA, fileA,
patchSetIdB, revisionB, fileB))
.filter(INVALID_WEBLINK);
.filter(INVALID_WEBLINK)
.toList();
}
/**
@@ -172,7 +176,7 @@ public class WebLinks {
* @param project Project name.
* @return Links for projects.
*/
public FluentIterable<WebLinkInfo> getProjectLinks(final String project) {
public List<WebLinkInfo> getProjectLinks(final String project) {
return filterLinks(
projectLinks,
webLink -> webLink.getProjectWeblink(project));
@@ -184,17 +188,18 @@ public class WebLinks {
* @param branch Branch name
* @return Links for branches.
*/
public FluentIterable<WebLinkInfo> getBranchLinks(final String project, final String branch) {
public List<WebLinkInfo> getBranchLinks(final String project, final String branch) {
return filterLinks(
branchLinks,
webLink -> webLink.getBranchWebLink(project, branch));
}
private <T extends WebLink> FluentIterable<WebLinkInfo> filterLinks(DynamicSet<T> links,
private <T extends WebLink> List<WebLinkInfo> filterLinks(DynamicSet<T> links,
Function<T, WebLinkInfo> transformer) {
return FluentIterable
.from(links)
.transform(transformer)
.filter(INVALID_WEBLINK);
.filter(INVALID_WEBLINK)
.toList();
}
}

5
gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeEdits.java
View File

@@ -15,7 +15,6 @@
package com.google.gerrit.server.change;
import com.google.common.base.Strings;
import com.google.common.collect.FluentIterable;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.extensions.common.DiffWebLinkInfo;
import com.google.gerrit.extensions.common.EditInfo;
@@ -490,7 +489,7 @@ public class ChangeEdits implements
FileInfo r = new FileInfo();
ChangeEdit edit = rsrc.getChangeEdit();
Change change = edit.getChange();
FluentIterable<DiffWebLinkInfo> links =
List<DiffWebLinkInfo> links =
webLinks.getDiffLinks(change.getProject().get(),
change.getChangeId(),
edit.getBasePatchSet().getPatchSetId(),
@@ -499,7 +498,7 @@ public class ChangeEdits implements
0,
edit.getRefName(),
rsrc.getPath());
r.webLinks = links.isEmpty() ? null : links.toList();
r.webLinks = links.isEmpty() ? null : links;
return r;
}

4
gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java
View File

@@ -1211,9 +1211,9 @@ public class ChangeJson {
info.message = commit.getFullMessage();
if (addLinks) {
FluentIterable<WebLinkInfo> links =
List<WebLinkInfo> links =
webLinks.getPatchSetLinks(project, commit.name());
info.webLinks = links.isEmpty() ? null : links.toList();
info.webLinks = links.isEmpty() ? null : links;
}
for (RevCommit parent : commit.getParents()) {

9
gerrit-server/src/main/java/com/google/gerrit/server/change/GetDiff.java
View File

@@ -17,7 +17,6 @@ package com.google.gerrit.server.change;
import static com.google.common.base.Preconditions.checkState;
import com.google.common.base.MoreObjects;
import com.google.common.collect.FluentIterable;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Lists;
@@ -207,7 +206,7 @@ public class GetDiff implements RestReadView<FileResource> {
? resource.getRevision().getEdit().get().getRefName()
: resource.getRevision().getPatchSet().getRefName();
FluentIterable<DiffWebLinkInfo> links =
List<DiffWebLinkInfo> links =
webLinks.getDiffLinks(state.getProject().getName(),
resource.getPatchKey().getParentKey().getParentKey().get(),
basePatchSet != null ? basePatchSet.getId().get() : null,
@@ -216,7 +215,7 @@ public class GetDiff implements RestReadView<FileResource> {
resource.getPatchKey().getParentKey().get(),
revB,
ps.getNewName());
result.webLinks = links.isEmpty() ? null : links.toList();
result.webLinks = links.isEmpty() ? null : links;
if (!webLinksOnly) {
if (ps.isBinary()) {
@@ -281,9 +280,9 @@ public class GetDiff implements RestReadView<FileResource> {
private List<WebLinkInfo> getFileWebLinks(Project project, String rev,
String file) {
FluentIterable<WebLinkInfo> links =
List<WebLinkInfo> links =
webLinks.getFileLinks(project.getName(), rev, file);
return links.isEmpty() ? null : links.toList();
return links.isEmpty() ? null : links;
}
public GetDiff setBase(String base) {

5
gerrit-server/src/main/java/com/google/gerrit/server/project/ListBranches.java
View File

@@ -15,7 +15,6 @@
package com.google.gerrit.server.project;
import com.google.common.collect.ComparisonChain;
import com.google.common.collect.FluentIterable;
import com.google.common.collect.Sets;
import com.google.gerrit.extensions.api.projects.BranchInfo;
import com.google.gerrit.extensions.common.ActionInfo;
@@ -191,10 +190,10 @@ public class ListBranches implements RestReadView<ProjectResource> {
}
info.actions.put(d.getId(), new ActionInfo(d));
}
FluentIterable<WebLinkInfo> links =
List<WebLinkInfo> links =
webLinks.getBranchLinks(
refControl.getProjectControl().getProject().getName(), ref.getName());
info.webLinks = links.isEmpty() ? null : links.toList();
info.webLinks = links.isEmpty() ? null : links;
return info;
}
}

5
gerrit-server/src/main/java/com/google/gerrit/server/project/ListProjects.java
View File

@@ -17,7 +17,6 @@ package com.google.gerrit.server.project;
import static java.nio.charset.StandardCharsets.UTF_8;
import com.google.common.base.Strings;
import com.google.common.collect.FluentIterable;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Iterables;
import com.google.gerrit.common.data.GroupReference;
@@ -378,9 +377,9 @@ public class ListProjects implements RestReadView<TopLevelResource> {
log.warn("Unexpected error reading " + projectName, err);
continue;
}
FluentIterable<WebLinkInfo> links =
List<WebLinkInfo> links =
webLinks.getProjectLinks(projectName.get());
info.webLinks = links.isEmpty() ? null : links.toList();
info.webLinks = links.isEmpty() ? null : links;
}
if (foundIndex++ < start) {

7
gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectJson.java
View File

@@ -15,7 +15,6 @@
package com.google.gerrit.server.project;
import com.google.common.base.Strings;
import com.google.common.collect.FluentIterable;
import com.google.gerrit.extensions.common.ProjectInfo;
import com.google.gerrit.extensions.common.WebLinkInfo;
import com.google.gerrit.extensions.restapi.Url;
@@ -25,6 +24,8 @@ import com.google.gerrit.server.config.AllProjectsName;
import com.google.inject.Inject;
import com.google.inject.Singleton;
import java.util.List;
@Singleton
public class ProjectJson {
@@ -50,9 +51,9 @@ public class ProjectJson {
info.description = Strings.emptyToNull(p.getDescription());
info.state = p.getState();
info.id = Url.encode(info.name);
FluentIterable<WebLinkInfo> links =
List<WebLinkInfo> links =
webLinks.getProjectLinks(p.getName());
info.webLinks = links.isEmpty() ? null : links.toList();
info.webLinks = links.isEmpty() ? null : links;
return info;
}
}