Modify

Opened 5 months ago

Closed 5 months ago

Last modified 5 months ago

#17763 closed enhancement (wontfix)

Redundant code with isMultipolygon()

Reported by: GerdP Owned by: team
Priority: minor Milestone:
Component: Core Version:
Keywords: Cc:

Description

The method isMultipolygon() is implemented in IPrimitive (return false) and (only) IRelation overrides it:

@Override
default boolean isMultipolygon() {
    return "multipolygon".equals(get("type")) || 
}

Still, we have many places where something like

e.osm instanceof Relation && ((Relation) e.osm).isMultipolygon()

is used instead of

e.osm.isMultipolygon()

What is the reason for this double check? It seems to be a waste of time and space as the java compiler doesn't remove the redundant check.
Even worse seems this code in MultipolygonCache:

    private static boolean isMultipolygon(OsmPrimitive p) {
        return p instanceof Relation && ((Relation) p).isMultipolygon();
    }

I work on a patch to remove all this redundant code, please stop me if I missed something.

Attachments (0)

Change History (12)

comment:1 Changed 5 months ago by GerdP

Oops, forgot that instanceof also is a "not null" check, so the replacement code would be

e.osm != null && e.osm.isMultipolygon()

comment:2 Changed 5 months ago by Don-vip

Milestone: 19.05
Priority: normalminor
Type: taskenhancement

comment:3 Changed 5 months ago by GerdP

Maybe a problem occurs when the primitive is an instance of RelationData (which implements IRelation).
So, the instanceof check cannot be simply removed but the cast is not needed.
BTW: I fear I've written a lot of code which would doesn't work with an IRelation that is not also a Relation :(

comment:4 Changed 5 months ago by stoecker

My guess: These checks come from a time, where isMultipolygon() was only implemented in Relation?

comment:5 Changed 5 months ago by GerdP

It was implemented in OsmPrimitive and Relation before r13667. Anyway, my idea doesn't work well. The instanceof check is in fact needed in many places because the code would crash with RelationData. Open question is if that code works at all when the primitves are not instances of OsmPrimitve?
Example: ConditionFactory.closed() will always return false when e.osm is not a Way or Relation

        static boolean closed(Environment e) { // NO_UCD (unused code)
            if (e.osm instanceof Way && ((Way) e.osm).isClosed())
                return true;
            return e.osm instanceof Relation && ((Relation) e.osm).isMultipolygon();
        }

So I have the gut feeling that the changes in r13810 were not yet used to render non-OSM data.

comment:6 in reply to:  5 Changed 5 months ago by Don-vip

Replying to GerdP:

I have the gut feeling that the changes in r13810 were not yet used to render non-OSM data.

It was. The use case was to render atlas files with atlas plugin. But I didn't check all JOSM possibilities, maybe some parts like this one are not working.

comment:7 Changed 5 months ago by Don-vip

In 15122/josm:

see #17763 - use interfaces where possible

comment:8 Changed 5 months ago by GerdP

Seems that atlas data doesn't use multipolygons?

comment:9 Changed 5 months ago by Don-vip

The data model is different, I don't remember how MPs are handled. Do you want to do something more in this ticket, or can we close it?

comment:10 Changed 5 months ago by GerdP

Resolution: fixed
Status: newclosed

I think my proposed changes would not improve much and may cause trouble, so yes, let's close it. I think wontfix is correct then?

comment:11 Changed 5 months ago by GerdP

Resolution: fixedwontfix

comment:12 Changed 5 months ago by Don-vip

Milestone: 19.05

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain team.
as The resolution will be set.
The resolution will be deleted.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.