engine: Minor refactor to prefer simplified ranged-for-loop

[tcoyvwac]

A minor refactor to reduce codebase size using standard algorithm functions.
(Previously this PR used the C++20 ranged version, but removed it because sadly the Apple Clang buildchain does not support it) 😿

[Swiftb0y]

I appreciate the willingness to refactor, but in this case I'm seriously doubting the usefulness. The resulting code is harder to read and understand for beginners while providing no obvious performance benefit. The only change that would've made sense IMO is to replace the oldschool for loop with index lookup by a ranged-for loop...

[Swiftb0y]

I'm quite a noob on C++20 ranges, so I would've liked to see that implementation, maybe the resulting code would've had better ergonomics.

[daschuer]

I agree with @Swiftb0y.

I think it is worth to eliminate all int i loop counters in our code base that are only used to access the container element. I am also open to do that as a (normally unwanted) mass refactoring, because the change is trivial, easy to review and possible conflicts easy to resolve.

Introducing lambdas is not that trivial. They need a thoroughly review and manual tests. Doing that in a mass refactoring will bind resources we can make better use of, in introducing new features ir fixing bugs.

[tcoyvwac]

I'm quite a noob on C++20 ranges, so I would've liked to see that implementation, maybe the resulting code would've had better ergonomics.

So @Swiftb0y, the ranges version of this PR is still available to view here: 625656d 🔍 | (raw .patch version) 📥
It is more natural, in my humble opinion, than the old-school verbose begin/end iterator version. However It was removed rapidly, as the current CI version used of Apple Clang does not have C++20 ranges yet. 😿

[tcoyvwac]

@daschuer I understand that the project has limited bandwidth to review refactoring PRs especially if it uses the standard library or lambda syntax. I apologise; it was on me to explain the reason for the code in this PR & what the specific lambda does, so to ease the review of the PR. ✔️

---

It is great @daschuer that we are in agreement that: 😸

it is worth to eliminate all int i loop counters in our code base that are only used to access the container element.

As the popular computer science joke goes:

"There are 2 hard problems in computer science: cache invalidation, naming things, and off-by-1 errors".

As you already know, the recommended common practice is by using the standard library, or some ranges form of code, it helps greatly to eliminate the even potential of variable-counter problems, and provides a uniform syntax to express "intent" of code.

From: C++ Core Guidelines

• P.1: Express ideas directly in code
  (Hint: Please note, the "example - bad" code-snippet 😿 near like-for-like matches the form / code previously used in codebase). 🔍 📝

• ES.1: Prefer the standard library to other libraries and to “handcrafted code”

• SL.con.3: Avoid bounds errors

Maybe helpful for @Swiftb0y:

From Sonarsource:

• Code smell fix: STL constrained algorithms with range parameter should be used when iterating over the entire range.

• Code smell fix: STL algorithms and range-based for loops should be preferred to traditional for loops
  • (Please keenly note that the std::algorithm is recommended + preferred even over their acceptable ranged-for-loop version.) 🔍

[Swiftb0y]

Thank you, I think the ranges version is a little easier to understand from a beginners PoV. The only thing that I still dislike about that is the conversion from std::end(m_channels) to nullptr (return (it != std::end(m_channels)) ? (*it)->m_pChannel : nullptr);) As I said, I'm not familiar enough with C++ ranges, but can't we create function or adapter for that?

The links you posted mostly concern themselves with increased complexity from the lack of use of iterators or ranges. While that's certainly the case in more complex code, I don't think the upfront cost of being paid makes sense. Sure, when you have complex filter code, using find_if makes sense, but for a case thats as simple as these ones, I don't think anything trumps the simplicity of a "for-loop; if; return" construct:

for (const ChannelInfo* pChannelInfo : m_channels) {
    if (pChannelInfo->m_pChannel->getGroup() == group) {
        return pChannelInfo->m_pChannel;
    }
}
return nullptr;

[tcoyvwac]

I don't think anything trumps the simplicity of a "for-loop; if; return" construct.

I agree with you a lot @Swiftb0y! 😉
With containers, it is so basic and common, and drives so much software in so many pieces of code!
So much so, that this "construct" was standardised in the C++ language... I am very confident you can guess what it is! 😸

"Possible implementation of std::find_if"

for (; first != last; ++first)
    if (p(*first))
        return first;
return last;

So you see @Swiftb0y, this "for-loop; if; return" construct you demonstrated does have a formal name in C++. That is std::find_if! 😉

std::find_if has been around for a very long time (Year 1989+), and made more streamlined in C++11. So do you wonder if it is wise, when using a C++ container, to let beginners use the pre-1989 C "for-loop; if; return" construct, vs the standardised way of std::find_if.

for (const ChannelInfo* pChannelInfo : m_channels) {
    if (pChannelInfo->m_pChannel->getGroup() == group) {
        return pChannelInfo->m_pChannel;
    }
}
return nullptr;

In your code example @Swiftb0y, one beginner would already:

• understand std::vector (or some sort of standardised container)
• referencing / dereferencing pointers
• predicates; what it is and how to use it

So why teach & continue non-standard coding methods in the project's codebase over the standardised way? I can understand & relate, if one has not used std::find_if often; I think it would be wiser to help beginners upgrade from their very old non-standard method, foster growth, and say "oh snap, that is a std::find_if!"...

To quote: C++ Core Guidelines - P.1: Express ideas directly in code

A C++ programmer should know the basics of the standard library, and use it where appropriate. Any programmer should know the basics of the foundation libraries of the project being worked on, and use them appropriately. [...]

We both agree that the construct is basic and simple, so let's look out for areas where we can use the basics of the standard way in the codebase in C++. 😉

[tcoyvwac]

The only thing that I still dislike about that is the conversion from std::end(m_channels) to nullptr (return (it != std::end(m_channels)) ? (*it)->m_pChannel : nullptr);

I agree with you once more @Swiftb0y 😉 .
Your comment however has activated my trap card question:

• (In this codebase & code-section), what is the purpose of nullptr; what does nullptr supposed to "represent"?

---

In your kind example, there are two code-patterns that seasoned C++ developers can see:

for (const ChannelInfo* pChannelInfo : m_channels) {
    if (pChannelInfo->m_pChannel->getGroup() == group) {
        return pChannelInfo->m_pChannel;
    }
}
return nullptr;

1. The "maybe-type" pattern: if(truthful_predicate) { return constructed_datatype } else {return null-type};

• https://github.com/mixxxdj/mixxx/blob/main/src/engine/enginemaster.cpp#L887-L889
• https://github.com/mixxxdj/mixxx/blob/main/src/engine/enginemaster.cpp#L922-L924
• https://github.com/mixxxdj/mixxx/blob/main/src/soundio/soundmanager.cpp#L408-L414

2. The located-item exist or not-exist pattern: if(in_all_exists(value)) { return true } else {return false };

• https://github.com/mixxxdj/mixxx/blob/main/src/engine/controls/enginecontrol.cpp#L115-L116
• https://github.com/mixxxdj/mixxx/blob/main/src/engine/sync/enginesync.cpp#L501-L504

If one uses S.O.L.I.D principles, this area of code has maybe already violated S for Single-responsibility principle.

The conversion code highlights this awkwardness. that piece of code-block must do above two responsibilities at once. To become some sort of better C++ "standardised" code, the point is to represent some sort of "maybe" state. In C++17 and later, we have an official datatype to represent this state. It is std::optional.

How to use it in the codebase with your current recommended code example... well, that is slightly outside of the scope of this current PR. But it is important down-the-line in making the code more closer to C++ standardisation (of algorithms & datatypes).

[Swiftb0y]

I fully agree with most of the points you make. The only thing I disagree with is that in order to understand std::find_if you don't need to know about the three points you mentioned (std::vector, pointers, predicates). Thus using std::find_if still requires a deeper understanding of C++ than the for of loop (which would be understood by pretty much anyone coming from common beginner languages such as python or js).

I also acknowledge that the design of the entire function and even the class is flawed ( $O(n)$ search to lookup an object by key?!?!).

The biggest problem I have with the proposed change (which also introduces the unwanted complexity) is the intermingling / conversion between the plain pointer-based code and the iterator based code. Unfortunately due to the member function signature, you can't change that without a bigger refactoring. For that reason I prefer the iterator-less function because the introduced shift in paradigm only introduces additional complexity.

[daschuer]

For me the issue is not the use of std::find_if() it is very useful to when initialize a value as const like:
const int result1 = std::find(vector.begin(), vector.end(), value);
It is the use of the lambda and the extra conditional assignment when returning.
You can spot instantly the control flow in an for .. if .. return control flow due to proper indention. This is of a high value but IMHO broken in your proposal.
The other thing is that a mass refactoring for introducing std::find_if() has IMHO not enough benefits for us, compared to the hassle with review and merge conflicts.

[daschuer]

Can you rebase this on main? The Arch linux build failure has been fixed.

[daschuer]

LGTM. Oh, never mind this is only a small commit.

[daschuer]

Thank you.

[tcoyvwac]

Ah, just saw your update. Thanks for merging, grateful! 😸