Dsp module and unneeded complications

General JUCE discussion
1

I’ve been scratching my head, trying to figure out why this doesn’t compile:

conv.loadImpulseResponse(BinaryData::hall_wav, BinaryData::hall_wavSize, true, true, 0);

It’s because we can’t provide simple bools to isStereo and requiresTrimming.

Turns out someone thought we really needed this:

enum class Stereo    { no, yes };
enum class Trim      { no, yes };
enum class Normalise { no, yes };

Come on, guys…

2

Just my opinion, of course, but to me the enum version is much more readable.

With the Boolean version I’d have no idea what these flags mean by looking at the call site, and it would be extremely easy to mess up the order.

1 Like
3

I don’t disagree, eyal, but given there are already a heap of functions that take multiple boolean arguments, it is a break away from what is essentially a convention in the juce framework. It is odd for it to exist here, imo.

4

juce::dsp::Convolution::Stereo::yes, juce::dsp::Convolution::Trim::yes

is really more readable than

true, true???

5

First of all, @Fandusss - the fact the JUCE used some bad practices in some older classes (including the boolean arguments, using owning raw pointers, etc) does not mean we have to keep it up in newer code.

@pizzafilms first, even the longer version you suggested is much more readable than the boolean version to me.

But - I do agree on the need to save typing, so here’s how I do it:



namespace EA::MyReverb
{
    //importing the commonly used namespace into mine.
    using juce::dsp::Convolution;

    void processReverb()
    {
        conv.loadImpulseResponse(BinaryData::hall_wav, 
                                 BinaryData::hall_wavSize, 
                                 Stereo::yes, 
                                 Trim::yes, 0);
    }
}
2 Likes
6

It is in the way that you can explicitly see what you are saying true/false to. But I’m with you, it’s an unexpected argument format compared to most juce functions. It’s the kinda thing where if you want to do it that way, you do it everywhere, not just in some places which will lead to tripping people up, as it has here.

7

Sorry for the snarknisess, guys. I’ve never really liked the coding style of the dsp module. I’ve always felt it was needlessly clever and over complicated and the one time I try to use it (tonight), it bites me with the difference between Stereo::yes vs true.

I mean, seriously…what C/C++ programmer doesn’t understand true/false?

8

I mean, seriously…what C/C++ programmer doesn’t understand true/false?

Take this function:

doSomething(true, false);

What do the first and second arguments mean? I have no idea.
It’s also very possible I’ll mess up the order (as happened to me many times in functions like setInterceptsMouseClicks() or setResizable()).

In this case adding a few extra characters would make the code easily understandable without going into the definition of the function, and help the compiler detect mistakes. To me that’s a win-win.

As mentioned earlier, the lengthy juce::dsp::Convolution namespace does indeed has a lot of typing and reading boilerplate that adds little additional information when repeating many times, and you can avoid that, but that is an unrelated subject.

1 Like
9

I dislike the change as well, it takes longer to write out than to quickly double check the order from the function itself.

10

So much agreed, which is the reason I’m not using it at all… and btw an enum with two members called Yes and No is indeed just stupid - just my two cents…

11

I think it’s good that JUCE is using the enums here.
I also try to use enums in my own code instead of true/false input.
I think it makes my code more readable.

2 Likes
12

That might be true in case you are writing the code, but how do you do it during code review? I always prefer reviewing code written like that because in a pull requests git diff like you see it during review on GitHub, Bitbucket, GitLab or whatever your team uses you just cannot quickly jump into a function but have to understand code from just reading it before hitting the approve button. This is where expressive two element enums are not stupid but extremely clever to keep code quality high when working with a larger team.

7 Likes
13

I don’t think there’s really a strong argument to be made against the benefits that strong types can provide. So it ultimately comes down to: is the extra typing worth it?

Generally, code is read far more often than it is written. Especially library code like JUCE. Spending a few extra keystrokes to make reading the code easier is an effective use of time. As others have mentioned, there are ways to get the benefits of stronger types without introducing too much verbosity.

If you don’t care about this sort of thing in your own code, it’s easy enough to write some helper functions that take booleans or whatever you like.

1 Like
14

To be clear, I’m not arguing against using strong types, but a 2 member enum that is essentially true or false seems overkill.

15

But what’s the alternative to make both code and behavior easier to read and predict, and in general more expressive? Python solves this very elegantly, but in C++ that’s the best solution imho.
I used the boolean loadImpulseResponse method several times, and when I came back to those lines of code couple of weeks later, I had no clue at all what this battery of bools wants to tell me.
Code readability and expressiveness plays a big role in code quality, so taking the extra seconds of time needed to write it is a very small price to pay for what you get in return.

Have to add: this topic seems to be one of those ‘everyone has an opinion but theres no right or wrong’. We might simply have to trust and submit to the API gods

5 Likes
16

True. Me, I’m strongly agree with both opinions at the same time! :upside_down_face:

1 Like