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 shared complex element filters #3674

Closed
wants to merge 2 commits into from
Closed

Simplify shared complex element filters #3674

wants to merge 2 commits into from

Conversation

FloEdelmann
Copy link
Member

Instead of keeping a large chunk of copy-pasted (and then adjusted) code in sync between three quests, create the filtered lists programmatically from one single source of truth.

I noticed this in #3649 because the auto-formatting really messed up the indentation there.

@westnordost
Copy link
Member

Uhm, this is kind of a dejavú. I thought I remember rejecting a very similar pull request some time ago. Yes, it is shorter (though just 50 lines), but this is where the advantages end:

  1. The filter logic is split up in several files. More difficult to navigate.

  2. Not the common filter logic is put into that separate file but all of it. So, after putting all the filter logic in one file, you need to separate it again with this unusual construct

   val common = setOf(AddOpeningHours::class, AddPlaceName::class, AddWheelchairAccessBusiness::class)
   val nameAndOpeningHours = setOf(AddPlaceName::class, AddOpeningHours::class)
   val nameAndWheelchair = setOf(AddPlaceName::class, AddWheelchairAccessBusiness::class)

which leads to the filter logic for each individual quest being scattered all over the place. I am surprised you find this more readable.

@FloEdelmann
Copy link
Member Author

Uhm, this is kind of a dejavú. I thought I remember rejecting a very similar pull request some time ago.

Hmm, I didn't know/remember that, otherwise I wouldn't have opened this PR.

The filter logic is split up in several files.

Yeah, but it kinda is right now, too, because you should check all three files when making a change in any one of them.


I'm fine with closing this if it doesn't improve (or actually harms) the readability for others. Do you think there is another way it could be improved?

@FloEdelmann FloEdelmann deleted the simplify-element-filters branch January 27, 2022 10:32
@westnordost
Copy link
Member

Probably not, what would be permissable is to outsource filter logic for certain properties of things. E.g. fun Element.isWalkIn() or similar for places where you can usually just walk in, the same way the isAKindOfShop function exists for things thta usually exist in a Ladenfläche. Though I am really not sure if something like that could be found, because in the end it maybe boils down to usuallyHasOpeningHours, usuallyHasName etc. which would be 1:1 what the quests filter now.

@mnalis
Copy link
Member

mnalis commented Jan 27, 2022

@FloEdelmann maybe related to this #3302 ?

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

Successfully merging this pull request may close these issues.

3 participants