Skip to content
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.

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

VCFHeader class design is unsafe #250

Open
droazen opened this issue May 13, 2015 · 2 comments
Open

VCFHeader class design is unsafe #250

droazen opened this issue May 13, 2015 · 2 comments
Assignees
Labels
bug VCFHeader Refactor Issues fixed by the header/line refactoring branch

Comments

@droazen
Copy link
Contributor

droazen commented May 13, 2015

The current version of the VCFHeader class maintains a master list of header lines (mMetaData) plus type-specific lookup tables for each kind of header line. This design is unsafe and a source of bugs (we've already seen one!), as the various views into the header data are prone to getting out of sync, and the various accessor methods are not consistent in the views they access (some rely on the master list of header lines, some on the type-specific tables).

We should refactor this so that each header line is only stored in one place. Since one of the operations we need to support is getMetaDataInInputOrder() we can't just use the type-specific maps, but we could get rid of the maps and just store everything in mMetaData. This would come at a performance cost when accessing header lines by key or by type, but would improve safety and simplify the code a great deal. Since header operations typically scale by the number of files rather than the number of records, performance should not trump safety in this case.

@james-d
Copy link
Contributor

james-d commented May 18, 2015

Just a comment: implementations of the type-specific (and key-based) accessor methods (getContigLines(), etc) will be substantially cleaner (and potentially quicker) using Java 8 API. Since it is relatively close (< 5 months) to the projected end of support for Java 7 in this project, perhaps it makes sense to wait until then to change the implementation.

@lbergelson
Copy link
Member

Everything will be better once java 8 is here!

@cmnbroad cmnbroad self-assigned this Mar 8, 2017
@yfarjoun yfarjoun added the bug label Apr 1, 2019
yfarjoun pushed a commit to yfarjoun/htsjdk that referenced this issue Jun 5, 2019
@cmnbroad cmnbroad added the VCFHeader Refactor Issues fixed by the header/line refactoring branch label Sep 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug VCFHeader Refactor Issues fixed by the header/line refactoring branch
Projects
None yet
Development

No branches or pull requests

5 participants