Skip to content

Fixed Line Length bug - #760 #841

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
r614 wants to merge 1 commit into from
Closed

Fixed Line Length bug - #760 #841

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions biojava-structure/src/main/java/org/biojava/nbio/structure/io/PDBFileParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2002,7 +2002,7 @@ private void switchCAOnly(){

/** safes repeating a few lines ... */
private Integer conect_helper (String line,int start,int end) {
if (line.length() < end) return null;
if (line.length() < start || line.length() < end) return null;
Copy link
Copy Markdown
Member

@lafita lafita Jun 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am a bit confused because for this function I assume that by definition start < end, and therefore checking line.length() < end should be sufficient here. @r614 do you know of a case where this is not true? Maybe the solution would be to check that indeed start < end always and track when this is problematic during parsing.

@Sheraf-DNG could you provide your test case so that we include it as a unit test and make sure it is fixed?

Copy link
Copy Markdown

@Sheraf-DNG Sheraf-DNG Jun 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was over ago, I can't remember it sorry... I'm myself baffled by the solution I proposed :/ but yep there was a StringOutOfBound trrown from this function, and adding this seemed to solve it in my tests...

Copy link
Copy Markdown
Author

@r614 r614 Jun 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lafita I agree with you, but I think it would also depend on how BioJava wants to handle specs.
I think the possible ways you get this error is if:

  1. (start > end ) < line length
  2. end < line length < start
  3. start < line length < end

If any of those cases are possible, or users can use the method then I think adding a checkpoint won't be a bad idea, but if the function is used internally then I don't think it would really matter, considering it passes all tests so far.


String sbond = line.substring(start,end).trim();
int bond = -1 ;
Expand Down Expand Up @@ -3651,4 +3651,4 @@ public FileParsingParameters getFileParsingParameters(){
}


}
}