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

Store returned metrics in variables prior to return statements. #105

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

MarcusSorealheis
Copy link

I'm stepping through the code and there's a lot here. In general, it's a fantastic step in the right direction and I appreciate the willingness to open source the system. One nit I have is arithmetic in the return statements. As these modules evolve, it would be helpful to store the calculated values in variables and then return those variables. It's an extremely small increment toward overall improvement.

I look forward to getting my hands dirty here more and in the coming search system. 👍

@faeezshaikh
Copy link

The downside of storing values in variables unnecessarily before returning is that it can increase the memory usage of the program, which can impact performance, especially for large datasets. Additionally, it can make the code harder to read and understand, especially if the variable name does not provide any additional clarity or context. Therefore, it is generally recommended to only use variables when they provide additional clarity or are necessary for the logic of the program.

In general, if the expression is short and simple, it may not be necessary to store it in a variable before returning.

@MarcusSorealheis
Copy link
Author

For metrics, the readability and extensibility of the code is probably more important than memory usage. There are other areas of a system where the same is not true. We could profile it if there were specific reasons why you think there could be some issue from a modest increase in memory usage here, but from what I can see the impact should be negligible.

@CLAassistant
Copy link

CLAassistant commented Apr 1, 2023

CLA assistant check
All committers have signed the CLA.

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.

3 participants