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 utilities.js #1251

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

ahmed0saber
Copy link

No description provided.

@mohitpanjwani
Copy link
Contributor

The refactor is good but the current domain check function checks a lot of values and it seems to be working well. Can you explain the reasoning behind updating the implementation for that?

@ahmed0saber
Copy link
Author

Hello @mohitpanjwani , the only reason for any refactoring process is to make the code more readable, more cleaner, more maintainable, also there is a rule in refactoring which discusses not changing the functionality of code during code refactor, so I'm not fixing a bug, error or anything, also I'm not adding a feature, but I'm just trying to write the same functionality in a better way, so it's easy to understand it and maintain it in the future. The first code (before refactoring) may take 1 hour to be understood then maintained if any problem occurred there, but the second one (after refactoring) may take about 10 minutes or less, so refactoring code can save us time, and cost depends on time, so we can also save some money or invest this time to do something else that may have much urgent priorities.

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.

2 participants