Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Make exception message informative in SAMUtils.charToCompressedBase #836
Conversation
ronlevine
self-assigned this
Mar 30, 2017
ronlevine
requested a review
from lbergelson
Mar 30, 2017
|
@lbergelson Please review. |
ronlevine
changed the title from
Make exception message informative to Make exception message informative in SAMUtils.charToCompressedBase
Mar 30, 2017
codecov-io
commented
Mar 30, 2017
•
Codecov Report
@@ Coverage Diff @@
## master #836 +/- ##
===============================================
+ Coverage 64.864% 64.866% +0.001%
- Complexity 7195 7197 +2
===============================================
Files 527 527
Lines 31774 31781 +7
Branches 5424 5424
===============================================
+ Hits 20610 20615 +5
- Misses 9016 9020 +4
+ Partials 2148 2146 -2
Continue to review full report at Codecov.
|
lbergelson
requested changes
Mar 30, 2017
@ronlevine I think that this should be reworked a little. Threading the read name through all of these methods seems like an unnecessary complication. I think it would be better to catch the exceptions in the place that has access to the read name and then rethrow them with added information. I would keep the change to initial throw site to have it output the character as well as the byte value since that's always more useful than just the byte value.
| - * Convert from a byte array containing =AaCcGgTtNn represented as ASCII, to a byte array half as long, | ||
| - * with =, A, C, G, T converted to 0, 1, 2, 4, 8, 15. | ||
| + * Convert from a byte array containing =AaCcGgTtNnMmRrSsVvWwYyHhKkDdBb represented as ASCII, to a byte array half as long, | ||
| + * with fore example, =, A, C, G, T converted to 0, 1, 2, 4, 8, 15. |
| } | ||
| return compressedBases; | ||
| } | ||
| /** | ||
| - * Convert from a byte array with basese stored in nybbles, with =, A, C, G, T represented as 0, 1, 2, 4, 8, 15, | ||
| + * Convert from a byte array with bases stored in nybbles, with fore example,=, A, C, G, T, N represented as 0, 1, 2, 4, 8, 15, |
| * @param compressedOffset Byte offset in compressedBases to start. | ||
| * @return New byte array with bases as ASCII bytes. | ||
| */ | ||
| - public static byte[] compressedBasesToBytes(final int length, final byte[] compressedBases, final int compressedOffset) { | ||
| + public static byte[] compressedBasesToBytes(final int length, final byte[] compressedBases, final int compressedOffset, final String readName) { |
lbergelson
Mar 30, 2017
Contributor
I don't think it 's a great idea to change the signature of this public method to require the read name to produce a better error message in a specific use case. I think it would be better to catch the exception in the calling method and re-throw an exception with the added information.
| @@ -214,17 +218,19 @@ private static byte charToCompressedBaseLow(final int base) { | ||
| case 'b': | ||
| return COMPRESSED_B_LOW; | ||
| default: | ||
| - throw new IllegalArgumentException("Bad byte passed to charToCompressedBase: " + base); | ||
| + throw new IllegalArgumentException("Bad base passed to charToCompressedBaseLow: " + Character.toString((char)base) + " in read: " + readName); |
lbergelson
Mar 30, 2017
Contributor
good idea to output the character equivalent of the byte number, but you probably also want the byte value itself in case its a non-printing or formatting character
| @@ -394,6 +404,7 @@ public static char phredToFastq(final int phredScore) { | ||
| } | ||
| /** | ||
| + * |
|
@lbergelson Reworked the code per your suggestion and responded to the rest of your comments. |
ronlevine commentedMar 30, 2017
Description
Fixes broadinstitute/picard#781.
Added the
baseandreadnames to the exception message inSAMUtils.charToCompressedBase.Checklist