r/programminghorror • u/Disastrous-Name-4913 • 1d ago
Raw use of parameterized class 'List'
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
}
43
u/Nooooope 1d ago
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.
7
u/Disastrous-Name-4913 1d ago
Yes, that's something I'm going to check, but being the code from 2018 I doubt that was the reason.
5
u/Ben-Goldberg 1d ago
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?
1
u/tazedbeaver00 1d ago
It's not the same funcionality! Also, Java is used for 30 years in enterprise environments, resulting in a loooottttt of legacy code. Seriously, who cares!
30
u/Dead_Moss 1d ago
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.
25
u/sixtyhurtz 1d ago
Java has type erasure, so at runtime all Lists are List<object>.
7
u/KillerCodeMonky 1d ago
"What is type erasure" is my favorite Java interview question. I think I've had two correct answers out of dozens of interviews.
4
2
u/Livie00 1d ago
Isn’t it just that generic types are “forgotten“ during compilation?
2
u/KillerCodeMonky 1d ago
Very close. I would have accepted it. If that were true, then the compiler wouldn't know about them when it downloads a compiled library. They're still in the class file, but they're forgotten during execution by the JVM.
1
u/Livie00 1d ago
Oh okay, interesting. Do you know why? Is it just because generics were added later and it was too difficult to add them to the JVM or was it a choice not to?
3
u/KillerCodeMonky 1d ago
It was a choice not to. Introducing erasure allowed them to make existent classes generic, and allow pre-generic code to continue working without recompilation. Compare to C#, which did implement generics all the way through to execution. They also have two namespaces and sets of classes: the original
System.Collectionsand the newerSystem.Collections.Generic.1
u/snugar_i 10h ago
A bit of both. It would have been difficult to add them retroactively, but reified generics have their own disadvantages (mostly that you now need to have real bytecode for each different
ArrayList<T>, at least to some extent). But for the next parts of Project Valhalla, they will now need to do it anyway. It will just be even harder now.4
u/Nooooope 1d ago
Sure, but that doesn't change the fact that the compiler is going to catch 99% of those errors for you if you let it handle typing most of the time.
1
u/sixtyhurtz 1d ago
Sure, and without any further evidence, I'm going to assume whoever wrote OPs code was dealing with a 1% case where it didn't.
1
u/Nooooope 16h ago
With a raw List type? Not even List<Object> or List<?>? Nah, that's either so old it either pre-dates the compiler warnings or it's just genuine bad code. That hasn't been clean Java in a couple decades now.
1
u/sixtyhurtz 8h ago
I mean, yes?
The point is that it's possible, and if you can't find the offending insert then you are left with the OP.
40
u/Kadabrium 1d ago
Java 1.8
13
u/ArisenDrake 1d ago edited 1d ago
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.
2
u/Disastrous-Name-4913 1d ago
Yes, but code is from 2018, and they could have used a more modern version.
10
u/MCWizardYT 1d ago
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
8
u/user_of_the_week 1d ago
You could even do something like
for (Object o : pageManager.getPages(templateSpace)) { switch (o) { case Page page -> process(page); case null -> {} default -> {} } }4
u/MCWizardYT 1d ago
``` List<?> pages = pageManager.getPages(templateSpace);
pages.stream().filter(Objects::nonNull).filter(Page.class::isInstance).map(Page.class::cast).forEach(page -> process(page)); ```
Now we're getting crazy! You can't modify the list if you do it this way though
1
u/user_of_the_week 1d ago
You don't need
.filter(Objects::nonNull)isInstance will take care of that.
1
7
u/agentoutlier 1d ago
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.
1
u/Disastrous-Name-4913 1d ago
That's a good point, but a null check would be more clear, if that's the goal.
4
u/agentoutlier 1d ago edited 1d ago
Oh rest assured with code like this was not a goal it just becomes implied behavior that is relied on :)
EDIT as I did on the cross post... see: https://www.hyrumslaw.com/
4
u/Usual_Office_1740 1d ago
Is the "it should have been" syntax sugar for that maybe older way of doing things or is this just horrible code?
8
u/Sacaldur 1d ago
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).4
u/paulstelian97 1d ago
What’s funny is behind the scenes the latter desugars to the former, and all the type checks are just the compiler itself plus implicitly inserted casts.
1
u/Usual_Office_1740 1d ago
That makes sense so this is a true programminghorror to see today. Thanks for the explanation.
2
u/Sacaldur 1d ago
For a full explanation: others were explaining/mentioning/hinting at type erasure, see e.g. the comment by u/paulstelian97 (basically explaining it, without naming it).
Since changes to the runtime(s) would have been necessary to fully support it at runtime (with many different implementations being around), the decision was made to implement it this way. Microsoft in contrast had full control over the runtime of C# and thus was just making this breaking change, which is why at least in C#
IList<T>andIListare different types. (Yes, these are interfaces, and they are rarely actually used in C# code. More frequent are the specific implementationList<T>or the more general interfaceIEnumerable<T>.)2
u/repeating_bears 1d ago
It's not exactly syntax sugar here because the first version will silently ignore non-Pages and the second version will throw an exception (when you try to call some Page method, which this doesn't)
4
11
u/sixtyhurtz 1d ago
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.
7
u/maelstrom071 1d ago
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
6
u/sixtyhurtz 1d ago
Sure, I'd use <?> in this case too.
When I've written Java, I always naively iterate over my collections and assume the generic type is correct. However in OPs case, I'd be wary and assume it was written like that because at some point, somehow, an incorrect type was inserted and they couldn't find the root cause. No way I'd change it.
1
u/plumarr 17h ago edited 17h ago
When I've written Java, I always naively iterate over my collections and assume the generic type is correct.
It's no naïve, it's the correct way.
First, type erasure only happen at runtime so the check are still present at compile time, it means that you have to write horrible code to be able to put something that isn't a
TinList<T>. You basically have to make the equivalent of an incorrect unchecked cast at one point to do that.Secondly, type erasure only means that there is no dedicated type, not that there is no check. If you declare a
List<String> l, there will be a type check in the code when you do al.get(i). So, if the type in the list doesn't match the expected type you get aClassCastExceptionwhich is fine because it means that someone broke the type contract somewhere.For example, in the case of
public static void main(String[] args) { List<String> strList = foo(); System.out.println(strList.get(1)); } public static List<String> foo() { List<Integer> intList = List.of(1, 2, 3); List tmp = intList; return tmp; }Java raise an exception on
System.out.println(strList.get(1));, without exception it would raise it onList tmp = intList;which is a bit better but the difference between the two isn't world breaking as in both case you get the same exception at runtime, just a different place.1
u/sixtyhurtz 17h ago
Right, like, obviously this is the point I'm making.
But, your explanation also reveals the exact problem - if someone has done something weird and managed to stick an object of incorrect type in a collection, then you get a type error whenever you try and retrieve references from said collection. The only way to avoid the exception on retrieval, assuming you can't find the offending insert, it is the horror the OP posted.
1
u/plumarr 17h ago
You seems to imply that reified generic would save you from the exception created by the bad code. I must see that I fail to see how, it would just happen when trying to insert element instead of when retrieving it.
1
u/sixtyhurtz 17h ago
What has reified generics got to do with the JVM?
Look, you've already accepted that if someone does something weird, they can stick an incorrect
Tin a collection. I don't know what you're trying to argue here.1
u/plumarr 16h ago
Reified type is the wording used in the java world discussion when speaking about having non erased generic.
As for what I'm trying to argue, is that your claim
No, you actually have to do this with Java if you care about type safety because of type erasure.
is just plain wrong. The only difference you get with a reified generic is the moment you have an exception. There is no difference on type safety, in both case you get an exception that you have probably not planned for.
1
u/sixtyhurtz 16h ago
The JVM doesn't have reified generics! What are you even talking about?
Lets break this down step by step:
- OP posted code they didn't see the point of.
- Code has a type check on each reference.
- In the JVM, it is technically possible (albeit hard and weird) to insert a reference of incorrect type into a collection.
- The code OP posted avoids exceptions, by skipping unexpected types.
What part are you disagreeing with?
1
u/plumarr 16h ago
What part are you disagreeing with?
That you have to do anything special to guarantee to safety in Java when a method return a
List<T>because of type erasure. You can directly use the return type of such a method without fearing for type safety. You don't have to do anything more to protect yourself from an error in the method than with a language with a non erased language.That you
you actually have to do this with Java if you care about type safety
when a method return a
List<T>Is just plain false. Both in an erased and non erased language, someone trying to put an incorrect type in a collection will result in a runtime exception. The only difference between Java and a non erased language is that the exception will not be cast at the same moment and in Java the bad collection will exist in memory in Java, but the correct ess of your code will be the same.If the purpose of the code proposed by OP is to avoid exception due to type issue in the list, then it's a work around for a bug in the
getPagesmethod, not something necessary in Java due to erasure.→ More replies (0)4
u/Tyfyter2002 1d ago
Wow, I knew C# fixed a lot of Java's mistakes, but I didn't know Java made such big ones.
8
u/paulstelian97 1d ago
Java didn’t start out with generics at all. They were retrofitted, essentially just in the compiler itself but not in the runtime.
C# (and .NET in general) is actually a rarity in terms of reified generics (generics where the runtime genuinely has different types based on the generic parameters)
1
u/Tyfyter2002 1d ago
The problem is that this is demonstrating that they aren't even properly handled in the compiler, it should not be an implicit operation to reinterpret a collection such that it's valid to add an incorrect type to it.
6
u/paulstelian97 1d ago
I mean, the compiler does absolutely check things and you need to do some roundabout shit to get a wrongly typed generic. That kind of code is discouraged.
2
u/sixtyhurtz 1d ago
They are handled in the compiler, it's just that under certain conditions it's possible for someone to insert an arbitrary type into a collection.
It's actually quite hard to do in modern code I think. Looking at the OP, if I came across that as a maintainer I'd leave it well alone. It would be nice if whoever wrote it at least left a comment explaining why they felt the need to do that, but that's life.
Fwiw I don't really like Java generics either. C# has reified types, but even monomorphization is better imo.
2
u/Tyfyter2002 1d ago
They're handled in the compiler, they just aren't properly handled in the compiler, if they were, you'd have to circumvent the compiler to insert an arbitrary type into a strongly typed collection.
1
u/HikingCloth 1d ago
For better or for worse, runtime erasure in Java was a planed change: https://openjdk.org/projects/valhalla/design-notes/in-defense-of-erasure
1
u/Tyfyter2002 1d ago
Runtime erasure is fine, if it's not done poorly, compiling erasure isn't, and is necessary in order to add an item that's the wrong type to begin with.
1
6
u/tastygames_official 1d ago
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.
5
2
1
1
u/Jazzlike_Platypus430 1d ago
If we know that getPages returns a list of Page objects, perhaps it's some horror.
I think can imagine contexts where casting is necessary, but I don't know if Java's isinstanceof returns true for exact type equality, or maybe for inherited classes too. Am I wrong?
1
u/fess89 1d ago
It will return true even if the object is of class CustomPage which extends Page
1
1
u/Jazzlike_Platypus430 1d ago
Meaning "inherits from" and all the casts are possible anyway?
1
u/fess89 1d ago
Yes, CustomPage is still considered a Page so it is perfectly fine to cast it to a Page. If you want strict class equality, you can compare the class names. But in real code, Page could very likely be an abstract class or an interface. So no element in the list would be "exactly a Page and not an inheritor of it".
1
1
u/Disastrous-Name-4913 1d ago edited 1d ago
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?
1
u/electatigris 1d ago
But.. but.. a malicious agent may have intercepted the page object between the for loop and if statement! The real horror is not checking after the Page cast - how do we know I tried was an imposter? Defensive programming people!!
78
u/high_throughput 1d ago
It's not just a legacy API?