Skip to content

refactor oshdb core (and more)#369

Merged
rtroilo merged 36 commits into
masterfrom
refactor-oshdb-core
Apr 29, 2021
Merged

refactor oshdb core (and more)#369
rtroilo merged 36 commits into
masterfrom
refactor-oshdb-core

Conversation

@rtroilo
Copy link
Copy Markdown
Member

@rtroilo rtroilo commented Apr 12, 2021

Refactoring oshdb core. This PR touches a lot of files because of moving of classes to different packages.

renaming methods

  • OSMWay.getRef() to OSMWay.getMembers()
    this makes is more unified with the OSMRelation interface
  • OSHDBTimestamp.getRawUnixTimestamp() to OSHDBTimestamp.getEpochSeconds()

deprecations of methods

  • OSMEntity.getRawTag
  • OSHEntity.getRawTagKeys

removed methods

  • OSHDBBoundingBox.add/set
    this makes OSHDBBoundingBox immutable!

reorganisation

  • move oshdb.util.OSHDBTag to oshdb.OSHDBTag
  • move oshdb.util.OSHDBTimestamp to oshdb.OSHDBTimestamp
  • move oshdb.util.OSHDBBoundingBox to oshdb.OSHDBBoundingBox
  • move oshdb.util.CellIterator.OSHEntityFilter to oshdb.osh.OSHEntityFilter
  • move oshdb.util.CellIterator.OSMEntityFilter to oshdb.osm.OSMEntityFilter
  • move oshdb-api.generic.function to oshdb.util.function
    makes a common/central place for our extended java function api.

new interfaces

  • OSHDBTimeable
    main interface for OSHDBTimestamp, allows to "flatten" OSMEntity and reduce object creation.
    add methods isAfter/isBefore to OSMEntity so that we directly can check like osmEntity.isAfter(timestamp)
  • OSHDBBoundable
    main interface for OSHDBBoundingBox allows to "flatten" OSHEntity and reduce object creation.
    add methods intersects/isInside to OSHEntity so that we direcly can check like oshEntity.intersects(bbox)

notes

  • code-style fixes will be in a separate PR to not clutter this PR with "unrelated" changes

Checklist

@rtroilo rtroilo added enhancement New feature or request code quality Related to our standards for 'good' code labels Apr 12, 2021
@rtroilo rtroilo added this to the release 0.7.0 milestone Apr 12, 2021
@tyrasd
Copy link
Copy Markdown
Member

tyrasd commented Apr 13, 2021

  • move oshdb-api.generic.function to oshdb.util.function
    makes a common/central place for our extended java function api.

Not sure I can follow here. AFAICS, the "extended" java functions are only ever used in the oshdb-api module, and are only necessary elsewhere (we only need it when running queries on a remote ignite cluster with peer class loading enabled). IMHO we should put as little "stuff" into the "core" module as possible. Keeping it slim allows that some fixes don't need to be deployed on the ignite cluster for example.

PS:

move oshdb.util.CellIterator.OSHEntityFilter to oshdb.osh.OSHEntityFilter
move oshdb.util.CellIterator.OSMEntityFilter to oshdb.osm.OSMEntityFilter

If I remember correctly, we had the two classes before SerializablePredicate<X> was introduced. It could make sense to just use SerializablePredicate<OSHEntity>/SerializablePredicate<OSMEntity> instead of having this extra interface lying around in the global scope. Or, if we want to keep it for readability/simplicity: why not keep it as a subclass of CellIterator?

@rtroilo
Copy link
Copy Markdown
Member Author

rtroilo commented Apr 13, 2021

I saw a commit 46ec2eb which introduces an other set of PredicateSerializables again, because of circular dependencies.
So to solve this, and to avoid to have incompatible PredicateSerializables I moved them to a more central place. Also the OSH/OSMEntityFilter fits nicely into the osh/osm packages.

@rtroilo
Copy link
Copy Markdown
Member Author

rtroilo commented Apr 13, 2021

IMHO we should put as little "stuff" into the "core" module as possible.

Yes sounds good, so what do you think about to move the SerializedFunctions into the oshdb-util module oshdb.util.function instead of the oshdb-api module. Because there are also used by the oshdb.util.celliterator.CellIterator and oshdb.filter.Filter

@tyrasd tyrasd changed the title 🚧 refactor oshdb core 🚧 refactor oshdb core (and more) Apr 13, 2021
@rtroilo rtroilo changed the title 🚧 refactor oshdb core (and more) refactor oshdb core (and more) Apr 16, 2021
@rtroilo rtroilo marked this pull request as ready for review April 16, 2021 09:02
Copy link
Copy Markdown
Member

@tyrasd tyrasd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this review I only looked at some of the code (i.e. where there are comments below).

In general I like the idea behind the introduction of the new OSHDBTimeable and OSHDBBoundable interfaces, but the naming could be "improved" maybe? IMHO, it could be made clearer the the time aspect is only for points in time, while the spatial aspect is (in general) for bounded regions.

Maybe the point-in-time interface could be just OSHDBEvent (or OSHDBTemporal) which would leave us the door open to later introduce a class for time-intervals similar to OSHDBBoundable) without confusion?!

PS: I found that there were a lot of lines introduced in this PR which contain trailing spaces or empty lines consisting of only spaces. IMHO this should be avoided. Is it not possible to configure your code editor to automatically remove trailing whitespace on save? 😜

Comment thread oshdb/src/main/java/org/heigit/ohsome/oshdb/OSHDBBoundingBox.java Outdated
Comment thread oshdb/src/main/java/org/heigit/ohsome/oshdb/OSHDBBoundingBox.java Outdated
Comment thread oshdb/src/main/java/org/heigit/ohsome/oshdb/OSHDBBoundingBox.java Outdated
Comment thread oshdb/src/main/java/org/heigit/ohsome/oshdb/OSHDBBoundable.java
Comment thread oshdb/src/main/java/org/heigit/ohsome/oshdb/OSHDBBoundable.java Outdated
Comment thread oshdb/src/main/java/org/heigit/ohsome/oshdb/OSHDBTag.java Outdated
Comment thread oshdb/src/main/java/org/heigit/ohsome/oshdb/OSHDBTag.java Outdated
Comment thread oshdb/src/main/java/org/heigit/ohsome/oshdb/OSHDBTimeable.java Outdated
Comment thread oshdb/src/main/java/org/heigit/ohsome/oshdb/OSHDBTimeable.java Outdated
Comment thread oshdb/src/main/java/org/heigit/ohsome/oshdb/OSHDBTimeable.java Outdated
@rtroilo rtroilo requested a review from tyrasd April 27, 2021 12:52
@tyrasd tyrasd force-pushed the refactor-oshdb-core branch from 14eab3b to 1c59dfc Compare April 27, 2021 14:00
@tyrasd
Copy link
Copy Markdown
Member

tyrasd commented Apr 28, 2021

@tyrasd
Copy link
Copy Markdown
Member

tyrasd commented Apr 28, 2021

  • I have written javadoc (required for public methods)
  • I have added sufficient unit tests

It would be nice to check these off, at least for the newly created interfaces. 🙏

Comment thread oshdb/src/main/java/org/heigit/ohsome/oshdb/impl/osh/OSHNodeImpl.java Outdated
@tyrasd tyrasd force-pushed the refactor-oshdb-core branch from 8628e38 to f8d91cc Compare April 29, 2021 07:13
@rtroilo
Copy link
Copy Markdown
Member Author

rtroilo commented Apr 29, 2021

I removed the getTimestamp for the OSHDBTemporal interface as well.

@rtroilo
Copy link
Copy Markdown
Member Author

rtroilo commented Apr 29, 2021

  • I have written javadoc (required for public methods)
  • I have added sufficient unit tests

It would be nice to check these off, at least for the newly created interfaces. pray

I added the javadoc but I'm not sure about the uni-tests for the interfaces. I think the should be covered by the implementing classes. Do you think that is enough, or should I write some test again.

@rtroilo
Copy link
Copy Markdown
Member Author

rtroilo commented Apr 29, 2021

Thank you and good review!

@rtroilo rtroilo merged commit ece7b85 into master Apr 29, 2021
@rtroilo rtroilo deleted the refactor-oshdb-core branch April 29, 2021 13:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

code quality Related to our standards for 'good' code enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants