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

misc: optimize by avoiding repetitive code #59

Closed

Conversation

Shwetha-Acharya
Copy link
Collaborator

Added a list of testcases to iterate over and to avoid repetition. Also removed unnecessary print statement.

Added a list of testcases to iterate over and to avoid repetition.
Also removed unnecessary print statement.
This also improves readability.

Signed-off-by: Shwetha K Acharya <[email protected]>
@spuiuk
Copy link
Collaborator

spuiuk commented Jan 24, 2024

I am not convinced that these changes are required. This particular code is not expected to change.
The addition of the loop doesn't really add much to functionality or readability in my opinion.

@Shwetha-Acharya
Copy link
Collaborator Author

Shwetha-Acharya commented Jan 29, 2024

I am not convinced that these changes are required. This particular code is not expected to change. The addition of the loop doesn't really add much to functionality or readability in my opinion.

I have just made this change according to the basic principle of avoiding repetitive code using loop.
5742b8a#diff-93cad77f223a70e92e5582538a5f34e8b1ab80b34927d4791694e154f776b941R97 avoids repeatedly calling three functions one after the other.

I agree it is not much of functionality, but it is just a clean practice and improves readability and maintainability of code. I am open to hear any opinions.

@spuiuk
Copy link
Collaborator

spuiuk commented Jan 30, 2024

I think it negatively impacts the readability of the code. I vote that we skip this change for now and re-visit it in case these tests get more complicated.

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