MapEnvelope shouldn't waste the hashCode object

[Shryne]

Currently, MapEnvelope (and probably some other classes, for example, CollectionEnvelope) creates a scalar inside of hashCode and calls value() immediately:

cactoos/src/main/java/org/cactoos/map/MapEnvelope.java

Lines 161 to 179 in 7ea22b9

	@Override
	public final int hashCode() {
	return new UncheckedScalar<>(
	new Folded<>(
	42,
	(hash, entry) -> {
	final int keys = new SumOfIntScalar(
	() -> 37 * hash,
	() -> entry.getKey().hashCode()
	).value();
	return new SumOfIntScalar(
	() -> 37 * keys,
	() -> entry.getValue().hashCode()
	).value();
	},
	this.map.value().entrySet()
	)
	).value();
	}

I guess you are desperately trying to hold onto the oop rules, but creating an object inside of a method is also against the rules. You are just misusing the object as a (less readable, less efficient and longer) function. Same applies to equals and partly to toString.

The simplest solution would be to hold the hashCode scalar in an instance variable.
Alternatively, you could also use the traditional, procedural (but shorter) approach inside of hashCode (and equals).

[0crat]

@llorllale/z please, pay attention to this issue

[0crat]

@Shryne/z this project will fix the problem faster if you donate a few dollars to it; just click here and pay via Stripe, it's very fast, convenient and appreciated; thanks a lot!

[llorllale]

@Shryne I'd first like to understand where exactly you think the problem is, whether it's the implementation's complexity or performance.

(To add to the context, see #947 where we discuss the separation of concerns of envelope vs. immutability in collections)

[Shryne]

I've read #947 but I don't think it adds something to this issue.
#892 led to the change of the equals method of MapEnvelope. The reason was that multiple return statements are against the rules according to: https://www.yegor256.com/2015/08/18/multiple-return-statements-in-oop.html

Yes, the new implementation with the scalar fixes that, but now it breaks this rule: Make sure you instantiate everything or almost everything in your secondary constructors

Additionally, it requires the construction of an object on each call. This is unnecessary and it'll probably go against the rule from PMD (and therefore qulice) if these methods are called inside of a loop: "Avoid instantiating new objects inside loops"

Conclusion: I think there are multiple problems:

1. It breaks the "new operator only in secondary constructors"-rule
2. It costs more performance than necessary (and breaks PMD)
3. It's more difficult to read for everybody who isn't used to this style
4. The object is misused as a function call. There is no laziness or anything else that one would normally find using the elegant object style of oop

[llorllale]

@Shryne

1. True, but that's a trade-off we're willing to make, especially for equals and hashcode
2. We haven't measured it. Can you provide some measurements? Also, no PMD rule is broken specifically because of this implementation (no relevant PMD suppressions were required for this)
3. We recommend users get used to the style. :)
4. It is true that we don't defer execution this way, but consider how much more elegant the code looks now than before.

Sounds like maybe you just don't agree with the style.

[llorllale]

@Shryne ping

Should we close this?

[Shryne]

Sry, I forgot about it.

1. Guess I can't say anything new against it.
2. Not a perfect benchmark but the result is similar to what I've expected:

public static void main(final String[] args) {
        // cactoos
        final var first = new MapOf<>(
            new MapEntry<>(1, 2),
            new MapEntry<>(3, 4),
            new MapEntry<>(5, 6)
        );
        final var second = new MapOf<>(
            new MapEntry<>(6, 7),
            new MapEntry<>(8, 9)
        );
        var number = 1_000_000;
        var start = System.currentTimeMillis();
        for (int round = 0; round < number; ++round) {
            first.equals(second);
        }
        System.out.println(System.currentTimeMillis() - start);
        
        // standard java
        final var third = new HashMap<Integer, Integer>(3);
        third.put(1, 2);
        third.put(3, 4);
        third.put(5, 6);
        final var fourth = new HashMap<Integer, Integer>(2);
        fourth.put(6, 7);
        fourth.put(8, 9);

        number = 1_000_000;
        start = System.currentTimeMillis();
        for (int round = 0; round < number; ++round) {
            third.equals(fourth);
        }
        System.out.println(System.currentTimeMillis() - start);
}

version of cactoos: 0.41
result on my computer: below 10 ms for the java version, ~750 ms for cactoos.

About PMD: I didn't mean you're breaking PMD rules inside your project. I meant that if I use the Map from cactoos and call equals, hashCode or toString in a loop, that would go against the AvoidInstantiatingObjectsInLoops rule. At least that was my argument, but now I see that this rule got removed.

3. I see.
4. That comparison is a little bit unfair since the second version is split into the actual equals method and a private helper method (which is also against the rules).

I guess I don't have more to say, so if you aren't convinced, I guess this can be closed.

[llorllale]

@Shryne I want to focus on your point about performance. What sort of application are you building that is being negatively impacted by cactoos' performance?

[Shryne]

I am working on a GUI library and I've already noticed that I need to reduce the number of throwaway objects.

[llorllale]

@Shryne what are the constraints on your resources (platform, RAM, cpu, etc)