-
Notifications
You must be signed in to change notification settings - Fork 104
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
Throws useless exceptions on optional fields #150
Comments
Hi @FraRo96, this is a design decision that was made already somewhere around 2005 and since there hasn't been pressure to change it, that's how it has remained. Surely you are not the first one to ask about this, but nevertheless among the few. In general, using try-catch shouldn't be too tedious in contrast to the fact that you'll most likely need to if-else the null values too. I believe this is also somewhat a matter of taste. With individual values/nulls checking you'd need to repeat the check for every value you are trying to get. So I found it more convenient to wrap sentence handling in a single try-catch and if any of the values that my app requires is missing, then I end up in the catch block.
For me to understand better, could you elaborate how it ruins the threading / execution for you? |
I really appreciate your library and your work, nonetheless it's a bad design choice that could easily be fixed though. Maybe I can open a PR if I have time. I can quote any source supporting the previous statement. It's stated so clearly here: do not use exceptions for control flow. throw is not simply an alternative way of returning a value from a function. It's cpp docs but it's recommended in every language afaik. Regarding specifically Java, and to answer this
Throwing an exception involves creating a stack trace, which is costly. It also creates a software interrupt and performs type check. Here a benchmark about how slow is to throw an exception in Java, doesn't matter if caught or not. As stated in the link, you can annotate the "not print stack trace" option for jvm, but it's boilerplate. It's much better to return null. Also please consider that the only people complaining about this are the one using parser at high frequency. In a real-time application reading nmea at 10 Hz, 10 exceptions per second are heavy. |
While I don't want to start arguing over it, I do must point out that the document you linked also gives good advice on using exceptions and I see many of them supporting the chosen approach. For example in here and here. The section you referred to, also says (emphasis by me):
In my honest opinion, marine-api is not throwing to "return values" and especially not to control program flow. It is simply a signal of runtime error and enables error handling as it is conventional in Java. But yeah, as for some more background on the decision, let's see this from more practical point of view. The |
You consider a runtime error the absence of value, while the protocol accepts such absence (please prove me wrong on this if I am). Therefore, seems that your library is not designed for apps using gnss receivers that, for a bunch of reasons like temporary bad or missing gnss signal, sometimes leave empty some fields of a sentence. So for such an app, marine-api would be usable but cause some troubles giving continue exceptions hitting performance. The rest of the answer, if I interpret well, you're saying it’s a pain to do null checks and not using primitive types, but I think it’s good design instead, that’s why e.g. in Kotlin the primitive types are only in the form of wrapper nullable classes. However, I haven't investigated so far wrapper classes to rule out that their use over primitive types could cause performance issue. |
Why when a field is not present does it throw an exception instead of just assigning null? I looked for mandatory vs optional fields in nmea 0183, looks like there aren't explicit mandatory rules for any field, so why throwing an exception that ruins the whole multi-threaded execution of an app.
The text was updated successfully, but these errors were encountered: