-
Notifications
You must be signed in to change notification settings - Fork 996
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
completed homework jv-DI #1000
base: main
Are you sure you want to change the base?
completed homework jv-DI #1000
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 following the checklist and ensuring that all files meet the requirements! Your attention to detail is commendable, and the code looks clean and well-structured. Keep up the good work, and remember that small optimizations and informative exception messages can further enhance your code in the future. 👍 Keep coding and stay awesome! 🌟
✨ 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) | ||
public @interface Inject { |
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.
The @Inject
annotation is correctly defined with the @Retention
policy set to RUNTIME
. This is necessary for runtime reflection. No changes needed.
} catch (Exception e) { | ||
throw new RuntimeException("Error initializing injector", 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. In this case, you can catch ReflectiveOperationException
instead of Exception
to be more specific about what you are catching.
} catch (NoSuchMethodException e) { | ||
throw new RuntimeException("Could not find constructor for " | ||
+ clazz.getName(), 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.
When throwing an exception, add an informative message to it. Also, don't forget to add an exception you're catching in the catch
block to the RuntimeException
object you throw. This is implemented correctly here, but ensure to follow this pattern consistently.
} catch (InstantiationException | ||
| IllegalAccessException | ||
| InvocationTargetException e) { | ||
throw new RuntimeException("Could not instantiate " | ||
+ clazz.getName(), 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.
Similar to the previous comment, it is better to replace many exceptions that have a common parent with a general parental exception. In this case, ReflectiveOperationException
can be used instead of listing all individual exceptions.
if (dependency == null) { | ||
throw new RuntimeException("No component found for: " | ||
+ fieldType); |
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. The message should include details about why the injection failed, such as missing @Component
annotation on the class. This helps in debugging the issue.
throw new RuntimeException("Not allowed to inject " | ||
+ fieldType); |
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. The message should include details about why the injection failed, such as missing @Component
annotation on the class. This helps in debugging the issue.
private void registerComponents() { | ||
Class<?>[] classes = { | ||
ProductServiceImpl.class, | ||
ProductParserImpl.class, | ||
FileReaderServiceImpl.class | ||
}; | ||
|
||
for (Class<?> clazz : classes) { | ||
if (clazz.isAnnotationPresent(Component.class)) { | ||
Constructor<?> constructor; | ||
try { | ||
constructor = clazz.getDeclaredConstructor(); | ||
} catch (NoSuchMethodException e) { | ||
throw new RuntimeException("Could not find constructor for " | ||
+ clazz.getName(), e); | ||
} | ||
Object instance = null; | ||
try { | ||
instance = constructor.newInstance(); | ||
} catch (InstantiationException | ||
| IllegalAccessException | ||
| InvocationTargetException e) { | ||
throw new RuntimeException("Could not instantiate " | ||
+ clazz.getName(), e); | ||
} | ||
components.put(clazz, instance); | ||
for (Class<?> element : clazz.getInterfaces()) { | ||
components.put(element, instance); | ||
} | ||
} | ||
} |
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.
Make Interface Implementations map a class field. You can fill it in using Map.of()
. This will make the code cleaner and more maintainable.
public class Injector { | ||
private static final Injector injector = new Injector(); | ||
private final Map<Class<?>, Object> components = 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.
Let's check instances map before new instance creation. This ensures that you do not create a new instance if one already exists, which would be more efficient.
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.
add target
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.
add target
public class Injector { | ||
private static final Injector injector = new Injector(); | ||
private final Map<Class<?>, Object> components = 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.
private final Map<Class<?>, Object> components = new HashMap<>(); | |
private final Map<Class<?>, Object> components = new HashMap<>(); | |
private static final Map<Class<?>, Class<?>> interfaceImplementations; | |
static { | |
interfaceImplementations = Map.of( | |
FileReaderService.class, FileReaderServiceImpl.class, | |
ProductService.class, ProductServiceImpl.class, | |
ProductParser.class, ProductParserImpl.class); | |
} |
Class<?>[] classes = { | ||
ProductServiceImpl.class, | ||
ProductParserImpl.class, | ||
FileReaderServiceImpl.class | ||
}; |
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.
Class<?>[] classes = { | |
ProductServiceImpl.class, | |
ProductParserImpl.class, | |
FileReaderServiceImpl.class | |
}; | |
Class<?> clazz = getImplementation(interfaceClazz); |
where
private static Class<?> getImplementation(Class<?> interfaceClazz) {
if (interfaceClazz.isInterface()) {
return interfaceImplementations.get(interfaceClazz);
}
return interfaceClazz;
}
No description provided.