Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactoring #75

Closed
6 tasks done
Clashsoft opened this issue Mar 31, 2024 · 0 comments · Fixed by #87
Closed
6 tasks done

Refactoring #75

Clashsoft opened this issue Mar 31, 2024 · 0 comments · Fixed by #87
Assignees
Labels
enhancement New feature or request

Comments

@Clashsoft
Copy link
Member

Clashsoft commented Mar 31, 2024

  • There are many uses of control statements without braces, which is a warning and discouraged. Please fix.
  • Rename OnXY annotations #84
  • The ControllerUtil.isController method actually checks if the given object is a Controller OR a Component. It should be named more aptly isControllerOrComponent. In addition, an actual isController method should be added. This example could benefit greatly:
    // Check if the instance is a controller/component
    boolean component = instance.getClass().isAnnotationPresent(Component.class) && ControllerUtil.isComponent(instance);
    if (!component && !instance.getClass().isAnnotationPresent(Controller.class))
  • This is a stream that only ever processes a single element:
    return fields
    .stream()
    .filter(field -> {
    if (field.getType().isAssignableFrom(ResourceBundle.class))
    return true;
    throw new RuntimeException(error(2004).formatted(field.getName(), instance.getClass().getName()));
    })
    .map(field -> {
    try {
    field.setAccessible(true);
    return (ResourceBundle) field.get(instance);
    } catch (IllegalAccessException e) {
    throw new RuntimeException(error(2005).formatted(field.getName(), instance.getClass().getName()), e);
    }
    })
    .filter(Objects::nonNull)
    .findFirst()
    .orElse(defaultResourceBundle);
    Please simplify.
  • This stream would be more readable as a simple forEach lambda that does not require Pairs.
    Reflection.getFieldsOfType(instance.getClass(), Subscriber.class) // Get all Subscriber fields
    .map(field -> {
    try {
    field.setAccessible(true);
    return new Pair<>(field, (Subscriber) field.get(instance)); // Get the Subscriber instance, if it exists
    } catch (IllegalAccessException e) {
    return null;
    }
    })
    .filter(Objects::nonNull)
    .filter(pair -> pair.getKey() != null)
    .filter(pair -> !pair.getValue().isDisposed()) // Filter out disposed subscribers
    .forEach(pair ->
    FulibFxApp.LOGGER.warning("Found undestroyed subscriber '%s' in class '%s'.".formatted(pair.getKey().getName(), instance.getClass().getName()))
    );
  • This supposed utility method has side effects in the filter lambda:
    return Reflection.getAllFieldsWithAnnotation(instance.getClass(), SubComponent.class)
    .filter(field -> {
    if (ControllerUtil.isComponent(field.getType())) return true;
    if (!ControllerUtil.canProvideSubComponent(field)) {
    FulibFxApp.LOGGER.warning(error(6005).formatted(field.getName(), instance.getClass().getName()));
    }
    return false;
    }).toList();
    Please put the warning into a more appropriate place where it is not printed every time the method is called.
@Clashsoft Clashsoft added the enhancement New feature or request label Mar 31, 2024
@LeStegii LeStegii mentioned this issue Apr 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants