Strange code in mkgmap?

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

Strange code in mkgmap?

UliBaer
Hi there,

examining the code of mkgmap i found some strange lines.

In /mkgmap/src/uk/me/parabola/util/ShapeSplitter.java Line 330:
	if (thisLine.highPoint == endEnclosed && thisLine.highPoint == endEnclosed) // consider carefully
This is redundant?

In /mkgmap/src/uk/me/parabola/mkgmap/osmstyle/housenumber/HousenumberGenerator.java Line 1537+1538:
			if (d != 0)
				return d;
This was already tested 6 lines above, so this cannot occur - Dead code?

In /mkgmap/src/uk/me/parabola/mkgmap/filters/ShapeMergeFilter.java Line 554:
			if (n1 == n2)
				return 0;
This is a String comparison, shouldn't n1.equals(n2) be used instead?

Just some thoughts of mine browsing the code - your opinions?
Reply | Threaded
Open this post in threaded view
|

Re: Strange code in mkgmap?

Ticker Berkin
The ShapeSplitter code looks like my mistake - I suspect it should be
testing the lowPoint and highPoint, but need to think about this more
carefully before committing myself.

HousenumberGenerator not mine but - yes, looks like those 2 lines
should be removed

ShapeMergeFilter isn't mine, but looks OK. For efficiency it first
tests if the names are the same object, then null/string ordering,
finally a string comparison.

Ticker

On Sat, 2017-07-15 at 10:49 -0700, UliBaer wrote:

> Hi there,
>
> examining the code of mkgmap i found some strange lines.
>
> In /mkgmap/src/uk/me/parabola/util/ShapeSplitter.java Line 330:
> This is redundant?
>
> In
> /mkgmap/src/uk/me/parabola/mkgmap/osmstyle/housenumber/HousenumberGen
> erator.java
> Line 1537+1538:
> This was already tested 6 lines above, so this cannot occur - Dead
> code?
>
> In /mkgmap/src/uk/me/parabola/mkgmap/filters/ShapeMergeFilter.java
> Line 554:
> This is a String comparison, shouldn't *n1.equals(n2)* be used
> instead?
>
> Just some thoughts of mine browsing the code - your opinions?
>
>
>
> --
> View this message in context:
> http://gis.19327.n8.nabble.com/Strange-code-in-mkgmap-tp5899307.html
> Sent from the Mkgmap Development mailing list archive at Nabble.com.
> _______________________________________________
> mkgmap-dev mailing list
> [hidden email]
> http://www.mkgmap.org.uk/mailman/listinfo/mkgmap-dev
_______________________________________________
mkgmap-dev mailing list
[hidden email]
http://www.mkgmap.org.uk/mailman/listinfo/mkgmap-dev
Reply | Threaded
Open this post in threaded view
|

Re: Strange code in mkgmap?

Ticker Berkin
Hi Gerd

Attached is patch for ShapeSplitter

Ticker

On Sat, 2017-07-15 at 19:40 +0100, Ticker Berkin wrote:

> The ShapeSplitter code looks like my mistake - I suspect it should be
> testing the lowPoint and highPoint, but need to think about this more
> carefully before committing myself.
>
> HousenumberGenerator not mine but - yes, looks like those 2 lines
> should be removed
>
> ShapeMergeFilter isn't mine, but looks OK. For efficiency it first
> tests if the names are the same object, then null/string ordering,
> finally a string comparison.
>
> Ticker
>
> On Sat, 2017-07-15 at 10:49 -0700, UliBaer wrote:
> > Hi there,
> >
> > examining the code of mkgmap i found some strange lines.
> >
> > In /mkgmap/src/uk/me/parabola/util/ShapeSplitter.java Line 330:
> > This is redundant?
> >
> > In
> > /mkgmap/src/uk/me/parabola/mkgmap/osmstyle/housenumber/HousenumberG
> > en
> > erator.java
> > Line 1537+1538:
> > This was already tested 6 lines above, so this cannot occur - Dead
> > code?
> >
> > In /mkgmap/src/uk/me/parabola/mkgmap/filters/ShapeMergeFilter.java
> > Line 554:
> > This is a String comparison, shouldn't *n1.equals(n2)* be used
> > instead?
> >
> > Just some thoughts of mine browsing the code - your opinions?
> >
> >
> >
> > --
> > View this message in context:
> > http://gis.19327.n8.nabble.com/Strange-code-in-mkgmap-tp5899307.htm
> > l
> > Sent from the Mkgmap Development mailing list archive at
> > Nabble.com.
> > _______________________________________________
> > mkgmap-dev mailing list
> > [hidden email]
> > http://www.mkgmap.org.uk/mailman/listinfo/mkgmap-dev
> _______________________________________________
> mkgmap-dev mailing list
> [hidden email]
> http://www.mkgmap.org.uk/mailman/listinfo/mkgmap-dev
_______________________________________________
mkgmap-dev mailing list
[hidden email]
http://www.mkgmap.org.uk/mailman/listinfo/mkgmap-dev

ShapeSplitFix.patch (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Strange code in mkgmap?

Gerd Petermann
Hi Ticker,

thanks, seems that Uil found an error here. Can you add a unit test that shows where this code is relevant?

Gerd
________________________________________
Von: mkgmap-dev <[hidden email]> im Auftrag von Ticker Berkin <[hidden email]>
Gesendet: Mittwoch, 26. Juli 2017 12:12:36
An: [hidden email]
Betreff: Re: [mkgmap-dev] Strange code in mkgmap?

Hi Gerd

Attached is patch for ShapeSplitter

Ticker

On Sat, 2017-07-15 at 19:40 +0100, Ticker Berkin wrote:

> The ShapeSplitter code looks like my mistake - I suspect it should be
> testing the lowPoint and highPoint, but need to think about this more
> carefully before committing myself.
>
> HousenumberGenerator not mine but - yes, looks like those 2 lines
> should be removed
>
> ShapeMergeFilter isn't mine, but looks OK. For efficiency it first
> tests if the names are the same object, then null/string ordering,
> finally a string comparison.
>
> Ticker
>
> On Sat, 2017-07-15 at 10:49 -0700, UliBaer wrote:
> > Hi there,
> >
> > examining the code of mkgmap i found some strange lines.
> >
> > In /mkgmap/src/uk/me/parabola/util/ShapeSplitter.java Line 330:
> > This is redundant?
> >
> > In
> > /mkgmap/src/uk/me/parabola/mkgmap/osmstyle/housenumber/HousenumberG
> > en
> > erator.java
> > Line 1537+1538:
> > This was already tested 6 lines above, so this cannot occur - Dead
> > code?
> >
> > In /mkgmap/src/uk/me/parabola/mkgmap/filters/ShapeMergeFilter.java
> > Line 554:
> > This is a String comparison, shouldn't *n1.equals(n2)* be used
> > instead?
> >
> > Just some thoughts of mine browsing the code - your opinions?
> >
> >
> >
> > --
> > View this message in context:
> > http://gis.19327.n8.nabble.com/Strange-code-in-mkgmap-tp5899307.htm
> > l
> > Sent from the Mkgmap Development mailing list archive at
> > Nabble.com.
> > _______________________________________________
> > mkgmap-dev mailing list
> > [hidden email]
> > http://www.mkgmap.org.uk/mailman/listinfo/mkgmap-dev
> _______________________________________________
> mkgmap-dev mailing list
> [hidden email]
> http://www.mkgmap.org.uk/mailman/listinfo/mkgmap-dev
_______________________________________________
mkgmap-dev mailing list
[hidden email]
http://www.mkgmap.org.uk/mailman/listinfo/mkgmap-dev
Reply | Threaded
Open this post in threaded view
|

Re: Strange code in mkgmap?

Ticker Berkin
Hi Gerd

I don't think this needs a unit test. The patch is just to fix a
copy/paste error.

This code deals with a pathological case where there is a cut through a
shape that loops around and along itself (3 lines on top of each other)
and to distinguish whether it is a hole looping back inside or an area
looping back outside.

Ticker

On Thu, 2017-07-27 at 06:32 +0000, Gerd Petermann wrote:

> Hi Ticker,
>
> thanks, seems that Uil found an error here. Can you add a unit test
> that shows where this code is relevant?
>
> Gerd
> ________________________________________
> Von: mkgmap-dev <[hidden email]> im Auftrag
> von Ticker Berkin <[hidden email]>
> Gesendet: Mittwoch, 26. Juli 2017 12:12:36
> An: [hidden email]
> Betreff: Re: [mkgmap-dev] Strange code in mkgmap?
>
> Hi Gerd
>
> Attached is patch for ShapeSplitter
>
> Ticker
>
> On Sat, 2017-07-15 at 19:40 +0100, Ticker Berkin wrote:
> > The ShapeSplitter code looks like my mistake - I suspect it should
> > be
> > testing the lowPoint and highPoint, but need to think about this
> > more
> > carefully before committing myself.
> >
> > HousenumberGenerator not mine but - yes, looks like those 2 lines
> > should be removed
> >
> > ShapeMergeFilter isn't mine, but looks OK. For efficiency it first
> > tests if the names are the same object, then null/string ordering,
> > finally a string comparison.
> >
> > Ticker
> >
> > On Sat, 2017-07-15 at 10:49 -0700, UliBaer wrote:
> > > Hi there,
> > >
> > > examining the code of mkgmap i found some strange lines.
> > >
> > > In /mkgmap/src/uk/me/parabola/util/ShapeSplitter.java Line 330:
> > > This is redundant?
> > >
> > > In
> > > /mkgmap/src/uk/me/parabola/mkgmap/osmstyle/housenumber/Housenumbe
> > > rG
> > > en
> > > erator.java
> > > Line 1537+1538:
> > > This was already tested 6 lines above, so this cannot occur -
> > > Dead
> > > code?
> > >
> > > In
> > > /mkgmap/src/uk/me/parabola/mkgmap/filters/ShapeMergeFilter.java
> > > Line 554:
> > > This is a String comparison, shouldn't *n1.equals(n2)* be used
> > > instead?
> > >
> > > Just some thoughts of mine browsing the code - your opinions?
> > >
> > >
> > >
> > > --
> > > View this message in context:
> > > http://gis.19327.n8.nabble.com/Strange-code-in-mkgmap-tp5899307.h
> > > tm
> > > l
> > > Sent from the Mkgmap Development mailing list archive at
> > > Nabble.com.
> > > _______________________________________________
> > > mkgmap-dev mailing list
> > > [hidden email]
> > > http://www.mkgmap.org.uk/mailman/listinfo/mkgmap-dev
> > _______________________________________________
> > mkgmap-dev mailing list
> > [hidden email]
> > http://www.mkgmap.org.uk/mailman/listinfo/mkgmap-dev
> _______________________________________________
> mkgmap-dev mailing list
> [hidden email]
> http://www.mkgmap.org.uk/mailman/listinfo/mkgmap-dev
_______________________________________________
mkgmap-dev mailing list
[hidden email]
http://www.mkgmap.org.uk/mailman/listinfo/mkgmap-dev