JvanKatwijk / dab-cmdline

DAB decoding library with example of its use
GNU General Public License v2.0
57 stars 29 forks source link

Formal things #93

Closed 1337Misom closed 1 year ago

1337Misom commented 1 year ago

Hi, After looking around in your code i noticed you used throw and catch with different integers and printing the errors directly into stderr. Wouldn't it be more practical to actually define exceptions which will then be thrown? Because if you were just too lazy i would maybe start creating exceptions. Another question also is do you use any code formatter for the code or is it just your code style?

JvanKatwijk commented 1 year ago

well, the "library approach" has a history. About 6 (maybe 7) years ago, there were some people asking for a command line rather than a graphical interface. So, about 6 years ago, I took the - then existing - sources of qt-dab and removed the dependencies on Qt. In 2 or 3 iterations there was the library. Then people started asking on how to use it, so I made some example programs, in the end even with a python interface. I made essentially only one larger change to the library, since then, rather than having to submit a number of callback functions as separate parameters, I created a single structure to be passed as parameter. (I added some support for e.g. Pluto and line devices, but the device support is almost the same as for e.g. the eti handler, and for some other local programs,

So, the code seems to work, but can be considered "old-fashioned". One of the more serious things that is missing is the handling of changes in the configuration, the BBC seems to apply quite a number of changes in the configuration dynamically. (the same applied to the "eti" handler, that software is from about the same period).

Wrt to the code srtyle I am not completely consistent with the style (especially naming of entities is not consistent over time. The lay out is however, especially spacing is impotant for readability

Wrt exceptions One of the few places (I think) where I use exceptions is with the instantiations of a device handler. As far as I can see, the most important message from this kind of exceptions is: be careful, the device interface does not work In some other software I map (for some devices) the number returned to a real error message describing the error.

best jan

Op ma 15 mei 2023 om 21:56 schreef 1337Misom @.***>:

Hi, After looking around in your code i noticed you used throw and catch with different integers and printing the errors directly into stderr. Wouldn't it be more practical to actually define exceptions which will then be thrown? Because if you were just too lazy i would maybe start creating exceptions. Another question also is do you use any code formatter for the code or is it just your code style?

— Reply to this email directly, view it on GitHub https://github.com/JvanKatwijk/dab-cmdline/issues/93, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACCPHQE4LMZKA6HJJMJJVYDXGKC5NANCNFSM6AAAAAAYCVA2CM . You are receiving this because you are subscribed to this thread.Message ID: @.***>

-- Jan van Katwijk

1337Misom commented 1 year ago

Ok so i will now add specific exceptions into my fork and if you want i can then create a pr with the changes.

1337Misom commented 1 year ago

I now have added simple exceptions and fixed a typo (example 1) in my fork. If you are ok with the changes tell me and i will create a pr. Also i replaced all the fprintf(stderr,...) with a DEBUG_PRINT which can be turned on inside device-handler.h.

JvanKatwijk commented 1 year ago

apology for the delay. Youn are always welcome to do a PR best jan

Op wo 17 mei 2023 om 18:01 schreef 1337Misom @.***>:

I now have added simple exceptions and fixed a typo in my fork. If you are ok with the changes tell me so that i can create a pr.

— Reply to this email directly, view it on GitHub https://github.com/JvanKatwijk/dab-cmdline/issues/93#issuecomment-1551665467, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACCPHQAJJVDH52CQEQZYIADXGTY7FANCNFSM6AAAAAAYCVA2CM . You are receiving this because you commented.Message ID: @.***>

-- Jan van Katwijk

1337Misom commented 1 year ago

Created PR #94 further discussions should be made in the PR.