Another WTF in my own code
-
Strangely proud and embarrassed by this line of code from a test GUI I'm using. I like to cast at the minute.
public void showContact(Contact contact)
{
/* eat your heart out, polymorphism! */
((contact instanceof Person) ? (PersonPanel)contactPanel : contactPanel).setContact(contact);
}
It's ugly but functional. I should mention that PersonPanels extend ContactPanels. Still, it tests the instanceof and does the downcast in one line instead of breaking it down into several steps. When I first came back to the code after nearly two years, I had to read the line a few times.
-
-
At the very least, I'd say your naming sucks.. rather than overload setContact(), I would have made a setPerson(Person) method. This is not subtype polymorphism--ContactPanel does not have a setContact(Person) method; that method is specific to PersonPanel.
-
@Gazzonyx said:
Strangely proud and embarrassed by this line of code from a test GUI I'm using. I like to cast at the minute.
It's ugly but functional. I should mention that PersonPanels extend ContactPanels. Still, ...Sorry if my question is dumb, I've never been too good at virtual dispatch. The code looks like Java, where instance methods are always virtual. So, how does the casting into (PersonPanel) possibly change the behavior of that line?
If contactPanel is an instanceof PersonPanel, and PersonPanel overrides setContact of the base ContactPanel class, then the PersonPanel implementation of setContact will always be called, even if contactPanel is declared as a ContactPanel.
So:
ContactPanel contactPanel = new PersonPanel();
contactPanel.setContact(contact); // Will call PersonPanel's setContact
((PersonPanel)contactPanel).setContact(contact); // This will call the exact same method as above.Could you elaborate on what makes your code behave differently? I know there's some black magic with "final" methods, but I always assumed that just made the compiler refuse to allow having a method of the same name in the subclass, rather than acting like non-virtual methods in, say, C++.
-
@Gazzonyx said:
/* eat your heart out, polymorphism! */
No no no! It should be:
// eat your heart out, polymorphism!
-
@Whoa314 said:
@Gazzonyx said:
Strangely proud and embarrassed by this line of code from a test GUI I'm using. I like to cast at the minute.
It's ugly but functional. I should mention that PersonPanels extend ContactPanels. Still, ...how does the casting into (PersonPanel) possibly change the behavior of that line?
It doesn't change what method gets called. But in Java, casts throw an exception if the object isn't an instance of the right type. So it will throw an exception if contactPanel is not an instance of PersonPanel. The OP said that this is test code, and in test code you often want to check objects are the right type. I'm not sure if that's what the OP is trying to do, or if the OP has just misunderstood Java. (The comment suggests the latter).
Note that in Java (and C), the result of the ?: operator has a type. It's not a different type depending on which branch got evaluated, it has to be the same type for both branches. The compiler will choose ContactPanel for this type, since PersonPanel can be converted to ContactPanel. So the compiler's going to expand that line of code as:
public void showContact(Contact contact)
{
/* eat your heart out, polymorphism! */
((contact instanceof Person) ? (ContactPanel)(PersonPanel)contactPanel : contactPanel).setContact(contact);
}Here's how I'd write the function, keeping the behaviour exactly the same:
public void showContact(Contact contact)
{
if ((contact instanceof Person) && !(contactPanel instanceof PersonPanel))
/* TODO: Document why we throw ClassCastException here... */
throw ClassCastException();
contactPanel.setContact(contact);
}
-
@random999 said:
It doesn't change what method gets called. But in Java, casts throw an exception if the object isn't an instance of the right type.
You're right; for some reason I thought this would be calling a setContact(Person) method, but contact is never cast. That being said, if you were going to do something awful like this, the ClassCastException should be in PersonPanel's setContact(Contact) method, not duplicated in all calling code.
-
@morbiuswilters said:
At the very least, I'd say your naming sucks.. rather than overload setContact(), I would have made a setPerson(Person) method. This is not subtype polymorphism--ContactPanel does not have a setContact(Person) method; that method is specific to PersonPanel.
Java has the@Overrideannotation for that. If you specify it and muck something up with the polymorphism, it won't compile. Obviously, this annotation is optional, so if you don't specify it, you can happily create all sorts of havoc.
-
@morbiuswilters said:
You're right; for some reason I thought this would be calling a setContact(Person) method, but contact is never cast. That being said, if you were going to do something awful like this, the ClassCastException should be in PersonPanel's setContact(Contact) method, not duplicated in all calling code.
It can't. It's making sure nobody sets Person to plain ContactPanel (or any other non-PersonPanel type of it), which PersonPanel, not being involved, can't do.
It still looks like loosing information somewhere up the call stack just to painfully reconstruct and check it down the line. If Person contacts can only be placed in PersonPanel and other contacts in other panels, the interface probably should have maintained the difference all the way through.
-
@Bulb said:
It can't. It's making sure nobody sets Person to plain ContactPanel (or any other non-PersonPanel type of it), which PersonPanel, not being involved, can't do.
Goddammit. I meant that the CastClassException should be in ContactPanel's setContact(Contact) method*. In my defense, this code is retarded enough to keep tripping me up and I was in a hurry when I wrote my response.
- And before someone jumps down my throat, I'm not saying that's a good idea. Just that it's better than repeating that shit all over the place.
-
@Severity One said:
@morbiuswilters said:
At the very least, I'd say your naming sucks.. rather than overload setContact(), I would have made a setPerson(Person) method. This is not subtype polymorphism--ContactPanel does not have a setContact(Person) method; that method is specific to PersonPanel.
Java has the@Overrideannotation for that. If you specify it and muck something up with the polymorphism, it won't compile. Obviously, this annotation is optional, so if you don't specify it, you can happily create all sorts of havoc.How does @Override apply here? The problem is that Person should not be a subtype of Contact because Person cannot be used everywhere Contact is used--specifically in a ContactPanel.
-
<font face="Courier New">((contact instanceof Person) ? (PersonPanel)contactPanel : contactPanel).setContact(contact);</font>
Really ugly code. What if contactPanel is NOT a PersonPanel? You get an exception.
If ContactPanel has a mthod setContact, and PersonPanel does not, then there is no need for the cast at all. Just write contactPanel.setContact(contact).
Otherwise, PersonPanel overrides the setContact method for a reason; so it is still appropriate to call contactPanel.setContact(contact).
The object contactPanel knows whether it is a PersonPanel or a contactPanel, and so calls the method appropriately. If I don't see ANY need for the above code at all.
-
@dogbrags said:
What if contactPanel is NOT a PersonPanel? You get an exception.
That's the point.
@dogbrags said:
Otherwise, PersonPanel overrides the setContact method for a reason; so it is still appropriate to call contactPanel.setContact(contact).
Unless ContactPanel's setContact(Contact) method does not work with a Person argument because somebody really fucked up the definition of Person. Then you would want to be sure that a Person never got passed to ContactPanel.setContact(Contact), perhaps by using some sort of cast to PersonPanel which would throw a ClassCastException. (We don't know whether the inverse applies--if PersonPanel.setContact(Contact) cannot take a non-Person argument.)
-
@morbiuswilters said:
@Severity One said:
Quite frankly, I don't know how it would apply. It probably doesn't. A delegator pattern would possibly have been more suited here, although I'm inclined to think that the entire design sucks from the ground up.@morbiuswilters said:
At the very least, I'd say your naming sucks.. rather than overload setContact(), I would have made a setPerson(Person) method. This is not subtype polymorphism--ContactPanel does not have a setContact(Person) method; that method is specific to PersonPanel.
Java has the@Overrideannotation for that. If you specify it and muck something up with the polymorphism, it won't compile. Obviously, this annotation is optional, so if you don't specify it, you can happily create all sorts of havoc.How does @Override apply here? The problem is that Person should not be a subtype of Contact because Person cannot be used everywhere Contact is used--specifically in a ContactPanel.
-
@Severity One said:
...although I'm inclined to think that the entire design sucks from the ground up.
That was my conclusion.
-
@Gazzonyx said:
Strangely proud and embarrassed by this line of code from a test GUI I'm using. I like to cast at the minute.
public void showContact(Contact contact)
{
/* eat your heart out, polymorphism! */
((contact instanceof Person) ? (PersonPanel)contactPanel : contactPanel).setContact(contact);
}It's ugly but functional. I should mention that PersonPanels extend ContactPanels. Still, it tests the instanceof and does the downcast in one line instead of breaking it down into several steps. When I first came back to the code after nearly two years, I had to read the line a few times.
If this i C# and PersonPanels setContract was new instead of override and contactPanel is a ContactPanel, then this code would have an effect. Even in that case your still doing it wrong; fix the declaration or the design, whichever is appropriate.
If this is some other language, it is either broken or supports a similar iheritance style to C#'s "new" inheritance.
Without additonal context, i can't see any good reason to do this; i.e. why hide this method this way, is there other code that calls contactPanel.setContact(contact) and expects ContactPanel::setContact() and not PersonPanel::setContact()?
-
I think you guys are reading too much into this code. I should add that a Person is a Contact (we have contacts that are not People). All the original code is doing is updating the GUI according to which type of Contact is being displayed. If it's a Contant and not a Person we call ContactPanel's setContact instead of PersonPanel's. I should probably rename the personPanel so that it's less confusing.
Here's the appropriate code snippets:
public class ContactPanel extends JPanel
{
[...]
public void setContact(Contact contact)
{
setCustomerID(contact.customerID);
setShipID(contact.shipID);
setPhone(contact.phone);
setPhoneExt(contact.phoneExt);
setTollFree(contact.tollFree);
setTollFreeExt(contact.tollFreeExt);
setFax(contact.fax);
setCell(contact.cell);
setEmail1(contact.email1);
setEmail2(contact.email2);
setEmail3(contact.email3);
setDepartment(contact.department);
setNotes(contact.getNotes());
}}
public class PersonPanel extends ContactPanel
{
[...]
@Override
public void setContact(Contact contact)
{
super.setContact(contact);
if (contact instanceof Person)
{
setContactName(((Person)contact).name);
setTitle(((Person)contact).title);
Integer id = ((Person)contact).ID;
setID(id.toString());
}
}}
Is this not a reasonable application of DRY, inheritence and polymorphism?
-
@Gazzonyx said:
Is this not a reasonable application of DRY, inheritence and polymorphism?
Then why the cast to PersonPanel? It seems like you wanted to prevent ContactPanel.setContact(Contact) from getting a Person object, but according to your code that works just fine.
-
@morbiuswilters said:
@Gazzonyx said:
Is this not a reasonable application of DRY, inheritence and polymorphism?
Then why the cast to PersonPanel? It seems like you wanted to prevent ContactPanel.setContact(Contact) from getting a Person object, but according to your code that works just fine.
D'oh! I just reread my code. That check in the PersonPanel was just defensive programming; I completely forgot that it was there. I feel foolish now.
-
@Gazzonyx said:
@morbiuswilters said:
@Gazzonyx said:
Is this not a reasonable application of DRY, inheritence and polymorphism?
Then why the cast to PersonPanel? It seems like you wanted to prevent ContactPanel.setContact(Contact) from getting a Person object, but according to your code that works just fine.
D'oh! I just reread my code. That check in the PersonPanel was just defensive programming; I completely forgot that it was there. I feel foolish now.
Well, the check in PersonPanel.setContact(Contact) allows either a Person object or a Contact object to be safely handled by that method. However, in the OP your cast to PersonPanel does the opposite: it prevents a ContactPanel from getting a Person object. But according to your code, that should work fine.
Anyway, seems like the whole thing was a brainfart.
-
