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

Merging from db_access.py get_unique and get_unique_count? #12

Open
ivergara opened this issue May 10, 2021 · 2 comments
Open

Merging from db_access.py get_unique and get_unique_count? #12

ivergara opened this issue May 10, 2021 · 2 comments

Comments

@ivergara
Copy link
Collaborator

Both functions are extremely similar, would it be possible to merge them in a sane way?

@kevinkleinQC
Copy link

kevinkleinQC commented May 11, 2021

I haven't come up with a good idea yet. I thought of the following options:

  1. Having one of the functions call the other.

Clearly, this could only work one way: get_unique_counts calls get_uniques. Yet, get_unique_counts should also work with very large numbers. This works as of now, since the computation happens within database. If one was to first retrieve all unique values and then count oneself, this could lead to very large resource usage.

  1. Externalizing the common denominator.

Afaict, the common denominator is 1-2 loc.

  1. Making one function out of the two and using a parameter indicating the desired return value/type.

I'm not so sure about how intuitive/user-friendly this is.

What did you have in mind?

@ivergara
Copy link
Collaborator Author

Option 3 is what I was thinking about. An argument telling that in one case you have to use an aggregation function and in the other, no function is used. Is there anything like a no-op function that could help in keeping the code simple?

I wouldn't be too concerned about it being intuitive, in the end, the user never sees these functions but a higher-level interface.

@kklein kklein transferred this issue from another repository Apr 22, 2022
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

No branches or pull requests

2 participants