Jv dependency injection - #1262
Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
The Injector implementation has several critical functional issues and violates explicit checklist requirements that will cause test failures. First, the createNewImstance method lacks the required @Component annotation check (clazz.isAnnotationPresent(Component.class)) before instantiation — this is explicitly mandated by checklist item #4. Second, the loop at line 36 creates a new instance for EACH @Inject field instead of creating ONE instance and injecting all fields into it, breaking singleton behavior. Third, the interfaceImplementations map is populated inside findImplementation() on every call rather than being initialized once as a class field using Map.of() (checklist item #6). Fourth, the catch block uses multiple specific exceptions (NoSuchMethodException | InstantiationException | IllegalAccessException | InvocationTargetException) instead of their common parent ReflectiveOperationException (checklist item #8), and caught exceptions are not chained to the RuntimeException (checklist item #7). Checkstyle violations are noted but are non-blocking since the build compiles successfully — focus on fixing the functional issues first.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
| } | ||
| return clazzImplementationInstance; | ||
| } | ||
|
|
There was a problem hiding this comment.
Checklist item #4 violated: There's no check for @Component annotation before creating an instance. According to the task requirements, if the Component annotation is missing above the class, we shouldn't be able to create an instance and should throw an exception. The createNewImstance method should verify clazz.isAnnotationPresent(Component.class) before instantiation.
| public class Injector { | ||
| private static final Injector injector = new Injector(); | ||
| private Map<Class<?>, Object> instances = new HashMap<>(); | ||
| Map<Class<?>, Class<?>> interfaceImplementations = new HashMap<>(); |
There was a problem hiding this comment.
Checklist item #6 violated: The interfaceImplementations map should be filled using Map.of() instead of new HashMap<>(). Change line 20 to: Map<Class<?>, Class<?>> interfaceImplementations = Map.of(...) and initialize it with all three mappings.
| return instance; | ||
| } catch (NoSuchMethodException | InstantiationException | ||
| | IllegalAccessException | InvocationTargetException e) { |
There was a problem hiding this comment.
Checklist item #8 violated: Lines 64-66 catch multiple specific exceptions. According to checklist, it is better to replace many exceptions that have a common parent with a general parental exception. Use catch (ReflectiveOperationException e) instead, as all these exceptions (NoSuchMethodException, InstantiationException, IllegalAccessException, InvocationTargetException) inherit from it.
| private Class<?> findImplementation(Class<?> interfaceClazz) { | ||
| interfaceImplementations.put(FileReaderService.class, FileReaderServiceImpl.class); | ||
| interfaceImplementations.put(ProductParser.class, ProductParserImpl.class); |
There was a problem hiding this comment.
Lines 72-74 populate the map on every call to findImplementation. This should be done once during initialization, not repeatedly. Move this initialization to a constructor or static initializer block, or initialize the map directly at declaration using Map.of().
| @Inject | ||
| private FileReaderService fileReaderService; | ||
|
|
||
| @Override |
There was a problem hiding this comment.
Checklist item #6 violation: interfaceImplementations should be initialized using Map.of() as a class field, not populated with put() inside findImplementation(). Change line 20 to: Map<Class, Class> interfaceImplementations = Map.of(FileReaderService.class, FileReaderServiceImpl.class, ProductParser.class, ProductParserImpl.class, ProductService.class, ProductServiceImpl.class);
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
The Injector.java implementation has four critical violations of mandatory checklist requirements that must be fixed before approval. First, the @Component annotation check at line 27 happens AFTER instantiation at line 26 - it must be moved to createNewInstance() or before calling it per [CHECKLIST ITEM #4]. Second, the interfaceImplementations map at line 17 is a HashMap populated with put() calls inside findImplementation() on every invocation instead of using Map.of() as a one-time class field initializer [CHECKLIST ITEM #6]. Third, both catch blocks in createNewInstance() don't chain exceptions - use throw new RuntimeException("message", e) instead of just throw new RuntimeException("message") [CHECKLIST ITEM #7]. Fourth, replace multiple specific exceptions (NoSuchMethodException, InstantiationException, IllegalAccessException, InvocationTargetException) with their common parent ReflectiveOperationException [CHECKLIST ITEM #8]. The service implementation classes are correctly done - focus on fixing the Injector class.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
| } | ||
|
|
||
| private Class<?> findImplementation(Class<?> interfaceClazz) { | ||
| interfaceImplementations.put(FileReaderService.class, FileReaderServiceImpl.class); |
There was a problem hiding this comment.
This violates checklist item #6: The interface implementations map should be initialized using Map.of() as a class field initializer, not populated with put() calls inside the method on every invocation. Move these mappings to line 17.
| return null; | ||
| Class<?> clazz = findImplementation(interfaceClazz); | ||
| Object clazzImplementationInstance = createNewInstance(clazz); | ||
| if (!clazz.isAnnotationPresent(Component.class)) { |
There was a problem hiding this comment.
The @Component check happens AFTER createNewInstance() is called at line 26. This violates checklist item #4 - the check must happen BEFORE instantiation. Move this validation to createNewInstance() or before calling it.
| fields.setAccessible(true); | ||
| fields.set(clazzImplementationInstance, fieldsInstance); | ||
| } catch (IllegalAccessException e) { | ||
| throw new RuntimeException("Can't initialize field value." |
There was a problem hiding this comment.
This violates checklist item #7: The caught exception is not chained to the RuntimeException. Use the constructor's second parameter: throw new RuntimeException(..., e);
| return instance; | ||
| } catch (NoSuchMethodException | InstantiationException |
There was a problem hiding this comment.
This violates checklist item #8: Replace multiple specific exceptions (NoSuchMethodException | InstantiationException | IllegalAccessException | InvocationTargetException) with their common parent ReflectiveOperationException.
| return instance; | ||
| } catch (NoSuchMethodException | InstantiationException | ||
| | IllegalAccessException | InvocationTargetException e) { | ||
| throw new RuntimeException("Can't create a new instance of " + clazz.getName()); |
There was a problem hiding this comment.
This violates checklist item #7: The caught exception is not chained to the RuntimeException. Add the caught exception as the second parameter to the RuntimeException constructor.
| interfaceImplementations.put(FileReaderService.class, FileReaderServiceImpl.class); | ||
| interfaceImplementations.put(ProductParser.class, ProductParserImpl.class); | ||
| interfaceImplementations.put(ProductService.class, ProductServiceImpl.class); |
There was a problem hiding this comment.
Checklist item #5 violation: The interfaceImplementations.put() calls at lines 69-71 should not be inside this method. Move map initialization to a field initializer using Map.of() at declaration time (line 17).
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
This is an educational override to help you continue learning. Your Injector.java implementation shows solid progress — the @component validation, interfaceImplementations map with Map.of(), and ReflectiveOperationException on line 62 are all correct. Two remaining items to fix before the next level: 1) Add exception chaining on line 46 by changing throw new RuntimeException("Can't initialize field value...") to throw new RuntimeException("Can't initialize field value. Class: " + clazz.getName() + ". Field: " + fields.getName(), e) per [CHECKLIST ITEM #7]; 2) Replace IllegalAccessException with ReflectiveOperationException on line 42 to be consistent with line 62 per [CHECKLIST ITEM #8]. You're almost there — these are quick fixes that will make your error handling much more maintainable. Keep going!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| fields.set(clazzImplementationInstance, fieldsInstance); | ||
| } catch (IllegalAccessException e) { | ||
| throw new RuntimeException("Can't initialize field value." | ||
| + "Class: " + clazz.getName() + ". Field: " |
There was a problem hiding this comment.
CHECKLIST ITEM #7 VIOLATION: The exception is not chained to the RuntimeException. Use throw new RuntimeException("message", e) to include the caught exception.
|
|
||
| try { | ||
| fields.setAccessible(true); | ||
| fields.set(clazzImplementationInstance, fieldsInstance); |
There was a problem hiding this comment.
CHECKLIST ITEM #8 VIOLATION: Replace IllegalAccessException with ReflectiveOperationException which is its parent class (consistent with line 62).
No description provided.