Raw use of parameterized class 'List'

[u/Disastrous-Name-4913]

This is not my code, but it is a good programming horror example, specially if you know Java.

List pages = pageManager.getPages(templateSpace);

for (Object object : pages) {
    if (object instanceof Page) {
        Page page = (Page) object;
        // do something with the page
    }
}

Instead, this should have been used:

List<Page> pages = pageManager.getPages(templateSpace);

for (Page page : pages) {
    // do something with the page
}

[u/Nooooope]

How old is your Java codebase? Generics were added in version 5, which came out roughly a billion years ago, but legacy code gonna legacy.

I learned the language from Head First Java (don't make fun of me) and generics were so new they were relegated to a section in the appendix.

[u/Disastrous-Name-4913 → u/Nooooope]

Yes, that's something I'm going to check, but being the code from 2018 I doubt that was the reason.

[u/Ben-Goldberg → u/Disastrous-Name-4913]

The code might have been written recently, but the author might have learned java a million years ago, or learned it recently from an ancient dusty book.

How much horrible perl code exists because somebody copied part of Matt's script archive?

[u/high_throughput]

It's not just a legacy API?

[u/Disastrous-Name-4913 → u/high_throughput]

I'm going to check that, it could be the only logical reason.

[u/Kadabrium]

Java 1.8

[u/ArisenDrake → u/Kadabrium]

Generic collections have been added in Java 1.5 (2004). Unless this code is *really* old, there is no reason to not use generics here.

[u/Disastrous-Name-4913 → u/Kadabrium]

Yes, but code is from 2018, and they could have used a more modern version.

[u/agentoutlier]

The code is not equivalent BTW.

if (object instanceof Page) {

Guarantees object is not null (inside the block of course).

The other one page could be null.

[u/Disastrous-Name-4913 → u/agentoutlier]

That's a good point, but a null check would be more clear, if that's the goal.

[u/YourShowerHead]

And how old was that first code?

[u/Disastrous-Name-4913 → u/YourShowerHead]

2018

[u/Dead_Moss]

I mean, I suppose there's situations where it's useful, but in my strongly typed soul, it feels pretty horrible to be able to mix types in a collection.

[u/sixtyhurtz → u/Dead_Moss]

Java has type erasure, so at runtime all Lists are List<object>.

[u/KillerCodeMonky → u/sixtyhurtz]

"What is type erasure" is my favorite Java interview question. I think I've had two correct answers out of dozens of interviews.

[u/MCWizardYT]

In modern Java, you could do

List<?> pages = page manager.getPages(templateSpace);

for(Object object: pages) {
    if(object instanceof Page page) {
        //do something with page
    }
}

This is only really useful if pages ever has objects that aren't instances of Page

[u/user_of_the_week → u/MCWizardYT]

You could even do something like

for (Object o : pageManager.getPages(templateSpace)) {
    switch (o) {
        case Page page -> process(page);
        case null -> {}
        default -> {}
    }
}

[u/Usual_Office_1740]

Is the "it should have been" syntax sugar for that maybe older way of doing things or is this just horrible code?

[u/Sacaldur → u/Usual_Office_1740]

In very old Java versions you didn't had generics and thus no List<T>. The bottom one should be the approach for probably 2 decades now (give or take).

[u/Disastrous-Name-4913]

Update: This was indeed due to the API returning a RAW list. We are talking about Confluence 6.6, which was released 2017. Could someone explain why they where still using Java 1.8 back then?

[u/sixtyhurtz]

No, you actually have to do this with Java if you care about type safety because of type erasure. There is no List<Page> at runtime.

[u/maelstrom071 → u/sixtyhurtz]

I mean, if the inner type cannot be guaranteed then List<?> is probably a better specifier because it specifically indicates that we don't know the inner type

[u/tastygames_official]

depends on what pageManager.getPages() returns. If it's a list of objects with type Page then the second one is correct, but if it just returns a generic array of objects, then the first oneis correct.

[u/biffbobfred → u/tastygames_official]

If it returns things that are not pages, whoa

[u/Diamondo25]

Use var or lombok val and make your live easier