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

Refactor and Optimization:: constants.py #264

Merged
merged 2 commits into from
Oct 2, 2024

Conversation

IIITM-Jay
Copy link
Member

@IIITM-Jay IIITM-Jay commented Jul 21, 2024

About

This PR deals with refactoring and code optimisation towards a portion of constants.py file . A helper function is created to read csv files and returns the list of required tuples.

Added/ Modified Changes

  1. A helper function read_csv is created: It takes the filename(str) as an argument and returns the list of tuples as before.
  2. Avoided repetition: codes being written to remove the duplicated functionality to achieve the same output as a return
  3. Comments are added
  4. Code Optimisation Techniques are used

Code Optimisation Technique used

  1. Use of List Comprehension: It allows us to use a shorter syntax to generate a list. For example, take a look in the code written earlier:
causes = []
with open("causes.csv") as f:
    csv_reader = csv.reader(f, skipinitialspace=True)
    for row in csv_reader:
        causes.append((int(row[0], 0), row[1]))

The following can be easily observed from the above piece of code:
a. causes need to be declared before appending to it.
b. The loop takes two line of codes to generate the tuple

Using List Comprehension the above can be rewritten to the below modified code:

with open("causes.csv") as f:
    csv_reader = csv.reader(f, skipinitialspace=True)
    causes = [(int(row[0], 0), row[1]) for row in csv_reader]
  1. Removal of Duplicated Codes: The codes for reading csv files and returning a tuple in the same format were written multiple times doing the same functionality. For better readability and to improve efficiency, a helper function is created which can be reused to return the same output as before.

@IIITM-Jay
Copy link
Member Author

Hi @aswaterman, this a step towards participation in the RISC-V Mentorship Program for the Fall 2024 regarding the concerned project Refactoring and Enhancing the RISC-V Opcodes Repository.
Requesting review from @aswaterman your side and for the feedback as well on what steps ahead to be taken for the above stated program. I have also seen the coding challenge mentioned on LFX platform. Will love to hear your views and opinions on the same.

Copy link
Contributor

@Timmmm Timmmm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Minor thing: I would just suggest calling it read_csv_int_map instead because it reads a specific kind of CSV, not just any CSV file.

Merge master into local
@IIITM-Jay
Copy link
Member Author

IIITM-Jay commented Oct 2, 2024

@aswaterman can these approaches be incorporated to reduce the number of lines of codes and to avoid redundancy. Let me know your thoughts on this PR. Requesting a review from your side. Will include the feedback and suggestions as provided. Thanks!

Copy link
Member

@aswaterman aswaterman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@IIITM-Jay Yes, LGTM.

Sorry that I'm very far behind on code reviews. Please feel free to ping me on any high-priority PRs that have languished.

@aswaterman aswaterman merged commit 0033120 into riscv:master Oct 2, 2024
2 checks passed
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