htsjdk icon indicating copy to clipboard operation
htsjdk copied to clipboard

Some improvements on CigarOperator enum.

Open vruano opened this issue 5 years ago • 2 comments

Description

  • Clean up a bit the code.
    • Removed per value switches.
    • String, character initialization.
  • Optimize some operations
    • Fast look-up of values from primitive codes.
    • Static to member methods to generate primitive codes.
  • Deprecated methods and update their uses to new ones.
  • Clear most of the warnings on the main and test code.

Things to think about before submitting:

  • [ ] Make sure your changes compile and new tests pass locally.
  • [ ] Add new tests or update existing ones:
    • A bug fix should include a test that previously would have failed and passes now.
    • New features should come with new tests that exercise and validate the new functionality.
  • [ ] Extended the README / documentation, if necessary
  • [ ] Check your code style.
  • [ ] Write a clear commit title and message
    • The commit message should describe what changed and is targeted at htsjdk developers
    • Breaking changes should be mentioned in the commit message.

vruano avatar Jan 01 '20 20:01 vruano

Codecov Report

Merging #1448 into master will decrease coverage by 0.001%. The diff coverage is 86.667%.

@@               Coverage Diff               @@
##              master     #1448       +/-   ##
===============================================
- Coverage     68.396%   68.394%   -0.001%     
+ Complexity      8497      8480       -17     
===============================================
  Files            583       583               
  Lines          34413     34418        +5     
  Branches        5729      5741       +12     
===============================================
+ Hits           23537     23540        +3     
  Misses          8653      8653               
- Partials        2223      2225        +2
Impacted Files Coverage Δ Complexity Δ
src/main/java/htsjdk/samtools/util/CigarUtil.java 30.303% <0%> (ø) 14 <0> (ø) :arrow_down:
...rc/main/java/htsjdk/samtools/BinaryCigarCodec.java 95.652% <100%> (ø) 8 <0> (ø) :arrow_down:
src/main/java/htsjdk/samtools/TextCigarCodec.java 92% <100%> (ø) 11 <0> (ø) :arrow_down:
src/main/java/htsjdk/samtools/CigarOperator.java 90% <87.5%> (-6.923%) 32 <26> (-18)
src/main/java/htsjdk/samtools/BAMFileReader.java 68.466% <0%> (+0.852%) 52% <0%> (+1%) :arrow_up:

codecov-io avatar Jan 01 '20 21:01 codecov-io

I have addressed some the comments and thought of another way to address broader operator categories meaning alignment, indel, clips, etc... It turns out that Alignment, Indel, Clipping, Padding and Skip (N) are disjointed and exhaustive categorization of the operators.

I added a non-public enum for those categories and the boolean final fields for each status accessor (e.g. isAlignment, isIndel... etc) is initialized using a single param in the constructor that indicates the class/category for that operator.

What do you think? @lbergelson @yfarjoun ...

You know what they say ... if you feel you need to ask to do somethings is because you know you shouldn't do it :P

vruano avatar Jan 24 '20 03:01 vruano