Mike Kolesnik has posted comments on this change.
Change subject: engine: Add ReplacementUtils
......................................................................
Patch Set 1: (5 inline comments)
....................................................
File
backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/ReplacementUtils.java
Line 9:
Line 10: /**
Line 11: * The class is used to replace a property defined within a message
with a bounded number of elements.<br>
Line 12: * In addition, if a counter appears in the message, it will be
replaced with the elements size.
Line 13: */
This looks more like method level documentation to me
Line 14: public class ReplacementUtils {
Line 15:
Line 16: private static final int COLLECTION_BOUNDED_SIZE = 5;
Line 17:
Line 20: List<String> names = new ArrayList<String>(size);
Line 21:
Line 22: for (int i = 0; i < size; i++) {
Line 23: names.add(String.format("\t%s\n",
items.get(i).toString()));
Line 24: }
If size exceeds maximum defined size, do you think it would be beneficial to
add "..." string (Ellipsis)?
It's a rather standard symbol that means that text has been omitted, and would
serve as a indicator to the user that the list is not complete
Line 25:
Line 26: return createMessages(collectionName, items, names);
Line 27: }
Line 28:
Line 25:
Line 26: return createMessages(collectionName, items, names);
Line 27: }
Line 28:
Line 29: public static <T extends Nameable> String[]
replaceWithNameableCollection(String collectionName, List<T> items) {
I would call the other method from this one with a list of the "names" and let
the other method handle the logic instead of cloning it.
I think the "hit" (perf/mem) would not be that harsh, but you'll gain the reuse
and cleanliness of code.
Line 30: int size = calculateOutputCollectionSize(items);
Line 31: List<String> names = new ArrayList<String>(size);
Line 32:
Line 33: for (int i = 0; i < size; i++) {
....................................................
File
backend/manager/modules/utils/src/test/java/org/ovirt/engine/core/utils/ReplacementUtilsTest.java
Line 9: import org.junit.Test;
Line 10: import org.ovirt.engine.core.common.businessentities.Nameable;
Line 11: import org.ovirt.engine.core.common.businessentities.VM;
Line 12:
Line 13: public class ReplacementUtilsTest {
What about tests for empty collection and for two or more items?
What about testing that the list of items exceeds the maximum defined?
Line 14:
Line 15: static private final String PROPERTY_NAME = "MY_SINGLE_ITEM_LIST";
Line 16: static private final String PROPERTY_VALUE =
"MY_SINGLE_ITEM_VALUE";
Line 17: static private final String PROPERTY_COUNTER_NAME =
"MY_SINGLE_ITEM_LIST_COUNTER";
Line 24: }
Line 25:
Line 26: @Test
Line 27: public void replaceWithNameableCollection() {
Line 28: VM vm = new VM();
I would use an anonymous implementation of nameable here
Line 29: vm.setVmName(PROPERTY_VALUE);
Line 30: List<Nameable> items = Collections.<Nameable>
singletonList(vm);
Line 31: String[] messageItems =
ReplacementUtils.replaceWithNameableCollection(PROPERTY_NAME, items);
Line 32: validateMessageItems(messageItems, items);
--
To view, visit http://gerrit.ovirt.org/10723
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia9d30047f090057561bd1a3e16f3edb4e724bd8d
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Moti Asayag <masa...@redhat.com>
Gerrit-Reviewer: Mike Kolesnik <mkole...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
