[
https://issues.apache.org/jira/browse/GEODE-3268?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16117491#comment-16117491
]
ASF GitHub Bot commented on GEODE-3268:
---------------------------------------
Github user PurelyApplied commented on a diff in the pull request:
https://github.com/apache/geode/pull/693#discussion_r131783322
--- Diff:
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ListRegionCommand.java
---
@@ -0,0 +1,113 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
contributor license
+ * agreements. See the NOTICE file distributed with this work for
additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache
License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF
ANY KIND, either express
+ * or implied. See the License for the specific language governing
permissions and limitations under
+ * the License.
+ */
+
+package org.apache.geode.management.internal.cli.commands;
+
+import java.util.ArrayList;
+import java.util.LinkedHashSet;
+import java.util.Set;
+import java.util.TreeSet;
+
+import org.springframework.shell.core.annotation.CliCommand;
+import org.springframework.shell.core.annotation.CliOption;
+
+import org.apache.geode.cache.execute.FunctionInvocationTargetException;
+import org.apache.geode.cache.execute.ResultCollector;
+import org.apache.geode.distributed.DistributedMember;
+import org.apache.geode.management.cli.CliMetaData;
+import org.apache.geode.management.cli.ConverterHint;
+import org.apache.geode.management.cli.Result;
+import org.apache.geode.management.internal.cli.CliUtil;
+import org.apache.geode.management.internal.cli.domain.RegionInformation;
+import
org.apache.geode.management.internal.cli.functions.GetRegionsFunction;
+import org.apache.geode.management.internal.cli.i18n.CliStrings;
+import org.apache.geode.management.internal.cli.result.ResultBuilder;
+import org.apache.geode.management.internal.cli.result.TabularResultData;
+import org.apache.geode.management.internal.security.ResourceOperation;
+import org.apache.geode.security.ResourcePermission;
+
+public class ListRegionCommand implements GfshCommand {
+ private static final GetRegionsFunction getRegionsFunction = new
GetRegionsFunction();
+
+ @CliCommand(value = {CliStrings.LIST_REGION}, help =
CliStrings.LIST_REGION__HELP)
+ @CliMetaData(relatedTopic = CliStrings.TOPIC_GEODE_REGION)
+ @ResourceOperation(resource = ResourcePermission.Resource.CLUSTER,
+ operation = ResourcePermission.Operation.READ)
+ public Result listRegion(
+ @CliOption(key = {CliStrings.GROUP, CliStrings.GROUPS},
+ optionContext = ConverterHint.MEMBERGROUP,
+ help = CliStrings.LIST_REGION__GROUP__HELP) String[] group,
+ @CliOption(key = {CliStrings.MEMBER, CliStrings.MEMBERS},
+ optionContext = ConverterHint.MEMBERIDNAME,
+ help = CliStrings.LIST_REGION__MEMBER__HELP) String[]
memberNameOrId) {
+ Result result = null;
+ try {
+ Set<RegionInformation> regionInfoSet = new LinkedHashSet<>();
+ ResultCollector<?, ?> rc;
+ Set<DistributedMember> targetMembers = CliUtil.findMembers(group,
memberNameOrId);
+
+ if (targetMembers.isEmpty()) {
+ return
ResultBuilder.createUserErrorResult(CliStrings.NO_MEMBERS_FOUND_MESSAGE);
+ }
+
+ TabularResultData resultData =
ResultBuilder.createTabularResultData();
+ rc = CliUtil.executeFunction(getRegionsFunction, null,
targetMembers);
+ ArrayList<?> resultList = (ArrayList<?>) rc.getResult();
+
+ if (resultList != null) {
--- End diff --
It is arguably beyond the scope of this ticket to refactor this, but this
sort of nesting makes my skin crawl.
Dropping line 72 because `instanceof` requires non-`null`.
([Docs](http://docs.oracle.com/javase/specs/jls/se7/html/jls-15.html#jls-15.20.2))
I'm pretty confident this is equivalent, but it would certainly warrant a
new precheckin.
```
Set<RegionInformation> regionInfoSet;
if (resultList != null) {
regionInfoSet = resultList.stream()
.filter(resultObj -> resultObj instanceof Object[])
.map(resultObj -> (Object[]) resultObj)
.flatMap(Arrays::stream)
.filter(regionInfo -> regionInfo instanceof RegionInformation)
.map(regionInfo -> (RegionInformation) regionInfo)
.collect(Collectors.toCollection(LinkedHashSet::new));
// ...
} // ends if (result != null)
Come to that, you could flatten a little more with a
```
if (resultList == null) {
return result;
}
```
While you were at it, you could probably change many of the other `for`
loops to be stream maps, but those aren't nearly as offensive to me as this one
is.
> Refactor RegionCommands
> -----------------------
>
> Key: GEODE-3268
> URL: https://issues.apache.org/jira/browse/GEODE-3268
> Project: Geode
> Issue Type: Sub-task
> Components: gfsh
> Reporter: Emily Yeh
> Assignee: Emily Yeh
>
> {{RegionCommands.java}} is a large class that contains multiple commands.
> Each command should be refactored into a separate class, and the methods
> shared by the commands should be refactored into a new and appropriately
> named class of their own.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
