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

Simplify TWRoute member functions #557

Closed
jcoupey opened this issue Aug 30, 2021 · 2 comments
Closed

Simplify TWRoute member functions #557

jcoupey opened this issue Aug 30, 2021 · 2 comments

Comments

@jcoupey
Copy link
Collaborator

jcoupey commented Aug 30, 2021

The TWRoute class has various member functions used to first check validity for a change, then apply it. They have fancy names such as:

  • TWRoute::is_valid_addition_for_tw (two versions);
  • TWRoute::add to add a single job to a route;
  • TWRoute::replace to replace a (potentially empty) range by another (potentially empty) job range.

The two versions of is_valid_addition_for_tw match the respective situation of add (only a single job with a single insertion position) and replace (using ranges).

This code has been sitting peacefully for quite some time now as the overall TW logic is somehow tricky and I've made a point of not touching anything as long as it works. The problem is that whenever we have to twist this logic (yes I mean #547), this sounds like twice the work for hysterical raisins.

I don't really recall why we have two versions, probably made sense at first to start with the easiest situation, then move to another version for the range situation. Now the sure thing is that the single-job variants could definitely be implemented in term of a single call to the range versions. Only overhead I can see would be creating a dummy vector, so would need some benchmarking but probably worth for the sake of simplifying the code.

@jcoupey
Copy link
Collaborator Author

jcoupey commented Aug 30, 2021

I'm tempted to apply this directly in #547 but for a meaningful comparison, I'll fire a PR for this sole change.

@jcoupey jcoupey mentioned this issue Aug 30, 2021
4 tasks
@jcoupey
Copy link
Collaborator Author

jcoupey commented Sep 2, 2021

Merged with #558.

@jcoupey jcoupey closed this as completed Sep 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant