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

Throws useless exceptions on optional fields #150

Open
FraRo96 opened this issue Nov 8, 2024 · 4 comments
Open

Throws useless exceptions on optional fields #150

FraRo96 opened this issue Nov 8, 2024 · 4 comments

Comments

@FraRo96
Copy link

FraRo96 commented Nov 8, 2024

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.

@FraRo96 FraRo96 changed the title Throw exceptions on optional fields Throws useless exceptions on optional fields Nov 8, 2024
@ktuukkan
Copy link
Owner

ktuukkan commented Nov 26, 2024

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.

throwing an exception that ruins the whole multi-threaded execution of an app

For me to understand better, could you elaborate how it ruins the threading / execution for you?

@FraRo96
Copy link
Author

FraRo96 commented Nov 26, 2024

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

For me to understand better, could you elaborate how it ruins the threading / execution for you?

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.
The "null check if-else" is just 1 clock time more, that is completely scalable with CPU pipelining and speculative execution ie guessing the "if-else" current expression result based on past results and possibly rollback later.

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.

@ktuukkan
Copy link
Owner

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):

Use throw only to signal an error (which means specifically that the function couldn’t do what it advertised, and establish its postconditions).

In particular, do not use exceptions for control flow. throw is not simply an alternative way of returning a value from a function (similar to return)

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 SentenceParser has been defined using native data types in order to avoid the overhead of creating corresponding data type objects (Double, Integer etc). Efficiency is also the reason why all sentence fields are not parsed immediately after reading the data, but mostly individually and on-demand. Java autoboxing/unboxing can handle converting non-String data types if we return null from SentenceParser#getStringValue(int), but in all other non-String getters there is an additional layer of parsing/conversion. For example, we convert String into a double or boolean. If this fails for some reason, be it due to malformed or missing value, an exception will be thrown by Java standard lib methods (eg. NumberFormatException). So there will be exceptions and if we also start returning nulls, you need to take care of both worlds. And because of autoboxing, there will also be some additional things you need to remember as the user of marine-api, for example the null assignment to native type variables and caveats with equality comparisons (object refs vs. values). Hence, I've chosen not to return nulls. But if you feel it's better and benefits everyone, feel free to refactor and open a PR and we'll see it from there.

@FraRo96
Copy link
Author

FraRo96 commented Dec 10, 2024

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.

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

No branches or pull requests

2 participants