From: Aron Roberts Date: Wed, 1 Jul 2009 01:13:08 +0000 (+0000) Subject: CSPACE-235,CSPACE-237: Fixed a number of minor errors in ID generator and ID part... X-Git-Url: https://git.aero2k.de/?a=commitdiff_plain;h=ffaf72601db99061ab6ebaa61ec8300d40eba28e;p=tmp%2Fjakarta-migration.git CSPACE-235,CSPACE-237: Fixed a number of minor errors in ID generator and ID part test classes, uncovered in part through code coverage reports. (These issues were marked as FIXED earlier; now they're 'more' fixed.) --- diff --git a/services/id/service/src/main/java/org/collectionspace/services/id/AlphabeticIDGenerator.java b/services/id/service/src/main/java/org/collectionspace/services/id/AlphabeticIDGenerator.java index 3083800dd..e3b4cab73 100644 --- a/services/id/service/src/main/java/org/collectionspace/services/id/AlphabeticIDGenerator.java +++ b/services/id/service/src/main/java/org/collectionspace/services/id/AlphabeticIDGenerator.java @@ -26,9 +26,10 @@ // @TODO: Add Javadoc comments -// @TODO: When auto expanding, we'll need to set a maximum length to which the -// generated IDs can grow, likely as an additional parameter to be +// @TODO: When auto expanding, we'll need to allow setting of a maximum length to +// which the generated IDs can grow, likely as an additional parameter to be // passed to a constructor, with a default value hard-coded in the class. +// If that length is exceeded, nextID() should throw an IllegalStateException. // @TODO: Consider handling escaped characters or sequences which represent Unicode // code points, both in the start and end characters of the sequence, and in the initial value. @@ -242,7 +243,7 @@ public class AlphabeticIDGenerator implements IDGenerator { // // See the TODOs at the top of this class for additional // functionality that needs to be implemented. - public synchronized String nextID() { + public synchronized String nextID() throws IllegalStateException { // Get next values for each character, from right to left // (least significant to most significant). @@ -297,10 +298,10 @@ public class AlphabeticIDGenerator implements IDGenerator { return sb.toString(); } - public synchronized boolean isValidID(String value) throws IllegalArgumentException { + public synchronized boolean isValidID(String value) { - if ( value == null || value == "") { - throw new IllegalArgumentException("ID to validate must not be null or empty"); + if (value == null || value.equals("")) { + return false; } Pattern pattern = Pattern.compile(getRegex()); diff --git a/services/id/service/src/main/java/org/collectionspace/services/id/IDGenerator.java b/services/id/service/src/main/java/org/collectionspace/services/id/IDGenerator.java index fa054a769..f9c69b2e6 100644 --- a/services/id/service/src/main/java/org/collectionspace/services/id/IDGenerator.java +++ b/services/id/service/src/main/java/org/collectionspace/services/id/IDGenerator.java @@ -38,14 +38,14 @@ public interface IDGenerator { public String getCurrentID(); // Sets the current value of an ID. - public void setCurrentID(String value); + public void setCurrentID(String value) throws IllegalArgumentException; // Resets an ID to its initial value. public void resetID(); // Returns the next ID in the sequence, and sets // the current value to that ID. - public String nextID(); + public String nextID() throws IllegalStateException; // Validates an ID against a pattern of generated IDs. public boolean isValidID(String value); diff --git a/services/id/service/src/main/java/org/collectionspace/services/id/IDPart.java b/services/id/service/src/main/java/org/collectionspace/services/id/IDPart.java index 25d7265cf..f70c42175 100644 --- a/services/id/service/src/main/java/org/collectionspace/services/id/IDPart.java +++ b/services/id/service/src/main/java/org/collectionspace/services/id/IDPart.java @@ -58,7 +58,7 @@ public abstract class IDPart { } // Sets the current value of the ID associated with this IDPart. - public synchronized void setCurrentID(String value) { + public synchronized void setCurrentID(String value) throws IllegalArgumentException { generator.setCurrentID(value); } diff --git a/services/id/service/src/main/java/org/collectionspace/services/id/NumericIDGenerator.java b/services/id/service/src/main/java/org/collectionspace/services/id/NumericIDGenerator.java index 05ea816bf..c2c6933e4 100644 --- a/services/id/service/src/main/java/org/collectionspace/services/id/NumericIDGenerator.java +++ b/services/id/service/src/main/java/org/collectionspace/services/id/NumericIDGenerator.java @@ -55,6 +55,10 @@ public class NumericIDGenerator implements IDGenerator { // Constructor. public NumericIDGenerator(String initialValue, String maxLength) throws IllegalArgumentException { + + if (initialValue == null || initialValue.equals("")) { + throw new IllegalArgumentException("Initial ID value must not be null or empty"); + } try { long l = Long.parseLong(initialValue.trim()); @@ -69,6 +73,10 @@ public class NumericIDGenerator implements IDGenerator { throw new IllegalArgumentException("Initial ID value must be parseable as a number"); } + if (maxLength == null || maxLength.equals("")) { + throw new IllegalArgumentException("Initial ID value must not be null or empty"); + } + try { this.maxLength = Integer.parseInt(maxLength); } catch (NumberFormatException e) { @@ -90,10 +98,15 @@ public class NumericIDGenerator implements IDGenerator { // @TODO Much of this code is copied from the main constructor, // and may be ripe for refactoring. + + if (value == null || value.equals("")) { + throw new IllegalArgumentException("ID value must not be null or empty"); + } + try { long l = Long.parseLong(value.trim()); if ( l < 0 ) { - throw new IllegalArgumentException("Initial ID value should be zero (0) or greater"); + throw new IllegalArgumentException("ID value should be zero (0) or greater"); } this.currentValue = l; this.initialValue = l; diff --git a/services/id/service/src/main/java/org/collectionspace/services/id/StringIDGenerator.java b/services/id/service/src/main/java/org/collectionspace/services/id/StringIDGenerator.java index 6ad0b94d5..68d62cf20 100644 --- a/services/id/service/src/main/java/org/collectionspace/services/id/StringIDGenerator.java +++ b/services/id/service/src/main/java/org/collectionspace/services/id/StringIDGenerator.java @@ -40,7 +40,7 @@ public class StringIDGenerator implements IDGenerator { public StringIDGenerator(String initialValue) throws IllegalArgumentException { - if ( initialValue == null || initialValue == "") { + if (initialValue == null || initialValue.equals("")) { throw new IllegalArgumentException("Initial ID value must not be null or empty"); } @@ -58,7 +58,7 @@ public class StringIDGenerator implements IDGenerator { } public synchronized void setCurrentID(String value) throws IllegalArgumentException { - if ( initialValue == null || initialValue == "") { + if (value == null || value.equals("")) { throw new IllegalArgumentException("ID value must not be null or empty"); } this.currentValue = value; @@ -72,10 +72,10 @@ public class StringIDGenerator implements IDGenerator { return this.currentValue; } - public synchronized boolean isValidID(String value) throws IllegalArgumentException { + public synchronized boolean isValidID(String value) { - if ( value == null || value == "") { - throw new IllegalArgumentException("ID to validate must not be null or empty"); + if (value == null || value.equals("")) { + return false; } Pattern pattern = Pattern.compile(getRegex()); diff --git a/services/id/service/src/main/java/org/collectionspace/services/id/YearIDGenerator.java b/services/id/service/src/main/java/org/collectionspace/services/id/YearIDGenerator.java index b098dfdce..e6cddab42 100644 --- a/services/id/service/src/main/java/org/collectionspace/services/id/YearIDGenerator.java +++ b/services/id/service/src/main/java/org/collectionspace/services/id/YearIDGenerator.java @@ -66,7 +66,7 @@ public class YearIDGenerator implements IDGenerator { public YearIDGenerator(String initialValue) throws IllegalArgumentException { - if ( initialValue == null || initialValue == "") { + if (initialValue == null || initialValue.equals("")) { throw new IllegalArgumentException("Initial ID value must not be null or empty"); } @@ -92,7 +92,7 @@ public class YearIDGenerator implements IDGenerator { // @TODO This code is copied from the main constructor, // and thus there may be an opportunity for refactoring. - if ( value == null || value == "") { + if (value == null || value.equals("")) { throw new IllegalArgumentException("ID value must not be null or empty"); } @@ -124,8 +124,8 @@ public class YearIDGenerator implements IDGenerator { public synchronized boolean isValidID(String value) throws IllegalArgumentException { - if ( value == null || value == "") { - throw new IllegalArgumentException("ID to validate must not be null or empty"); + if (value == null || value.equals("")) { + return false; } Pattern pattern = Pattern.compile(getRegex()); diff --git a/services/id/service/src/test/java/org/collectionspace/services/id/NumericIDPartTest.java b/services/id/service/src/test/java/org/collectionspace/services/id/NumericIDPartTest.java index a21cd88a2..374fd1474 100644 --- a/services/id/service/src/test/java/org/collectionspace/services/id/NumericIDPartTest.java +++ b/services/id/service/src/test/java/org/collectionspace/services/id/NumericIDPartTest.java @@ -71,7 +71,7 @@ public class NumericIDPartTest extends TestCase { } - public void testresetID() { + public void testResetID() { part = new NumericIDPart("25"); assertEquals("26", part.nextID()); @@ -139,6 +139,17 @@ public class NumericIDPartTest extends TestCase { } + public void testNegativeInitialValue() { + + try { + part = new NumericIDPart("-1"); + fail("Should have thrown IllegalArgumentException here"); + } catch (IllegalArgumentException expected) { + // This Exception should be thrown, and thus the test should pass. + } + } + + public void testIsValidID() { part = new NumericIDPart("1"); @@ -164,6 +175,12 @@ public class NumericIDPartTest extends TestCase { part = new NumericIDPart("1", "3"); assertFalse(part.isValidID("not a parseable long")); + + part = new NumericIDPart("1", "3"); + assertFalse(part.isValidID(null)); + + part = new NumericIDPart("1", "3"); + assertFalse(part.isValidID("")); } diff --git a/services/id/service/src/test/java/org/collectionspace/services/id/StringIDPartTest.java b/services/id/service/src/test/java/org/collectionspace/services/id/StringIDPartTest.java index 8a4b0ebfb..54ef72b89 100644 --- a/services/id/service/src/test/java/org/collectionspace/services/id/StringIDPartTest.java +++ b/services/id/service/src/test/java/org/collectionspace/services/id/StringIDPartTest.java @@ -98,20 +98,15 @@ public class StringIDPartTest extends TestCase { part = new StringIDPart("T"); assertFalse(part.isValidID("TE")); - - } - public void testNullValidationValue() { - - try { - part = new StringIDPart("-"); - assertFalse(part.isValidID(null)); - fail("Should have thrown IllegalArgumentException here"); - } catch (IllegalArgumentException expected) { - // This Exception should be thrown, and thus the test should pass. - } + part = new StringIDPart("-"); + assertFalse(part.isValidID(null)); + part = new StringIDPart("-"); + assertFalse(part.isValidID("")); + } + // @TODO: Add more tests of boundary conditions, exceptions ... } diff --git a/services/id/service/src/test/java/org/collectionspace/services/id/YearIDPartTest.java b/services/id/service/src/test/java/org/collectionspace/services/id/YearIDPartTest.java index 60e4b37cc..90d6bfb75 100644 --- a/services/id/service/src/test/java/org/collectionspace/services/id/YearIDPartTest.java +++ b/services/id/service/src/test/java/org/collectionspace/services/id/YearIDPartTest.java @@ -50,29 +50,60 @@ public class YearIDPartTest extends TestCase { assertEquals(year, part.getCurrentID()); } - -/* + public void testSetCurrentID() { + + part = new YearIDPart("1999"); + part.setCurrentID("1999"); + assertEquals("1999", part.getCurrentID()); + part.setCurrentID("2000"); + assertEquals("2000", part.getCurrentID()); + + } + + public void testSetCurrentIDNullOrEmpty() { + + part = new YearIDPart(); + + try { + part.setCurrentID(null); + fail("Should have thrown IllegalArgumentException here"); + } catch (IllegalArgumentException expected) { + // This Exception should be thrown, and thus the test should pass. + } + + try { + part.setCurrentID(""); + fail("Should have thrown IllegalArgumentException here"); + } catch (IllegalArgumentException expected) { + // This Exception should be thrown, and thus the test should pass. + } + + } + public void testNextID() { - part = new YearIDPart("XYZ"); - assertEquals("XYZ", part.nextID()); + + part = new YearIDPart("1999"); + assertEquals("1999", part.nextID()); + } public void testresetID() { - part = new YearIDPart("."); - assertEquals(".", part.nextID()); + part = new YearIDPart("1999"); + assertEquals("1999", part.nextID()); part.resetID(); - assertEquals(".", part.nextID()); + assertEquals("1999", part.getCurrentID()); } public void testInitialID() { - part = new YearIDPart("-"); - assertEquals("-", part.getInitialID()); + + part = new YearIDPart("1999"); + assertEquals("1999", part.getInitialID()); + } -*/ public void testNullInitialValue() { @@ -109,20 +140,15 @@ public class YearIDPartTest extends TestCase { part = new YearIDPart(); assertFalse(part.isValidID("non-numeric value")); - + + part = new YearIDPart(); + assertFalse(part.isValidID(null)); + + part = new YearIDPart(); + assertFalse(part.isValidID("")); + } - public void testNullValidationValue() { - - try { - part = new YearIDPart(); - assertFalse(part.isValidID(null)); - fail("Should have thrown IllegalArgumentException here"); - } catch (IllegalArgumentException expected) { - // This Exception should be thrown, and thus the test should pass. - } - - } // @TODO: Add more tests of boundary conditions, exceptions ... }