JavaDependencyInjection #1001
JavaDependencyInjection #1001
Conversation
There was a problem hiding this comment.
Hey there! It looks like there's a bit more work to do before we can merge this in. 🛠️ While the main parts are looking great, we need to address a few things to align with best practices and the project's requirements. Here's what we need to fix:
- Let's catch
ReflectiveOperationExceptioninstead of multiple separate exceptions to streamline the code. - Consider making the implementations map a class field for better efficiency.
- We should improve the exception messages to be more informative, helping us understand what went wrong and why.
- Don't forget to add a
@Targetannotation to your custom annotations to specify where they can be applied.
Once you've made these changes, we'll be all set to approve this PR. Keep up the good work, you're almost there! 👍
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
|
|
||
| @Retention(RetentionPolicy.RUNTIME) | ||
| public @interface Component { |
There was a problem hiding this comment.
According to the checklist, custom annotations should have a @Target annotation to specify where this annotation can be used. For example, if @Component is meant to be used on classes, you should add @Target(ElementType.TYPE).
There was a problem hiding this comment.
According to the checklist, custom annotations should have a @target annotation to specify where this annotation can be used. For example, if @component is meant to be used on classes, you should add @target(ElementType.TYPE).
| if (!interfaceClazz.isInterface()) { | ||
| throw new RuntimeException("Only interfaces are supported"); |
There was a problem hiding this comment.
The exception message could be more informative. According to the checklist, when throwing an exception, it should contain an informative message. Consider including the type of the class that is not an interface.
| if (implClass == null || !implClass.isAnnotationPresent(Component.class)) { | ||
| throw new RuntimeException("No implementation found for " + interfaceClazz.getName()); |
There was a problem hiding this comment.
The exception message should be more informative and include the reason why the implementation wasn't found. According to the checklist, the message should indicate the missing @Component annotation if that's the case.
| Map<Class<?>, Class<?>> implementations = Map.of( | ||
| ProductService.class, ProductServiceImpl.class, | ||
| ProductParser.class, ProductParserImpl.class, | ||
| FileReaderService.class, FileReaderServiceImpl.class |
There was a problem hiding this comment.
The implementations map should be a class field instead of being created in the findImplementation method. This way, it's only initialized once and can be reused, which is more efficient.
| } catch (ReflectiveOperationException e) { | ||
| throw new RuntimeException("Failed to create instance of " + implClass.getName(), e); |
There was a problem hiding this comment.
Instead of catching multiple exceptions separately, it's recommended to catch their common parent, ReflectiveOperationException, to simplify the code as per the checklist.
|
|
||
| @Retention(RetentionPolicy.RUNTIME) | ||
| public @interface Component { |
There was a problem hiding this comment.
According to the checklist, custom annotations should have a @target annotation to specify where this annotation can be used. For example, if @component is meant to be used on classes, you should add @target(ElementType.TYPE).
| if (interfaceClazz.equals(ProductService.class)) { | ||
| return ProductServiceImpl.class; | ||
| } else if (interfaceClazz.equals(ProductParser.class)) { | ||
| return ProductParserImpl.class; | ||
| } else if (interfaceClazz.equals(FileReaderService.class)) { | ||
| return FileReaderServiceImpl.class; | ||
| } |
|
|
||
| @Retention(RetentionPolicy.RUNTIME) |
| @@ -3,18 +3,12 @@ | |||
| import java.math.BigDecimal; | |||
|
|
|||
| public class Product { | |||
| * @return newly create {@link Product} object based on the `productInfo` input. | ||
| */ | ||
| Product parse(String productInfo); | ||
| Product parse(String line); |
| @@ -4,5 +4,7 @@ | |||
| import mate.academy.model.Product; | |||
|
|
|||
| public interface ProductService { | |||
| List<Product> getAllFromFile(String filePath); | |||
| List<Product> getAllProducts(); | |||
| public class FileReaderServiceImpl implements FileReaderService { | ||
| @Override | ||
| public List<String> readFile(String fileName) { | ||
| try { | ||
| return Files.readAllLines(new File(fileName).toPath()); | ||
| return Files.lines(new File(fileName).toPath()) |
| } catch (IOException e) { | ||
| throw new RuntimeException("Can't read file: " + fileName, e); | ||
| throw new RuntimeException("Can't read file " + fileName, e); |
| product.setDescription(data[DESCRIPTION_POSITION]); | ||
| product.setPrice(new BigDecimal(data[PRICE_POSITION])); | ||
| return product; | ||
| public Product parse(String line) { |
| private FileReaderService fileReaderService; | ||
|
|
||
| @Override | ||
| public List<Product> getAllFromFile(String filePath) { | ||
| return fileReaderService.readFile(filePath) | ||
| public List<Product> getAllProducts() { |
| public Object getInstance(Class<?> interfaceClazz) { | ||
| return null; | ||
| public <T> T getInstance(Class<T> interfaceClazz) { | ||
| Class<?> implClass = interfaceToImplementation.get(interfaceClazz); |
There was a problem hiding this comment.
This is not full implementation os Injector
The convention is to use Component on the classes that are used for instance creation (usually these are interface implementations) and Inject to mark fields that need to be initialized. If the Component annotation is missing above the class, we shouldn't be able to create an instance of this class in Injector, and we should throw an exception
Don't forget about annotations checks in your Injector class.
Let's check instances map before new instance creation.
No description provided.