Martin Mucha has posted comments on this change. Change subject: core: refactored gathering violations to separate class ......................................................................
Patch Set 47: (9 comments) https://gerrit.ovirt.org/#/c/37154/47/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/FailingValidationResults.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/FailingValidationResults.java: Line 10: Line 11: import org.apache.commons.lang.StringUtils; Line 12: import org.ovirt.engine.core.common.errors.VdcBllMessages; Line 13: Line 14: public class FailingValidationResults<T> { > 1. Why do you need generic here? Isn't the violatingEntity always a String? 1. I don't think this is class which should be used just for one validator. Having 'violating entity' designed as String seems absurd to me, since we cannot know, who will use this class and how will process the violations. Avoiding future effort of someone in mapping strings back to entities, need to generifying this class back to current state, or even deciding not to use this class just because of 'wrong' violating entity type, I'm designing this class as generic one. It's not big deal; due to type erasure binaries will be just same. Here we're talking about 8 extra chars per this class instantiation compared to reduced class usability. It's not worth it to save 8 chars and reduce reusability of class. (it's more reusable this way, and it will cost us extra time to create less reusable code) 2. renamed to 'ValidationsWithViolatingEntities' Done. Line 15: protected static final String VIOLATING_ENTITIES_LIST_FORMAT = "${0}_LIST {1}"; Line 16: Line 17: Line 18: private Map<ValidationResult, List<T>> validationResultToViolationCauseMap = new HashMap<>(); Line 14: public class FailingValidationResults<T> { Line 15: protected static final String VIOLATING_ENTITIES_LIST_FORMAT = "${0}_LIST {1}"; Line 16: Line 17: Line 18: private Map<ValidationResult, List<T>> validationResultToViolationCauseMap = new HashMap<>(); > maybe- validationResultToViolatingEntity (why do you need to specify it is renamed to 'validationResultToViolatingEntities' (plural) Done Line 19: Line 20: public Collection<ValidationResult> failingValidationResults() { Line 21: return validationResultToViolationCauseMap.keySet(); Line 22: } Line 16: Line 17: Line 18: private Map<ValidationResult, List<T>> validationResultToViolationCauseMap = new HashMap<>(); Line 19: Line 20: public Collection<ValidationResult> failingValidationResults() { > A method should start with a verb- getFailingValidationResults() Done Line 21: return validationResultToViolationCauseMap.keySet(); Line 22: } Line 23: Line 24: /** Line 32: if (validationResult.isValid()) { Line 33: return false; Line 34: } Line 35: Line 36: List<T> violationCauses = getCausesList(validationResult); > You can use the MultiValueMapUtils.addToMap(..) instead. It cannot be easily transformed, how you suggest, since original code expected non-null values in causes list. But Done. I'll fix that as well.Done. Line 37: Line 38: if (violatingEntity != null) { Line 39: violationCauses.add(violatingEntity); Line 40: } Line 40: } Line 41: return true; Line 42: } Line 43: Line 44: //TODO MM: use helper. > Please move it to your TODO list. The code cannot be merged with this uncle MultiValueMapUtils.addToMap you suggested above. It's not that easy however. Todo removed. Done. Line 45: private List<T> getCausesList(ValidationResult validationResult) { Line 46: List<T> violationCauses = validationResultToViolationCauseMap.get(validationResult); Line 47: if (violationCauses == null) { Line 48: violationCauses = new ArrayList<>(); Line 41: return true; Line 42: } Line 43: Line 44: //TODO MM: use helper. Line 45: private List<T> getCausesList(ValidationResult validationResult) { > Maybe- getViolatingEntities removed altogether.Done. Line 46: List<T> violationCauses = validationResultToViolationCauseMap.get(validationResult); Line 47: if (violationCauses == null) { Line 48: violationCauses = new ArrayList<>(); Line 49: validationResultToViolationCauseMap.put(validationResult, violationCauses); Line 55: return addViolation(validationResult, null); Line 56: } Line 57: Line 58: public boolean addViolation(VdcBllMessages vdcBllMessage) { Line 59: return addViolation(new ValidationResult(vdcBllMessage), null); > Please call- addViolation(vdcBllMessage, null) not used, removing. Done. Line 60: } Line 61: Line 62: public boolean addViolation(VdcBllMessages vdcBllMessage, T violatingEntity) { Line 63: return addViolation(new ValidationResult(vdcBllMessage), violatingEntity); Line 72: Line 73: return !failingValidationResults.validationResultToViolationCauseMap.isEmpty(); Line 74: } Line 75: Line 76: public Map<VdcBllMessages, List<T>> getMessageToCausesMap() { > Also, the translation to a map with the message as a key is really bug bug this is the implication of 'currently' religion. Well currently, there's no ValidationResult being passed through this method, which has set replacements, so map 'cannot' get smaller like you said. This is why I (usually) say, that 'currently' is not important at all and why 'currently' is actually a set trap for our future selves. this is ± how original code looked like (check it up). I did not want to touch it although I knew about this problems. I'm fixing it now, ± throwing away original implementation. Done. Line 77: Map<VdcBllMessages, List<T>> result = new HashMap<>(validationResultToViolationCauseMap.size()); Line 78: for (Map.Entry<ValidationResult, List<T>> entry : validationResultToViolationCauseMap.entrySet()) { Line 79: ValidationResult validationResult = entry.getKey(); Line 80: VdcBllMessages vdcBllMessage = validationResult.getMessage(); Line 89: return result; Line 90: Line 91: } Line 92: Line 93: public List<String> translateToViolationMessages() { > I you translation you're ignoring the variableReplacements. this is ± how original code looked like (check it up). I did not want to touch it although I knew about this problems. I'm fixing it now, ± throwing away original implementation.Done Line 94: Map<VdcBllMessages, List<T>> messageToCausesMap = getMessageToCausesMap(); Line 95: List<String> violationMessages = new ArrayList<>(messageToCausesMap.size() * 2); Line 96: for (Map.Entry<VdcBllMessages, List<T>> entry : messageToCausesMap.entrySet()) { Line 97: VdcBllMessages vdcBllMessage = entry.getKey(); -- To view, visit https://gerrit.ovirt.org/37154 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: If92e3476ad490d5bd5957358ad69be6ace4fd4fa Gerrit-PatchSet: 47 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Martin Mucha <mmu...@redhat.com> Gerrit-Reviewer: Alona Kaplan <alkap...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Mucha <mmu...@redhat.com> Gerrit-Reviewer: Moti Asayag <masa...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
