-
Notifications
You must be signed in to change notification settings - Fork 998
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
implemented annotations Inject and Component #1019
base: main
Are you sure you want to change the base?
implemented annotations Inject and Component #1019
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job on implementing the task requirements! While there are some minor improvements suggested, they don't impact the core functionality. Keep up the good work and consider the feedback for future enhancements! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
import java.lang.annotation.Retention; | ||
import java.lang.annotation.RetentionPolicy; | ||
|
||
@Retention(RetentionPolicy.RUNTIME) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should add @Target
to the @Component
annotation as per the checklist requirement: 'Do not forget to add @Target
to custom annotations.'
import java.lang.annotation.Retention; | ||
import java.lang.annotation.RetentionPolicy; | ||
|
||
@Retention(RetentionPolicy.RUNTIME) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You forgot to add @Target
to the @Inject
annotation. According to the checklist, it is necessary to specify the target for custom annotations.
public class Injector { | ||
private static final Injector injector = new Injector(); | ||
private Map<Class<?>, Object> instances = new HashMap<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is recommended to make the Interface Implementations map a class field and fill it using Map.of() as per the checklist.
throw new RuntimeException("No implementation found for interface: " | ||
+ interfaceClazz.getName()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When throwing an exception, add an informative message to it. Also, don't forget to add the exception you're catching in the catch block to the RuntimeException object you throw.
throw new RuntimeException("Can't initialize field value. Class: " | ||
+ clazz.getName() + ". Field: " + field.getName()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When throwing an exception, add the caught exception to the RuntimeException object you throw to provide more context.
} catch (NoSuchMethodException | InvocationTargetException | ||
| InstantiationException | IllegalAccessException e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is better to replace many exceptions that have a common parent with a general parental exception, such as ReflectiveOperationException.
return instance; | ||
} catch (NoSuchMethodException | InvocationTargetException | ||
| InstantiationException | IllegalAccessException e) { | ||
throw new RuntimeException("Can't create a new instance of " + clazz.getName()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When throwing an exception, add the caught exception to the RuntimeException object you throw to provide more context.
No description provided.