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

Check if we can remove code duplication in search classes #264

Closed
Roel opened this issue Apr 18, 2020 · 1 comment · Fixed by #302
Closed

Check if we can remove code duplication in search classes #264

Roel opened this issue Apr 18, 2020 · 1 comment · Fixed by #302
Assignees
Milestone

Comments

@Roel
Copy link
Member

Roel commented Apr 18, 2020

From the pyOpenSci review:

My question about the code relates to the search sub-package. I understand the need for the types sub-package, because each class in that sub-package encapsulates the metadata such as wfs and xml fields specific to each dataset type. However it's not quite clear to me why different classes are needed for the search objects that correspond to each type in the search package. Am I right that all the search classes have the exact same attributes and methods? My read of the code is that the only difference between the search classes is the default value for objecttype. E.g., a BoringSearch class has its objecttype default =Boring , which dictates what fields it has, but aside from that, it has the exact same methods as all the other search types. If I'm misunderstanding something about the code, please let me know and I'll just remove this question. But it seems like it might be possible to reduce code repetition by having another class that inherits from AbstractSearch with all the code that is common to the specific Search classes, and then just subclass this additional class for specific types.

Something like:

class MetaSearch(AbstractSearch):
    def __init__(self, layer, objecttype):
        super(MetaSearch, self).__init__('dov-pub:Boringen', objecttype)

    def __common_search_methods_here__():
    ...

import Boring

BoringSearch(MetaSearch)
    def __init__(self):
        super(BoringSearch, self).__init__(layer='dov-pub:Boringen', objecttype=Boring)

I think this is a valid question, as there is quite some code duplication in the search classes. If I remember correctly this has something to do with the private class variables containing the WFS schema, metadata, XSD schema etc:

    __wfs_schema = None
    __wfs_namespace = None
    __md_metadata = None
    __fc_featurecatalogue = None
    __xsd_schemas = None

We want to avoid downloading this data multiple times even if a user creates multiple instances of the search class (hence they are class variables). The data is however specific to a certain type so cannot be moved to a base class..

I think there are two routes to evaluate:

  • Look into a clever way to keep this behaviour while still reducing some of the code duplication.
  • Let go the behaviour of keeping the data between multiple instances of the search classes. This would allow refactoring the class variables to instance variables which would facilitate moving duplicate code into the base class. Since the search instances don't persist data between searches anyway, this might be a valid option to consider.
@Roel
Copy link
Member Author

Roel commented Oct 26, 2020

This might also relate to #281 as this will remove the major reason we implemented this shared data in the first place, which is to share the common (and large) WFS capabilities data between (instances of) search classes.

@Roel Roel added this to the v2.1.0 milestone Nov 7, 2020
@Roel Roel self-assigned this Nov 22, 2020
@Roel Roel closed this as completed in #302 Feb 18, 2021
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 a pull request may close this issue.

1 participant