My hometask dependency injection #1005
Conversation
There was a problem hiding this comment.
Hey there! It looks like there are a few things that need your attention before we can move forward. Here are some pointers to help you out:
- Make sure to provide more informative exception messages, which will be super helpful during debugging.
- Consider using
clazz.getDeclaredFields()to include private fields in your search, as it's more comprehensive thanclazz.getFields(). - Don't forget to check for required annotations like
@Componentbefore creating instances in your Injector class. - Catching a general exception like
ReflectiveOperationExceptioncan make your code cleaner and easier to manage. - When rethrowing exceptions, it's a good practice to include the original exception to maintain the stack trace.
- Lastly, be careful with the initialization of your Interface Implementations map to avoid confusion and redundancy.
Keep up the good work, and with a few tweaks, your code will be in great shape! 👍 Remember, these changes are important to ensure that your code is robust and maintainable. You're doing great, and I'm sure you'll have it polished up in no time! 😊
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| static { | ||
| interfaceImplementations = | ||
| Map.of(FileReaderService.class, FileReaderServiceImpl.class, | ||
| ProductParser.class, ProductParserImpl.class, | ||
| ProductService.class, ProductServiceImpl.class | ||
| ); | ||
| } |
There was a problem hiding this comment.
Make Interface Implementations map a class field. You can fill it in using Map.of(). However, it should be initialized in a static block or directly upon declaration, not in both places.
| if (!clazzImplementation.isAnnotationPresent(Component.class)) { | ||
| throw new RuntimeException("Class implementation: " | ||
| + clazzImplementation.getName() | ||
| + "doesn't have 'Component' annotation "); |
There was a problem hiding this comment.
When throwing an exception, add an informative message to it. Also, include the caught exception in the RuntimeException to provide a stack trace for easier debugging. For example: throw new RuntimeException("Injection failed, missing @Component annotation on the class " + clazzImplementation.getName(), e);
| field.setAccessible(true); | ||
| field.set(clazzImplementationInstance, fieldTypeObject); | ||
| } catch (IllegalAccessException e) { | ||
| throw new RuntimeException("Can't initialize field: " + field.getName()); |
There was a problem hiding this comment.
When rethrowing the exception, you should include the original exception as a cause to maintain the stack trace. For example: throw new RuntimeException("Can't initialize field: " + field.getName(), e);
| Object newInstance = constructor.newInstance(); | ||
| instances.put(clazzImplementation, newInstance); | ||
| return newInstance; | ||
| } catch (ReflectiveOperationException e) { |
There was a problem hiding this comment.
It is better to replace many exceptions that have a common parent with a general parental exception. In this case, you can catch ReflectiveOperationException instead of listing all reflective exceptions separately.
| } | ||
| } | ||
|
|
||
| private Class<?> findImplementation(Class<?> clazz) { |
There was a problem hiding this comment.
Don't forget about annotations checks in your Injector class. You should check if the class has the @component annotation before attempting to create an instance.
| + clazzImplementation.getName() | ||
| + "doesn't have 'Component' annotation "); | ||
| } | ||
| Field[] fields = clazzImplementation.getFields(); |
There was a problem hiding this comment.
You should use clazz.getDeclaredFields() instead of clazz.getFields() to also include private fields in your search, as getFields() only returns public fields including inherited ones.
| if (clazz.isInterface()) { | ||
| return interfaceImplementations.get(clazz); | ||
| } | ||
| throw new RuntimeException("Unsupported type class: " + clazz.getName()); |
There was a problem hiding this comment.
This exception message could be more informative by explaining that the class is not an interface. For example: throw new RuntimeException("Expected an interface but got class: " + clazz.getName());
|
I corrected my hometask |
No description provided.