Migrate guava Function/Predicate to java

[Hanmac]

These functions are Legacy:

import com.google.common.base.Function;
import com.google.common.base.Predicate;

use these instead

import java.util.function.Function;
import java.util.function.Predicate;

If possible, try to remove the usage of anonymous classes with lambda expressions and/or method references instead of classes.

[Jetz72]

I think we could also get guava's Optionals and Suppliers. Was already planning to do "anonymous -> lambda -> method reference" wherever possible in the follow up to #5703. I can include this as well.

[Hanmac]

@Jetz72 I think an easy thing to refactor could be to remove the CardPredicates.Accessors and replace fnGetNetPower with Card::getNetPower

and because of these Functional Interfaces, it shouldn't make a difference

[Jetz72]

So I tried inlining the items in CardPredicates.Accessors as method references, and that worked great. Then the next thing on my agenda was the anonymous to lambda to method reference cleanup. But it turns out a lot of the static ones are in the same boat as the accessors - it just made more sense to have PaperCard::getName in the code than PaperCard.FN_GET_NAME, since the latter is just an extra step in getting to PaperCard::getName. So I tried inlining all the static Function fields, and that went well for all but a couple ones that were already a bit messy to begin with (e.g. DeckProxy). I've since started on some of the other functional interface types and I'm starting to see the same pattern again.

So now it's a question of how far to go with this. A huge chunk of methods in CardPredicates reduce to method references and could be cleanly inlined. With the addition of some simple helper methods on Card, I think most of the remaining ones could be simplified the same way.

This would also simplify the process of migrating from the old Guava libraries to Java ones. I tried having the IDE do it automatically but it had very little success. Guava's Predicate and Function types are used explicitly in many places, and to change one you often have to change a large web of them. There are also a couple points where they're fed back into other Guava classes that ask for Guava predicates specifically. Lambdas and method references are much more flexible and aren't specific to a single functional interface. In many cases already I've been able to cut out the import statement entirely.

Does this seem like a good direction to proceed in?

[Hanmac]

Yeah, small steps if able ... like removing the Accessors if able is a good start

For the others, some functions that feed into Guava functions can be used for Lambdas or method references. Like CardLists.filter that feeds into Iterables.filter can be used with method references. If we later update CardLists.filter to use stream filter instead, then we don't need to change places where it is used.

[Hanmac]

@Jetz72 maybe for next cleanup, or I try this separate:

but this line https://github.com/Card-Forge/forge/blob/master/forge-game/src/main/java/forge/game/card/CardLists.java#L354 could be rewritten to this:

public static CardCollection filter(Iterable<Card> cardList, Predicate<Card> filt) {
    return StreamSupport.stream(cardList, false).filter(filt).collect(Collectors::toCollection(CardCollection::new));
}

or like this:

public static CardCollection filter(Collection<Card> cardList, Predicate<Card> filt) {
    return cardList.stream().filter(filt).collect(Collectors::toCollection(CardCollection::new));
}

Because the Guava Predicate extends from the base java Predicate, we could change how the filter function works without breaking anything? (also i need to benchmark this, but i think this could be implemented as parallel?)

It might be a good idea to remove all the Iterable types when it should return a Immutable? See this #3397

[Jetz72]

Some kind of reworking of FCollection and its related classes seems like a good idea. I believe it currently breaks from the interface's contract in a few ways, and that leaves me a bit hesitant whenever I'm doing these cleanups around it. I originally did an IDE-assisted sweep for cases of list.indexOf(x) != -1 as part of the ongoing cleanup, but stopped and rolled it all back after I saw this comment: https://github.com/Card-Forge/forge/blob/c0230d1cb937e1366774cf492ba628e7ac7f0519/forge-game/src/main/java/forge/game/card/CardView.java#L559

If we can push the uses of Iterables over to java's streams, that'd be a nice dependency to drop (if a bit more wordy). But since the IDE inspections make assumptions about collection classes, we'd want to be careful around those ones unless they can be patched up.

[Hanmac]

i saw some more uses with Iterables.any and Iterables.all sprinkled in the code ...

it might be a good idea to wrap them in our own functions, so we can switch the underlying code easier 🤔

[github-actions[bot]]

This issue has not been updated in a while and has now been marked as stale. Stale messages will be auto closed.