8000 Code cleanup 160 by reenu153 · Pull Request #565 · TreyWW/MyFinances · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Code cleanup 160 #565

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Code cleanup 160 #565

wants to merge 12 commits into from

Conversation

reenu153
Copy link
@reenu153 reenu153 commented Apr 10, 2025 •
edited
Loading

Description

This pull request refactors the code primarily to address the code cleanup tasks outlined in issue #160. The refactoring addresses several code smells and aims to improve the readability, maintainability, and structure of the codebase.

Refactoring Details:

  1. Magic Numbers/Constants:

    • Issue: Magic numbers and hardcoded values were scattered across the codebase, making the code harder to maintain.
    • Refactoring:
      • Created a backend/core/constants.py file to define named constants for recurring numerical values (e.g., MAX_LENGTH_STANDARD, MAX_LENGTH_NAME, MAX_LENGTH_DESCRIPTION, DECIMAL_MAX_DIGITS, DECIMAL_PLACES).
      • Replaced hardcoded max_length and decimal constraints in models with the defined constants.

  2. Inappropriate/Unclear Naming:

    • Issue: Some variable and function names were ambiguous and did not clearly convey their purpose.
    • Refactoring:
      • Renamed functions to be more descriptive and Pythonic:
        • _public_storage → get_public_storage
        • _private_storage → get_private_storage
        • RandomCode → generate_verification_code
        • RandomAPICode → generate_api_key
        • upload_to_user_separate_folder → get_file_upload_path
      • These changes enhance clarity by explicitly describing the functionality of each function.

  3. Set blank=False for name fields: Ensures name and related fields (e.g., description, event_name) are required across multiple models to prevent missing data.

Checklist

  • Ran the Black Formatter and
    djLint-er on any new code
    (checks
    will
    fail without)
  • Made any changes or additions to the documentation where required
  • Changes generate no new warnings/errors
  • New and existing unit tests pass locally with my
    changes

What type of PR is this?

  • ♻️ Code Refactor

Added/updated tests?

  • 🙅 no, because they aren't needed

Related PRs, Issues etc

@TreyWW
Copy link
Owner
TreyWW commented Apr 10, 2025

Hey @reenu153 / @LaraMerdol,

Thank you for these changes! There's currently a fairly big queue in PRs so I wont be able to merge this right away, but I do appreciate the effort! I'll let you know when I get round to testing and merging this :)

@TreyWW TreyWW self-assigned this Apr 10, 2025
Maffey
Maffey reviewed May 18, 2025
View reviewed changes
backend/core/models.py
return storages["private_media"]


def RandomCode(length=6):
def generate_verification_code(length=6):
Copy link

Choose a reason for hiding this comment

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

Suggested change
def generate_verification_code(length=6):
def generate_verification_code(length: int = 6):

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Maffey Maffey Maffey left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@TreyWW TreyWW

Labels
no-stale
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@reenu153 @TreyWW @Maffey @LaraMerdol @Siddhant-Mhapankar
0