Skip to content

Conversation

@r614
Copy link

@r614 r614 commented Jun 10, 2019

Added the recommended solution for the line length bug from Issue #760

/** 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
Member

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?

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
Author

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.

@josemduarte
Copy link
Contributor

I'll close this. The conclusion seems to be that this is a private function and does not need extra input validation checks.

@josemduarte josemduarte closed this Sep 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants