Merge pull request #429 from wttech/fix-code-smells

dprzybyl · web-flow · commit c79476815b10 · 2023-11-03T02:12:27.000+01:00
fixed some code smells
diff --git a/app/aem/actions.checks/src/main/java/com/cognifide/apm/checks/actions/exists/CheckAuthorizableExists.java b/app/aem/actions.checks/src/main/java/com/cognifide/apm/checks/actions/exists/CheckAuthorizableExists.java
@@ -56,7 +56,7 @@ public ActionResult execute(final Context context) {
   public ActionResult process(final Context context, boolean execute) {
     ActionResult actionResult = context.createActionResult();
     try {
-      Authorizable authorizable = null;
+      Authorizable authorizable;
       if (shouldBeGroup) {
         authorizable = context.getAuthorizableManager().getGroupIfExists(id);
       } else {
diff --git a/app/aem/actions.checks/src/main/java/com/cognifide/apm/checks/utils/ActionUtils.java b/app/aem/actions.checks/src/main/java/com/cognifide/apm/checks/utils/ActionUtils.java
@@ -20,55 +20,14 @@
 package com.cognifide.apm.checks.utils;
 
 import com.cognifide.apm.api.actions.ActionResult;
-import com.cognifide.apm.api.exceptions.ActionExecutionException;
-import java.util.Iterator;
 import java.util.List;
-import javax.jcr.RepositoryException;
-import org.apache.jackrabbit.api.security.user.Authorizable;
-import org.apache.jackrabbit.api.security.user.Group;
 
 public final class ActionUtils {
 
   public static final String ASSERTION_FAILED_MSG = "Assertion failed";
 
   private ActionUtils() {
-  }
-
-  /**
-   * Adding group to another group may result in cyclic relation. Let current group be the group where we
-   * want to add current authorizable to. If current authorizable contains group such that current group
-   * belongs to, then we prevent such operation.
-   *
-   * @param currentGroup   The group where we want to add current authorizable
-   * @param groupToBeAdded Authorizable we want to add
-   * @throws ActionExecutionException Throw exception, if adding operation results in cyclic relation
-   */
-  public static void checkCyclicRelations(Group currentGroup, Group groupToBeAdded)
-      throws ActionExecutionException {
-    try {
-      if (groupToBeAdded.getID().equals(currentGroup.getID())) {
-        throw new ActionExecutionException(MessagingUtils.addingGroupToItself(currentGroup.getID()));
-      }
-      Iterator<Group> parents = currentGroup.memberOf();
-      while (parents.hasNext()) {
-        Group currentParent = parents.next();
-        // Is added group among my parents?
-        if (currentParent.getID().equals(groupToBeAdded.getID())) {
-          throw new ActionExecutionException(MessagingUtils.cyclicRelationsForbidden(
-              currentGroup.getID(), groupToBeAdded.getID()));
-        }
-        // ... and are its children among my parents?
-        for (Iterator<Authorizable> children = groupToBeAdded.getMembers(); children.hasNext(); ) {
-          Authorizable currentChild = children.next();
-          if (currentParent.getID().equals(currentChild.getID())) {
-            throw new ActionExecutionException(MessagingUtils.cyclicRelationsForbidden(
-                currentChild.getID(), groupToBeAdded.getID()));
-          }
-        }
-      }
-    } catch (RepositoryException e) {
-      throw new ActionExecutionException(MessagingUtils.createMessage(e));
-    }
+    // intentionally empty
   }
 
   public static void logErrors(List<String> errors, ActionResult actionResult) {
diff --git a/app/aem/actions.checks/src/main/java/com/cognifide/apm/checks/utils/MessagingUtils.java b/app/aem/actions.checks/src/main/java/com/cognifide/apm/checks/utils/MessagingUtils.java
@@ -31,16 +31,7 @@ public static String createMessage(Exception e) {
     return StringUtils.isBlank(e.getMessage()) ? "Internal error: " + e.getClass() : e.getMessage();
   }
 
-  public static String addingGroupToItself(String groupId) {
-    return "You can not add group " + groupId + " to itself";
-  }
-
   public static String authorizableNotExists(String authorizableId) {
     return "Authorizable with id: " + authorizableId + " does not exists";
   }
-
-  public static String cyclicRelationsForbidden(String currentGroup, String groupToBeAdded) {
-    return "Cannot add group " + groupToBeAdded + " to group " + currentGroup
-        + " due to resulting cyclic relation";
-  }
 }
diff --git a/app/aem/actions.main/src/main/java/com/cognifide/apm/main/permissions/PermissionActionHelper.java b/app/aem/actions.main/src/main/java/com/cognifide/apm/main/permissions/PermissionActionHelper.java
@@ -81,7 +81,7 @@ private void addEntry(boolean allow, final List<Privilege> privileges,
 
     Map<String, Value> singleValueRestrictions = restrictions.getSingleValueRestrictions(valueFactory);
     Map<String, Value[]> multiValueRestrictions = restrictions.getMultiValueRestrictions(valueFactory);
-    jackrabbitAcl.addEntry(principal, privileges.toArray(new Privilege[privileges.size()]), allow,
+    jackrabbitAcl.addEntry(principal, privileges.toArray(new Privilege[]{}), allow,
         singleValueRestrictions, multiValueRestrictions);
   }
 
diff --git a/app/aem/actions.main/src/main/java/com/cognifide/apm/main/utils/ActionUtils.java b/app/aem/actions.main/src/main/java/com/cognifide/apm/main/utils/ActionUtils.java
@@ -19,10 +19,8 @@
  */
 package com.cognifide.apm.main.utils;
 
-import com.cognifide.apm.api.actions.ActionResult;
 import com.cognifide.apm.api.exceptions.ActionExecutionException;
 import java.util.Iterator;
-import java.util.List;
 import javax.jcr.RepositoryException;
 import org.apache.jackrabbit.api.security.user.Authorizable;
 import org.apache.jackrabbit.api.security.user.Group;
@@ -32,6 +30,7 @@ public final class ActionUtils {
   public static final String EXECUTION_INTERRUPTED_MSG = "Execution interrupted";
 
   private ActionUtils() {
+    // intentionally empty
   }
 
   /**
@@ -70,10 +69,4 @@ public static void checkCyclicRelations(Group currentGroup, Group groupToBeAdded
       throw new ActionExecutionException(MessagingUtils.createMessage(e));
     }
   }
-
-  public static void logErrors(List<String> errors, ActionResult actionResult) {
-    for (String error : errors) {
-      actionResult.logError(error);
-    }
-  }
 }
diff --git a/app/aem/api/src/main/java/com/cognifide/apm/api/actions/Message.java b/app/aem/api/src/main/java/com/cognifide/apm/api/actions/Message.java
@@ -32,8 +32,8 @@ public Message(String text, String type) {
     this.type = type;
   }
 
-  // Gson library needs this
   private Message() {
+    // GSON library needs
   }
 
   public String getText() {
diff --git a/app/aem/api/src/main/java/com/cognifide/apm/api/exceptions/InitializationException.java b/app/aem/api/src/main/java/com/cognifide/apm/api/exceptions/InitializationException.java
diff --git a/app/aem/core/src/main/java/com/cognifide/apm/core/actions/ActionFactoryImpl.java b/app/aem/core/src/main/java/com/cognifide/apm/core/actions/ActionFactoryImpl.java
@@ -24,7 +24,6 @@
 import com.cognifide.apm.core.Property;
 import com.cognifide.apm.core.grammar.argument.Arguments;
 import java.util.ArrayList;
-import java.util.Collections;
 import java.util.Comparator;
 import java.util.List;
 import java.util.Optional;
@@ -82,7 +81,7 @@ public List<CommandDescription> getCommandDescriptions() {
   }
 
   private void sortReferences(List<CommandDescription> references) {
-    Collections.sort(references, Comparator.comparing(CommandDescription::getGroup, (o1, o2) -> {
+    references.sort(Comparator.comparing(CommandDescription::getGroup, (o1, o2) -> {
       if (CORE_GROUP.equals(o1) && CORE_GROUP.equals(o2)) {
         return 0;
       }
diff --git a/app/aem/core/src/main/java/com/cognifide/apm/core/actions/ActionMapperRegistryImpl.java b/app/aem/core/src/main/java/com/cognifide/apm/core/actions/ActionMapperRegistryImpl.java
@@ -28,13 +28,13 @@
 import com.cognifide.apm.main.services.ApmActionsMainService;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Maps;
+import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.List;
 import java.util.Map;
 import java.util.Optional;
 import java.util.concurrent.atomic.AtomicReference;
-import java.util.stream.Collectors;
 import org.osgi.service.component.ComponentContext;
 import org.osgi.service.component.annotations.Activate;
 import org.osgi.service.component.annotations.Component;
@@ -92,15 +92,13 @@ public Optional<MapperDescriptor> getMapper(String name) {
 
   @Override
   public Collection<MapperDescriptor> getMappers() {
-    return mappers.get().values()
-        .stream()
-        .collect(Collectors.toList());
+    return new ArrayList<>(mappers.get().values());
   }
 
   private static Map<String, MapperDescriptor> createActionMappers(List<Class<?>> classes) {
     MapperDescriptorFactory mapperDescriptorFactory = new MapperDescriptorFactory();
     Map<String, MapperDescriptor> mappers = Maps.newHashMapWithExpectedSize(classes.size());
-    for (Class clazz : classes) {
+    for (Class<?> clazz : classes) {
       try {
         MapperDescriptor mapperDescriptor = mapperDescriptorFactory.create(clazz);
         mappers.put(mapperDescriptor.getName(), mapperDescriptor);
diff --git a/app/aem/core/src/main/java/com/cognifide/apm/core/actions/MapperDescriptorFactory.java b/app/aem/core/src/main/java/com/cognifide/apm/core/actions/MapperDescriptorFactory.java
@@ -115,9 +115,9 @@ private Optional<MappingDescriptor> create(Mapper mapper, Method method) {
   }
 
   private <T extends Annotation> T getAnnotation(Annotation[] annotations, Class<T> type) {
-    for (int i = 0; i < annotations.length; i++) {
-      if (type.isInstance(annotations[i])) {
-        return (T) annotations[i];
+    for (Annotation annotation : annotations) {
+      if (type.isInstance(annotation)) {
+        return (T) annotation;
       }
     }
     return null;
@@ -129,16 +129,15 @@ private <T extends Annotation> boolean containsAnnotation(Annotation[] annotatio
 
   private Class<? extends ApmType> getApmType(Type type) {
     if (type instanceof Class) {
-      Class aClass = (Class) type;
-      if (String.class.equals(aClass)) {
+      Class<?> clazz = (Class<?>) type;
+      if (String.class.equals(clazz)) {
         return ApmString.class;
-      }
-      if (Integer.class.equals(aClass)) {
+      } else if (Integer.class.equals(clazz)) {
         return ApmInteger.class;
       }
     } else if (type instanceof ParameterizedType) {
       ParameterizedType parameterizedType = (ParameterizedType) type;
-      Class rawType = (Class) parameterizedType.getRawType();
+      Class<?> rawType = (Class<?>) parameterizedType.getRawType();
       if (List.class.equals(rawType)) {
         return ApmList.class;
       } else if (Map.class.equals(rawType)) {
diff --git a/app/aem/core/src/main/java/com/cognifide/apm/core/actions/scanner/AnnotatedClassRegistry.java b/app/aem/core/src/main/java/com/cognifide/apm/core/actions/scanner/AnnotatedClassRegistry.java
@@ -44,7 +44,7 @@ public class AnnotatedClassRegistry {
 
   private static final Logger LOG = LoggerFactory.getLogger(AnnotatedClassRegistry.class);
 
-  private final BundleTracker tracker;
+  private final BundleTracker<?> tracker;
 
   private final Map<Long, List<Class<?>>> classes = new ConcurrentHashMap<>();
 
@@ -62,7 +62,7 @@ public AnnotatedClassRegistry(final BundleContext bundleContext, final String bu
     this.bundleHeader = bundleHeader;
     this.annotationClass = annotationClass;
 
-    this.tracker = new BundleTracker(bundleContext, Bundle.ACTIVE, new BundleTrackerCustomizer<Bundle>() {
+    this.tracker = new BundleTracker<>(bundleContext, Bundle.ACTIVE, new BundleTrackerCustomizer<Bundle>() {
 
       @Override
       public Bundle addingBundle(Bundle bundle, BundleEvent bundleEvent) {
diff --git a/app/aem/core/src/main/java/com/cognifide/apm/core/launchers/AbstractLauncher.java b/app/aem/core/src/main/java/com/cognifide/apm/core/launchers/AbstractLauncher.java
@@ -43,8 +43,8 @@ protected void processScripts(List<Script> scripts, ResourceResolver resolver) t
     if (!scripts.isEmpty()) {
       String scriptPathsStr = scripts.stream()
           .map(Script::getPath)
-          .collect(Collectors.joining("\n"));
-      logger.info("Launcher will try to run following scripts: {}\n{}", scripts.size(), scriptPathsStr);
+          .collect(Collectors.joining(", "));
+      logger.info("Launcher will try to run following scripts ({}): {}", scripts.size(), scriptPathsStr);
       for (Script script : scripts) {
         processScript(script, resolver);
       }
diff --git a/app/aem/core/src/main/java/com/cognifide/apm/core/ui/datasources/HistoryDatasourceServlet.java b/app/aem/core/src/main/java/com/cognifide/apm/core/ui/datasources/HistoryDatasourceServlet.java
@@ -24,7 +24,6 @@
 import com.cognifide.apm.core.Property;
 import com.cognifide.apm.core.history.History;
 import com.cognifide.apm.core.utils.Pagination;
-import com.google.common.primitives.Ints;
 import java.util.List;
 import java.util.stream.Collectors;
 import javax.servlet.Servlet;
@@ -64,16 +63,16 @@ protected void doGet(SlingHttpServletRequest request, SlingHttpServletResponse r
 
   private Pagination createPagination(SlingHttpServletRequest request) {
     String[] selectors = request.getRequestPathInfo().getSelectors();
-    if (selectors == null || selectors.length < 2) {
+    if (selectors.length < 2) {
       return new Pagination(0, DEFAULT_LIMIT + 1);
     } else {
-      Integer offset = Ints.tryParse(selectors[0]);
-      Integer limit = Ints.tryParse(selectors[1]);
+      int offset = Integer.parseInt(selectors[0]);
+      int limit = Integer.parseInt(selectors[1]);
       return new Pagination(offset, limit + 1);
     }
   }
 
-  private class ResourceTypeWrapper extends ResourceWrapper {
+  private static class ResourceTypeWrapper extends ResourceWrapper {
 
     ResourceTypeWrapper(Resource resource) {
       super(resource);
diff --git a/app/aem/core/src/main/java/com/cognifide/apm/core/ui/datasources/ScriptsDatasourceServlet.java b/app/aem/core/src/main/java/com/cognifide/apm/core/ui/datasources/ScriptsDatasourceServlet.java
@@ -20,8 +20,6 @@
  */
 package com.cognifide.apm.core.ui.datasources;
 
-import static com.cognifide.apm.core.ui.models.ScriptsRowModel.SCRIPTS_ROW_RESOURCE_TYPE;
-
 import com.adobe.granite.ui.components.ds.DataSource;
 import com.adobe.granite.ui.components.ds.SimpleDataSource;
 import com.cognifide.apm.core.Property;
@@ -68,15 +66,15 @@ protected void doGet(SlingHttpServletRequest request, SlingHttpServletResponse r
     request.setAttribute(DataSource.class.getName(), dataSource);
   }
 
-  private class ResourceTypeWrapper extends ResourceWrapper {
+  private static class ResourceTypeWrapper extends ResourceWrapper {
 
     ResourceTypeWrapper(Resource resource) {
       super(resource);
     }
 
     @Override
     public String getResourceType() {
-      return SCRIPTS_ROW_RESOURCE_TYPE;
+      return ScriptsRowModel.SCRIPTS_ROW_RESOURCE_TYPE;
     }
   }
 
diff --git a/app/aem/core/src/main/java/com/cognifide/apm/core/utils/ResourceMixinUtil.java b/app/aem/core/src/main/java/com/cognifide/apm/core/utils/ResourceMixinUtil.java
@@ -31,10 +31,10 @@ private ResourceMixinUtil() {
     // empty constructor
   }
 
-  public static void addMixin(ModifiableValueMap vm, String mixin) {
-    Set<String> mixins = new HashSet<>(Arrays.asList(vm.get(JcrConstants.JCR_MIXINTYPES, new String[0])));
+  public static void addMixin(ModifiableValueMap valueMap, String mixin) {
+    Set<String> mixins = new HashSet<>(Arrays.asList(valueMap.get(JcrConstants.JCR_MIXINTYPES, new String[]{})));
     mixins.add(mixin);
-    vm.put(JcrConstants.JCR_MIXINTYPES, mixins.toArray(new String[]{}));
+    valueMap.put(JcrConstants.JCR_MIXINTYPES, mixins.toArray(new String[]{}));
   }
 
 }

Original file line number	Diff line number	Diff line change
`@@ -31,16 +31,7 @@ public static String createMessage(Exception e) {`
`31`	`31`	`return StringUtils.isBlank(e.getMessage()) ? "Internal error: " + e.getClass() : e.getMessage();`
`32`	`32`	`}`
`33`	`33`
`34`		`- public static String addingGroupToItself(String groupId) {`
`35`		`- return "You can not add group " + groupId + " to itself";`
`36`		`- }`
`37`		`-`
`38`	`34`	`public static String authorizableNotExists(String authorizableId) {`
`39`	`35`	`return "Authorizable with id: " + authorizableId + " does not exists";`
`40`	`36`	`}`
`41`		`-`
`42`		`- public static String cyclicRelationsForbidden(String currentGroup, String groupToBeAdded) {`
`43`		`- return "Cannot add group " + groupToBeAdded + " to group " + currentGroup`
`44`		`- + " due to resulting cyclic relation";`
`45`		`- }`
`46`	`37`	`}`
Original file line number	Diff line number	Diff line change
`@@ -81,7 +81,7 @@ private void addEntry(boolean allow, final List<Privilege> privileges,`
`81`	`81`
`82`	`82`	`Map<String, Value> singleValueRestrictions = restrictions.getSingleValueRestrictions(valueFactory);`
`83`	`83`	`Map<String, Value[]> multiValueRestrictions = restrictions.getMultiValueRestrictions(valueFactory);`
`84`		`- jackrabbitAcl.addEntry(principal, privileges.toArray(new Privilege[privileges.size()]), allow,`
	`84`	`+ jackrabbitAcl.addEntry(principal, privileges.toArray(new Privilege[]{}), allow,`
`85`	`85`	`singleValueRestrictions, multiValueRestrictions);`
`86`	`86`	`}`
`87`	`87`
Original file line number	Diff line number	Diff line change
`@@ -32,8 +32,8 @@ public Message(String text, String type) {`
`32`	`32`	`this.type = type;`
`33`	`33`	`}`
`34`	`34`
`35`		`- // Gson library needs this`
`36`	`35`	`private Message() {`
	`36`	`+ // GSON library needs`
`37`	`37`	`}`
`38`	`38`
`39`	`39`	`public String getText() {`