[GitHub] [fineract] vidakovic commented on a diff in pull request #2623: FINERACT-1694 Categorization for Events

[GitBox <gi...@apache.org>]

vidakovic commented on code in PR #2623:
URL: https://github.com/apache/fineract/pull/2623#discussion_r982662129


##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/event/business/domain/BulkBusinessEvent.java:
##########
@@ -30,4 +30,9 @@ public BulkBusinessEvent(List<BusinessEvent<?>> value) {
     public String getType() {
         return "BulkBusinessEvent";

Review Comment:
   I know, a bit picky, but could we add somewhere `static final String` variables with these kind of values (if in doubt then put it in class `BulkBusinessEvent.java` as a private variable. Like it is now the string will always be created new when the function `getType()` is called. Having a variable is also a good practice in terms of avoiding "magic strings".
   
   Another suggestion: why not use `BulkBusinessEvent.class.getSimpleName()` (="BulkBusinessEvent")? In that case this whole thing would be immune against refactoring errors...
   
   If I were you I would do this:
   
   ```
   public class BulkBusinessEvent imiplements BusinessEvent {
       private static final String TYPE = getClass().getSimpleName();
   
   ...
   
       @Override
       public String getType() {
           return TYPE;
       }
   
   ...
   
   }
   ```
   
   And finally: as the function `getType()` is required by all `BusinessEvent` classes you could create an abstract class and move above code there. Benefits:
   - only have to write this code once
   - completely type-safe
   - avoiding all magic strings (aka refactoring works like a charm)
   - ... and memory usage is a tiny bit better (no "new strings" on every call to `getType()`)
   
   This could look like this:
   
   ```
   public class AbstractBusinessEvent imiplements BusinessEvent {
       protected static final String TYPE = getClass().getSimpleName();
   
   ...
   
       @Override
       public String getType() {
           return TYPE;
       }
   
   ...
   
   }
   
   public class BulkBusinessEvent extends AbstractBusinessEvent {
   
   ...
   
   }
   ```
   
   



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org