Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#347 - Custom properties #348

Merged
merged 1 commit into from
Jun 1, 2017
Merged

#347 - Custom properties #348

merged 1 commit into from
Jun 1, 2017

Conversation

balage1541
Copy link

I've implemented the request in Vehicle, VehicleType, Location and in the Job classes.
Added some unit tests as well.

I've tried hard to keep formatting, but in some cases it was impossible, so there is some unchanged lines marked as changed.

Still need to migrate this feature to the new branch.

@oblonski
Copy link
Member

oblonski commented Jun 1, 2017

Great! Thanks.

@oblonski oblonski merged commit 29fbdd6 into graphhopper:master Jun 1, 2017
@carhartl
Copy link

carhartl commented Jun 13, 2017

Would it make sense to add the setUserData() and getUserData() methods to the job interface? This would allow me to do the following:

Job job = ((TourActivity.JobActivity) tourActivity).getJob();
job.getUserData(); // I may not care about the type of job (service, shipment, ...)

Right now I have to do this:

Job job = (Job) ((TourActivity.JobActivity) tourActivity).getJob();
if (job instanceof Service) {
    ((Service) job).getUserData();
} else if (job instanceof Shipment) {
    ((Shipment) job).getUserData();
}

(There could be a nicer way to do this anyway...)

Alternatively, casting to an abstract (more of internal?) class kind of felt dirty:

AbstractJob job = (AbstractJob) ((TourActivity.JobActivity) tourActivity).getJob();
job.getUserData();

@balage1551
Copy link

Thanks, your suggestion makes sense. However, this version of Jsprit is Java7 complient, which means, adding a new method to an interface may break any implementation based on it. (All user defined Job types.)

Jsprit 2 will be built on Java 8, so there we could extend an interface by adding default implementation. Surely we will do it.

As for the current version, you have to take some unconfortable casting for the sake of backward compatibility. By the way, the AbstractJob is not private api, but a minimal base for all job implementation, so don't feel guilty to cast on it!

@carhartl
Copy link

Makes sense 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants