Addressing Reviewer's comments

biojava · josemduarte · Oct 29, 2021 · Jul 27, 2021 · Jul 31, 2021 · Aug 3, 2021
commit 02974cd8ee1bedba68d09fc08b2b09de00af82c0
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -35,15 +35,13 @@ BioJava 6.0.0 (future release)
 * Fixed `CRC64Checksum#public void update(byte[] b, int offset, int length)` to use the `length` argument correctly as specified in `java.util.zip.Checksum` interface.
 * In `SubstructureIdentifier`, `StructureName`, `EcodDomain`, `ScopDomain` : `getPdbId()` returns `PdbId` object instead of `String`.
 * Removed `DownloadChemCompProvider.useDefaultUrlLayout` with a more flexible system to provide templated URLs `DownloadChemCompProvider.setChemCompPathUrlTemplate()` and `DownloadChemCompProvider.setServerBaseUrl()`
+* In `Structure` (and `StructureImple`), the accessor methods `String getPdbId()` and `setPdbId(String)` were previously depricated. They were revived in BioJava 6.0.0 but as `PdbId getPdbId()` and `setPdbId(PdbId)` instead.n
 
 ### Added
 * New `keywords` field in `PDBHeader` class, populated by PDB and mmCIF parsers #946
 * OBO parsing now supports multiple altids, #960
 * New class `PdbId` that wrapps a PDB Identifier and handles conversion between current short PDBID format and upcoming extended PDBID format #930
 
-### revived
-* In `Structure` (and `StructureImple`), the accessor methods `String getPdbId()` and `setPdbId(String)` were previously depricated. They were revived in BioJava 6.0.0 but as `PdbId getPdbId()` and `setPdbId(PdbId)` instead.
-
 ### Fixed
 * Correct chain assignment to entities when parsing PDB/mmCIF without entity information (in cases with more than 3 chains per entity) #931
 * Dealing with chain ids correctly when parsing bonds in PDB-format files #943 #929

diff --git a/biojava-structure/src/main/java/org/biojava/nbio/structure/PdbId.java b/biojava-structure/src/main/java/org/biojava/nbio/structure/PdbId.java
@@ -44,19 +44,10 @@ public class PdbId implements Comparable<PdbId>, Serializable{
 	private static final String PDB_0000 = PREFIX_PDB_ + STRING_0000;
 
 	/**
-	 * How the PDB ID output/conversion should go, if possible.
+	 * Controls how the PDB ID output/conversion should go, if possible.
+	 * The default is to try to produce short PDB ID. If failed, produce extended PDB ID.
 	 */
-	public enum Behavior{
-		/**
-		 * Try to produce short PDB ID. If failed, produce extended PDB ID.
-		 */
-		PREFER_SHORT,
-		/**
-		 * Always produce Extended PDB ID.
-		 */
-		PREFER_EXTENDED
-	}
-	private static Behavior defaultShorteningBehaviour = Behavior.PREFER_SHORT;
+	private static final boolean defaultShorteningBehaviour = true;
 
 
 	/**
@@ -66,7 +57,11 @@ public enum Behavior{
 	/**
 	 * A regular expression that matches a PDB ID in the extended format.
 	 */
-	public static final Pattern PATTERN_EXTENDED_PDBID = Pattern.compile("PDB_\\d{4}[1-9]\\p{Alnum}{3}"); 
+	public static final Pattern PATTERN_EXTENDED_PDBID = Pattern.compile("(pdb|PDB)_\\p{Alnum}{8}"); 
+	/**
+/	 * A regular expression that matches an extended PDB ID that is compatible with the short format.
+	 */
+	public static final Pattern PATTERN_SHORTABLE_EXTENDED_PDBID = Pattern.compile("(pdb|PDB)_0000[1-9]\\p{Alnum}{3}"); 
 
 	/**
 	 * Keeps the ID in <b>UPPER CASE</b>, in a <em>reduced</em> form (without the <code>PDB_</code> prefix).
@@ -109,13 +104,13 @@ public static boolean isValidExtendedPdbId(String id) {
 
 	/**
 	 * Checks whether an Extended PDB ID is shortable, <i>assuming it is a valid extended PDB ID</i>.
-	 * If you are not sure the String represents a valid extended PdbId, use {@link #isValidExtendedPdbId(String)} first.
 	 * @see #isValidExtendedPdbId(String)
 	 * @param extendedId the supposedly valid extended PDB ID.
-	 * @return <code>true</code> if <code>extendedId</code> starts with "PDB_0000", <code>false</code> otherwise.
+	 * @return <code>true</code> if <code>extendedId</code> can be shortened 
+	 * (ie. it matches the regular expression "(pdb|PDB)_0000[1-9][a-zA-Z0-9]{3}"), <code>false</code> otherwise.
 	 */
 	public static boolean isShortCompatible(String extendedId) {
-		return extendedId.length() >= 8 && extendedId.substring(0, 8).equals/*IgnoreCase*/(PDB_0000);
+		return PATTERN_SHORTABLE_EXTENDED_PDBID.matcher(extendedId).matches();
 	}
 
 	@Override
@@ -158,17 +153,17 @@ public String getId() {
 
 	/**
 	 * Get a <code>String</code> representation of this PdbId instance, using the <i>passed in</i> behavior.<br>
-	 * @param b when it equals <code>Behavior.PREFER_SHORT</code>, the class will try to produce the short ID whenever possible.
-	 * @return The PdbId in short format if possible and <code>b</code> equals <code>Behavior.PREFER_SHORT</code>, the extended PDB ID form otherwise.
+	 * @param prefereShort when it is <code>true</code>, the class will try to produce the short ID whenever possible.
+	 * @return The PdbId in short format if possible and <code>prefereShort</code> is <code>true</code>, the extended PDB ID form otherwise.
 	 */
-	public String getId(Behavior b) {
-		if (b == Behavior.PREFER_SHORT && isInternalShortCompatible(idCode))
+	public String getId(boolean prefereShort) {
+		if (prefereShort && isInternalShortCompatible(idCode))
 			return internalToShortNoCheck(idCode);
 		return PREFIX_PDB_ + idCode;
 	}
 
 	/**
-	 * Get the PDB Id in the sort format. Throws an exception if the conversion is not possible.<br>
+	 * Get the PDB Id in the short format. Throws an exception if the conversion is not possible.<br>
 	 * Use this method only if you know that this PDB ID is shortable.
 	 * @return the PDB ID in the short format.
 	 * @throws StructureException if the conversion was not possible.
@@ -183,7 +178,7 @@ public String getShortId() throws StructureException{
 
 	/**
 	 * Converts <code>shortId</code> to the PDB ID extended format.
-	 * If <code>shortId</code> is a valid short PDB ID (or XXXX), it would be converted to an extended ID,
+	 * If <code>shortId</code> is a valid short PDB ID, it would be converted to an extended ID,
 	 * if <code>shortId</code> is a valid extended PDB ID, it would be returned in UPPER CASE,
 	 * a {@link StructureException} is thrown otherwise.
 	 * @param shortId the PDB ID to convert to extended format
@@ -210,7 +205,7 @@ public static String toExtendedId(String shortId) throws StructureException{
 	 * @throws StructureException if the conversion was not possible.
 	 */
 	public static String toShortId(String extendedId) throws StructureException{
-		if (isValidExtendedPdbId(extendedId) && isShortCompatible(extendedId)) {
+		if (isShortCompatible(extendedId)) {
 			return extendedId.substring(8).toUpperCase();
 		} else if (isValidShortPdbId(extendedId)) {
 			return extendedId.toUpperCase();

diff --git a/biojava-structure/src/main/java/org/biojava/nbio/structure/StructureIO.java b/biojava-structure/src/main/java/org/biojava/nbio/structure/StructureIO.java
@@ -53,7 +53,7 @@ public class StructureIO {
 					   | chainID
 					   | chainID '_' resNum '-' resNum
 		pdbID         := [1-9][a-zA-Z0-9]{3}
-					   | PDB_[0-9]{4}[1-9][a-zA-Z0-9]{3}
+					   | PDB_[a-zA-Z0-9]{8}
 		chainID       := [a-zA-Z0-9]
 		scopID        := 'd' pdbID [a-z_][0-9_]
 		biol		  := 'BIO:' pdbID [:]? [0-9]+

diff --git a/biojava-structure/src/main/java/org/biojava/nbio/structure/SubstructureIdentifier.java b/biojava-structure/src/main/java/org/biojava/nbio/structure/SubstructureIdentifier.java
@@ -51,7 +51,7 @@
  * 		               | chainID
  * 		               | chainID '_' resNum '-' resNum
  *		pdbID         := [1-9][a-zA-Z0-9]{3}
- *		               | PDB_[0-9]{4}[1-9][a-zA-Z0-9]{3}
+ *		               | PDB_[a-zA-Z0-9]{8}
  * 		chainID       := [a-zA-Z0-9]+
  * 		resNum        := [-+]?[0-9]+[A-Za-z]?
  * </pre>

diff --git a/biojava-structure/src/main/java/org/biojava/nbio/structure/align/util/AtomCache.java b/biojava-structure/src/main/java/org/biojava/nbio/structure/align/util/AtomCache.java
@@ -444,7 +444,7 @@ public String getPath() {
 	 * 		               | chainID
 	 * 		               | chainID '_' resNum '-' resNum
 	 *		pdbID         := [1-9][a-zA-Z0-9]{3}
-	 *		               | PDB_[0-9]{4}[1-9][a-zA-Z0-9]{3}
+	 *		               | PDB_[a-zA-Z0-9]{8}
 	 * 		chainID       := [a-zA-Z0-9]
 	 * 		scopID        := 'd' pdbID [a-z_][0-9_]
 	 * 		resNum        := [-+]?[0-9]+[A-Za-z]?

diff --git a/biojava-structure/src/main/java/org/biojava/nbio/structure/io/PDBFileParser.java b/biojava-structure/src/main/java/org/biojava/nbio/structure/io/PDBFileParser.java
@@ -376,6 +376,7 @@ private void pdb_HEADER_Handler(String line) {
 			try {
 				pdbIdToSet = new PdbId(pdbCode);
 			} catch (IllegalArgumentException e) {
+				logger.info("Malformed (or null) PDB ID {}. setting PdbId to null", pdbCode);
 				pdbIdToSet = null;
 			}
 			structure.setPdbId(pdbIdToSet);

diff --git a/...a-structure/src/main/java/org/biojava/nbio/structure/io/cif/CifStructureConsumerImpl.java b/...a-structure/src/main/java/org/biojava/nbio/structure/io/cif/CifStructureConsumerImpl.java
@@ -870,9 +870,11 @@ public void consumeStruct(Struct struct) {
 
         if (struct.isDefined() && struct.getEntryId().isDefined()) {
             PdbId pdbId;
+            String pdbCode = struct.getEntryId().get(0);
 			try {
-				pdbId = new PdbId(struct.getEntryId().get(0));
+				pdbId = new PdbId(pdbCode);
 			} catch (IllegalArgumentException e) {
+				logger.info("Malformed (or null) PDB ID {}. setting PdbId to null", pdbCode);
 				pdbId = null;
 			}
             pdbHeader.setPdbId(pdbId);
@@ -940,7 +942,7 @@ public void consumeStructRefSeq(StructRefSeq structRefSeq) {
 
             DBRef dbRef = new DBRef();
 
-            dbRef.setIdCode(structRefSeq.getPdbxPDBIdCode().get(rowIndex));  //TODO Shall we change this as well?
+            dbRef.setIdCode(structRefSeq.getPdbxPDBIdCode().get(rowIndex));
             dbRef.setDbAccession(structRefSeq.getPdbxDbAccession().get(rowIndex));
             dbRef.setDbIdCode(structRefSeq.getPdbxDbAccession().get(rowIndex));
             dbRef.setChainName(structRefSeq.getPdbxStrandId().get(rowIndex));

diff --git a/biojava-structure/src/test/java/org/biojava/nbio/structure/TestPdbId.java b/biojava-structure/src/test/java/org/biojava/nbio/structure/TestPdbId.java
@@ -63,11 +63,11 @@ public void testGetIdPrefereShortFormat() {
 		String id;
 
 		pdbId = new PdbId("1abc");
-		id = pdbId.getId(PdbId.Behavior.PREFER_SHORT);
+		id = pdbId.getId(true);
 		assertEquals(id, "1ABC");
 
 		pdbId = new PdbId("PDB_55551abc");
-		id = pdbId.getId(PdbId.Behavior.PREFER_SHORT);
+		id = pdbId.getId(true);
 		assertEquals(id, "PDB_55551ABC");
 	}
 
@@ -77,11 +77,11 @@ public void testGetIdPrefereExtendedFormat() {
 		String id;
 
 		pdbId = new PdbId("1abc");
-		id = pdbId.getId(PdbId.Behavior.PREFER_EXTENDED);
+		id = pdbId.getId(false);
 		assertEquals(id, "PDB_00001ABC");
 
 		pdbId = new PdbId("PDB_55551abc");
-		id = pdbId.getId(PdbId.Behavior.PREFER_EXTENDED);
+		id = pdbId.getId(false);
 		assertEquals(id, "PDB_55551ABC");
 	}
 
@@ -113,7 +113,7 @@ public void testIsExtendedPDBID() {
 		assertTrue(PdbId.isValidExtendedPdbId("PDB_00001abc"), "Didn't accept lower case");
 		assertTrue(PdbId.isValidExtendedPdbId("PDB_00004HHB"), "Didn't accept upper case");
 		assertTrue(PdbId.isValidExtendedPdbId("PDB_22224HHB"), "Didn't accept upper case");
-		assertFalse(PdbId.isValidExtendedPdbId("PDB_AAAA4HHB"), "Accepted wrong format");
+		assertTrue(PdbId.isValidExtendedPdbId("PDB_AAAA4HHB"), "It should accept any 8 alphanumeric values");
 		assertFalse(PdbId.isValidExtendedPdbId("1ABC"), "Accepted short format");
 	}
 
@@ -123,10 +123,8 @@ public void testIsShortCompatible() {
 		assertTrue(PdbId.isShortCompatible("PDB_00004HHB"), "Didn't accept upper case");
 		assertFalse(PdbId.isShortCompatible("1ABC"), "Accepted short format");
 		assertFalse(PdbId.isShortCompatible("PDB_AAAA4HHB"), "Accepted wrong format");
-
-		//Although this is wrong, returning true is the expected behavior of 
-		// this method; because it does NOT validate the passed in string.
-		assertTrue(PdbId.isShortCompatible("PDB_0000XXXXXXXXXXXXX"), "Accepted wrong format");
+		assertFalse(PdbId.isShortCompatible("PDB_0000AHHB"), "Accepted letter (1HHB should pass but AHHB should not pass");
+		assertFalse(PdbId.isShortCompatible("PDB_0000AHHBBBBB"), "should be a valid extended PDB ID");
 	}
 
 	@Test
@@ -139,9 +137,9 @@ public void testToExtendedFormat() {
 			assertEquals(PdbId.toExtendedId("PDB_00001abc"), "PDB_00001ABC");
 		}, "Didn't recognize extended format");
 
-		assertThrows(StructureException.class, () -> {
+		assertDoesNotThrow(() -> {
 			PdbId.toExtendedId("PDB_aaaa1abc");
-		}, "Accepted wrong format");
+		}, "Should accept any 8 alphanumeric values");
 	}
 
 	@Test