From a07949da33a4418114603e5ebf9eea4fe53e9475 Mon Sep 17 00:00:00 2001 From: gmungoc Date: Thu, 22 Jun 2017 10:46:39 +0100 Subject: [PATCH] JAL-2480 simpler and safer test to avoid adding duplicates --- src/jalview/datamodel/features/FeatureStore.java | 100 ++++++++++++++------ .../datamodel/features/FeatureStoreTest.java | 99 +++++++++++++++++-- 2 files changed, 161 insertions(+), 38 deletions(-) diff --git a/src/jalview/datamodel/features/FeatureStore.java b/src/jalview/datamodel/features/FeatureStore.java index 84e7736..b082b56 100644 --- a/src/jalview/datamodel/features/FeatureStore.java +++ b/src/jalview/datamodel/features/FeatureStore.java @@ -33,6 +33,13 @@ public class FeatureStore */ abstract boolean compare(SequenceFeature entry); + /** + * serves a search condition for finding the first feature whose start + * position follows a given target location + * + * @param target + * @return + */ static SearchCriterion byStart(final long target) { return new SearchCriterion() { @@ -45,6 +52,13 @@ public class FeatureStore }; } + /** + * serves a search condition for finding the first feature whose end + * position follows a given target location + * + * @param target + * @return + */ static SearchCriterion byEnd(final long target) { return new SearchCriterion() @@ -58,6 +72,13 @@ public class FeatureStore }; } + /** + * serves a search condition for finding the first feature which follows the + * given range as determined by a supplied comparator + * + * @param target + * @return + */ static SearchCriterion byFeature(final ContiguousI to, final Comparator rc) { @@ -159,6 +180,11 @@ public class FeatureStore */ public boolean addFeature(SequenceFeature feature) { + if (contains(feature)) + { + return false; + } + /* * keep a record of feature groups */ @@ -179,16 +205,13 @@ public class FeatureStore } else { - if (!contains(nonNestedFeatures, feature)) + added = addNonNestedFeature(feature); + if (!added) { - added = addNonNestedFeature(feature); - if (!added) - { - /* - * detected a nested feature - put it in the NCList structure - */ - added = addNestedFeature(feature); - } + /* + * detected a nested feature - put it in the NCList structure + */ + added = addNestedFeature(feature); } } @@ -225,6 +248,36 @@ public class FeatureStore } /** + * Answers true if this store contains the given feature (testing by + * SequenceFeature.equals), else false + * + * @param feature + * @return + */ + public boolean contains(SequenceFeature feature) + { + if (feature.isNonPositional()) + { + return nonPositionalFeatures == null ? false : nonPositionalFeatures + .contains(feature); + } + + if (feature.isContactFeature()) + { + return contactFeatureStarts == null ? false : listContains( + contactFeatureStarts, feature); + } + + if (listContains(nonNestedFeatures, feature)) + { + return true; + } + + return nestedFeatures == null ? false : nestedFeatures + .contains(feature); + } + + /** * Answers the 'length' of the feature, counting 0 for non-positional features * and 1 for contact features * @@ -246,10 +299,10 @@ public class FeatureStore /** * Adds the feature to the list of non-positional features (with lazy - * instantiation of the list if it is null), and returns true. If the - * non-positional features already include the new feature (by equality test), - * then it is not added, and this method returns false. The feature group is - * added to the set of distinct feature groups for non-positional features. + * instantiation of the list if it is null), and returns true. The feature + * group is added to the set of distinct feature groups for non-positional + * features. This method allows duplicate features, so test before calling to + * prevent this. * * @param feature */ @@ -259,10 +312,6 @@ public class FeatureStore { nonPositionalFeatures = new ArrayList(); } - if (nonPositionalFeatures.contains(feature)) - { - return false; - } nonPositionalFeatures.add(feature); @@ -362,8 +411,8 @@ public class FeatureStore /** * Add a contact feature to the lists that hold them ordered by start (first * contact) and by end (second contact) position, ensuring the lists remain - * ordered, and returns true. If the contact feature lists already contain the - * given feature (by test for equality), does not add it and returns false. + * ordered, and returns true. This method allows duplicate features to be + * added, so test before calling to avoid this. * * @param feature * @return @@ -379,11 +428,6 @@ public class FeatureStore contactFeatureEnds = new ArrayList(); } - if (contains(contactFeatureStarts, feature)) - { - return false; - } - /* * binary search the sorted list to find the insertion point */ @@ -412,7 +456,7 @@ public class FeatureStore * @param feature * @return */ - protected static boolean contains(List features, + protected static boolean listContains(List features, SequenceFeature feature) { if (features == null || feature == null) @@ -562,7 +606,7 @@ public class FeatureStore * Scans the list of non-nested features, starting from startIndex, to find * those that overlap the from-to range, and adds them to the result list. * Returns the index of the first feature whose start position is after the - * target range (or the length of the whole list if none such feature exists). + * target range (or the length of the whole list if no such feature exists). * * @param startIndex * @param from @@ -581,9 +625,7 @@ public class FeatureStore { break; } - int start = sf.getBegin(); - int end = sf.getEnd(); - if (start <= to && end >= from) + if (sf.getBegin() <= to && sf.getEnd() >= from) { result.add(sf); } diff --git a/test/jalview/datamodel/features/FeatureStoreTest.java b/test/jalview/datamodel/features/FeatureStoreTest.java index f5be818..db21c2f 100644 --- a/test/jalview/datamodel/features/FeatureStoreTest.java +++ b/test/jalview/datamodel/features/FeatureStoreTest.java @@ -394,12 +394,12 @@ public class FeatureStoreTest /* * add contact */ - SequenceFeature sf5 = new SequenceFeature("Disulfide bond", "", 0, 0, + SequenceFeature sf5 = new SequenceFeature("Disulfide bond", "", 10, 20, Float.NaN, null); assertTrue(fs.addFeature(sf5)); assertEquals(fs.getFeatureCount(true), 2); // positional - add 1 for contact assertEquals(fs.getFeatureCount(false), 1); // non-positional - SequenceFeature sf6 = new SequenceFeature("Disulfide bond", "", 0, 0, + SequenceFeature sf6 = new SequenceFeature("Disulfide bond", "", 10, 20, Float.NaN, null); assertFalse(fs.addFeature(sf6)); // already stored assertEquals(fs.getFeatureCount(true), 2); // no change @@ -690,16 +690,16 @@ public class FeatureStoreTest } @Test(groups = "Functional") - public void testContains() + public void testListContains() { - assertFalse(FeatureStore.contains(null, null)); + assertFalse(FeatureStore.listContains(null, null)); List features = new ArrayList(); - assertFalse(FeatureStore.contains(features, null)); + assertFalse(FeatureStore.listContains(features, null)); SequenceFeature sf1 = new SequenceFeature("type1", "desc1", 20, 30, 3f, "group1"); - assertFalse(FeatureStore.contains(null, sf1)); - assertFalse(FeatureStore.contains(features, sf1)); + assertFalse(FeatureStore.listContains(null, sf1)); + assertFalse(FeatureStore.listContains(features, sf1)); features.add(sf1); SequenceFeature sf2 = new SequenceFeature("type1", "desc1", 20, 30, 3f, @@ -708,8 +708,8 @@ public class FeatureStoreTest "group1"); // sf2.equals(sf1) so contains should return true - assertTrue(FeatureStore.contains(features, sf2)); - assertFalse(FeatureStore.contains(features, sf3)); + assertTrue(FeatureStore.listContains(features, sf2)); + assertFalse(FeatureStore.listContains(features, sf3)); } @Test(groups = "Functional") @@ -827,4 +827,85 @@ public class FeatureStoreTest assertEquals(pos.get(1).getBegin(), 13); assertEquals(pos.get(1).getEnd(), 22); } + + @Test(groups = "Functional") + public void testDelete_readd() + { + /* + * add a feature and a nested feature + */ + FeatureStore store = new FeatureStore(); + SequenceFeature sf1 = addFeature(store, 10, 20); + // sf2 is nested in sf1 so will be stored in nestedFeatures + SequenceFeature sf2 = addFeature(store, 12, 14); + List features = store.getPositionalFeatures(); + assertEquals(features.size(), 2); + assertTrue(features.contains(sf1)); + assertTrue(features.contains(sf2)); + assertTrue(store.nonNestedFeatures.contains(sf1)); + assertTrue(store.nestedFeatures.contains(sf2)); + + /* + * delete the first feature + */ + assertTrue(store.delete(sf1)); + features = store.getPositionalFeatures(); + assertFalse(features.contains(sf1)); + assertTrue(features.contains(sf2)); + + /* + * re-add the 'nested' feature; is it now duplicated? + */ + store.addFeature(sf2); + features = store.getPositionalFeatures(); + assertEquals(features.size(), 1); + assertTrue(features.contains(sf2)); + } + + @Test(groups = "Functional") + public void testContains() + { + FeatureStore fs = new FeatureStore(); + SequenceFeature sf1 = new SequenceFeature("Cath", "", 10, 20, + Float.NaN, "group1"); + SequenceFeature sf2 = new SequenceFeature("Cath", "", 10, 20, + Float.NaN, "group2"); + SequenceFeature sf3 = new SequenceFeature("Cath", "", 0, 0, Float.NaN, + "group1"); + SequenceFeature sf4 = new SequenceFeature("Cath", "", 0, 0, 0f, + "group1"); + SequenceFeature sf5 = new SequenceFeature("Disulphide Bond", "", 5, 15, + Float.NaN, "group1"); + SequenceFeature sf6 = new SequenceFeature("Disulphide Bond", "", 5, 15, + Float.NaN, "group2"); + + fs.addFeature(sf1); + fs.addFeature(sf3); + fs.addFeature(sf5); + assertTrue(fs.contains(sf1)); // positional feature + assertTrue(fs.contains(new SequenceFeature(sf1))); // identical feature + assertFalse(fs.contains(sf2)); // different group + assertTrue(fs.contains(sf3)); // non-positional + assertTrue(fs.contains(new SequenceFeature(sf3))); + assertFalse(fs.contains(sf4)); // different score + assertTrue(fs.contains(sf5)); // contact feature + assertTrue(fs.contains(new SequenceFeature(sf5))); + assertFalse(fs.contains(sf6)); // different group + + /* + * add a nested feature + */ + SequenceFeature sf7 = new SequenceFeature("Cath", "", 12, 16, + Float.NaN, "group1"); + fs.addFeature(sf7); + assertTrue(fs.contains(sf7)); + assertTrue(fs.contains(new SequenceFeature(sf7))); + + /* + * delete the outer (enclosing, non-nested) feature + */ + fs.delete(sf1); + assertFalse(fs.contains(sf1)); + assertTrue(fs.contains(sf7)); + } } -- 1.7.10.2