Refactor JavaScript: Reduce Redundancy and Implement Modern Practices #367
Conversation
WalkthroughThe changes in this pull request focus on enhancing the structure and functionality of the Changes
Assessment against linked issues
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
❌ Deploy Preview for manageyourwaste failed. Why did it fail? →
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- script.js (1 hunks)
🧰 Additional context used
🔇 Additional comments (1)
script.js (1)
161-187: 🛠️ Refactor suggestionConsolidate multiple 'DOMContentLoaded' event listeners
There are two separate
DOMContentLoadedevent listeners in the script, which can lead to potential conflicts and increased overhead.Combine the code within both
DOMContentLoadedevent listeners into a single listener to improve performance and maintainability:document.addEventListener('DOMContentLoaded', () => { // Existing code... + // Ecotips slider functionality + const tipsSlider = document.querySelector('.eco-tips-content'); + const slides = document.querySelectorAll('.eco-tip'); + const prevSlide = document.getElementById('prev-slide'); + const nextSlide = document.getElementById('next-slide'); + let currentIndex = 0; + + const updateSlider = () => { + if (slides.length === 0) return; + const slideWidth = slides[0].clientWidth; + tipsSlider.style.transform = `translateX(-${currentIndex * slideWidth}px)`; + }; + + tipsSlider.style.transition = 'transform 0.3s ease-in-out'; + + nextSlide?.addEventListener('click', () => { + currentIndex = (currentIndex + 1) % slides.length; + updateSlider(); + }); + + prevSlide?.addEventListener('click', () => { + currentIndex = (currentIndex - 1 + slides.length) % slides.length; + updateSlider(); + }); + + window.addEventListener('resize', updateSlider); + window.addEventListener('load', updateSlider); });Likely invalid or redundant comment.
| console.log(e); | ||
| }); | ||
| lenis.on('scroll', (e) => { | ||
| console.log(e); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unnecessary console logging
The console.log(e); statement in the smoothScrolling function may not be needed in production code and could clutter the console output.
Consider removing or commenting out the console.log statement:
- console.log(e);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| console.log(e); |
| const slideWidth = slides[0].clientWidth; | ||
| tipsSlider.style.transform = `translateX(-${currentIndex * slideWidth}px)`; |
There was a problem hiding this comment.
Add check for empty slides array to prevent errors
In the updateSlider function, accessing slides[0] without confirming that slides contains elements may result in a runtime error if the array is empty.
Add a check to ensure slides is not empty before proceeding:
const updateSlider = () => {
+ if (slides.length === 0) return;
const slideWidth = slides[0].clientWidth;
tipsSlider.style.transform = `translateX(-${currentIndex * slideWidth}px)`;
};This prevents potential errors when the slides are not available.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const slideWidth = slides[0].clientWidth; | |
| tipsSlider.style.transform = `translateX(-${currentIndex * slideWidth}px)`; | |
| if (slides.length === 0) return; | |
| const slideWidth = slides[0].clientWidth; | |
| tipsSlider.style.transform = `translateX(-${currentIndex * slideWidth}px)`; |
| feedbackForm?.addEventListener('submit', (event) => { | ||
| event.preventDefault(); | ||
| const [name, email, message] = ['input[type="text"]', 'input[type="email"]', 'textarea'] | ||
| .map(selector => feedbackForm.querySelector(selector)?.value.trim()); | ||
|
|
||
| const nameValid = name?.length >= 2; | ||
| const emailValid = /^[^\s@]+@[^\s@]+\.[^\s@]+$/.test(email); | ||
| const messageValid = message?.length >= 10; | ||
|
|
||
| if (nameValid && emailValid && messageValid) { | ||
| alert('Feedback submitted successfully!'); | ||
| feedbackForm.reset(); | ||
| } else { | ||
| if (!nameValid) feedbackForm.querySelector('input[type="text"]').setCustomValidity('Name must be at least 2 characters long.'); | ||
| if (!emailValid) feedbackForm.querySelector('input[type="email"]').setCustomValidity('Please enter a valid email address.'); | ||
| if (!messageValid) alert('Message must be at least 10 characters long.'); | ||
| feedbackForm.reportValidity(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure consistent validation feedback
In the feedback form validation, setCustomValidity is used for the name and email fields, but an alert is used for the message field. This inconsistency may confuse users.
Use setCustomValidity for the message field to provide consistent validation feedback:
+ // Reset custom validity messages
+ feedbackForm.querySelector('input[type="text"]').setCustomValidity('');
+ feedbackForm.querySelector('input[type="email"]').setCustomValidity('');
+ feedbackForm.querySelector('textarea').setCustomValidity('');
if (nameValid && emailValid && messageValid) {
alert('Feedback submitted successfully!');
feedbackForm.reset();
} else {
if (!nameValid) feedbackForm.querySelector('input[type="text"]').setCustomValidity('Name must be at least 2 characters long.');
if (!emailValid) feedbackForm.querySelector('input[type="email"]').setCustomValidity('Please enter a valid email address.');
- if (!messageValid) alert('Message must be at least 10 characters long.');
+ if (!messageValid) feedbackForm.querySelector('textarea').setCustomValidity('Message must be at least 10 characters long.');
feedbackForm.reportValidity();
}This approach ensures all validation messages are displayed uniformly.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| feedbackForm?.addEventListener('submit', (event) => { | |
| event.preventDefault(); | |
| const [name, email, message] = ['input[type="text"]', 'input[type="email"]', 'textarea'] | |
| .map(selector => feedbackForm.querySelector(selector)?.value.trim()); | |
| const nameValid = name?.length >= 2; | |
| const emailValid = /^[^\s@]+@[^\s@]+\.[^\s@]+$/.test(email); | |
| const messageValid = message?.length >= 10; | |
| if (nameValid && emailValid && messageValid) { | |
| alert('Feedback submitted successfully!'); | |
| feedbackForm.reset(); | |
| } else { | |
| if (!nameValid) feedbackForm.querySelector('input[type="text"]').setCustomValidity('Name must be at least 2 characters long.'); | |
| if (!emailValid) feedbackForm.querySelector('input[type="email"]').setCustomValidity('Please enter a valid email address.'); | |
| if (!messageValid) alert('Message must be at least 10 characters long.'); | |
| feedbackForm.reportValidity(); | |
| } | |
| }); | |
| feedbackForm?.addEventListener('submit', (event) => { | |
| event.preventDefault(); | |
| const [name, email, message] = ['input[type="text"]', 'input[type="email"]', 'textarea'] | |
| .map(selector => feedbackForm.querySelector(selector)?.value.trim()); | |
| const nameValid = name?.length >= 2; | |
| const emailValid = /^[^\s@]+@[^\s@]+\.[^\s@]+$/.test(email); | |
| const messageValid = message?.length >= 10; | |
| // Reset custom validity messages | |
| feedbackForm.querySelector('input[type="text"]').setCustomValidity(''); | |
| feedbackForm.querySelector('input[type="email"]').setCustomValidity(''); | |
| feedbackForm.querySelector('textarea').setCustomValidity(''); | |
| if (nameValid && emailValid && messageValid) { | |
| alert('Feedback submitted successfully!'); | |
| feedbackForm.reset(); | |
| } else { | |
| if (!nameValid) feedbackForm.querySelector('input[type="text"]').setCustomValidity('Name must be at least 2 characters long.'); | |
| if (!emailValid) feedbackForm.querySelector('input[type="email"]').setCustomValidity('Please enter a valid email address.'); | |
| if (!messageValid) feedbackForm.querySelector('textarea').setCustomValidity('Message must be at least 10 characters long.'); | |
| feedbackForm.reportValidity(); | |
| } | |
| }); |
script.js
Outdated
| function validateForm(formType) { | ||
| const form = document.getElementById(formType === 'signup' ? 'form1' : 'form2'); | ||
| const errorMessage = document.getElementById('error-message'); | ||
| const fields = Array.from(form.querySelectorAll('input')); | ||
|
|
||
| errorMessage.innerText = ''; | ||
|
|
||
| // Check if fields are empty | ||
| if (!fullName || !email || !password1 || !password2) { | ||
| if (fields.some(field => !field.value)) { | ||
| errorMessage.innerText = 'Please fill all the details.'; | ||
| return false; // Prevent form submission | ||
| } | ||
|
|
||
| // Password validation: at least 8 characters, one uppercase, one lowercase, and one special character | ||
| const passwordRegex = /^(?=.*[a-z])(?=.*[A-Z])(?=.*[\W_]).{8,}$/; | ||
| if (!passwordRegex.test(password1)) { | ||
| errorMessage.innerText = 'Password must be at least 8 characters long, including uppercase, lowercase, and special characters.'; | ||
| return false; // Prevent form submission | ||
| } | ||
|
|
||
| // Check if passwords match | ||
| if (password1 !== password2) { | ||
| errorMessage.innerText = 'Passwords do not match.'; | ||
| return false; // Prevent form submission | ||
| return false; | ||
| } | ||
|
|
||
| return true; // Allow form submission | ||
| } | ||
|
|
||
| function validateLogin() { | ||
| const email = document.querySelector('input[name="email"]').value; | ||
| const password1 = document.querySelector('input[name="password1"]').value; | ||
| const errorMessage = document.getElementById('error-message'); | ||
| if (formType === 'signup') { | ||
| const [password1, password2] = fields.slice(-2).map(field => field.value); | ||
| const passwordRegex = /^(?=.*[a-z])(?=.*[A-Z])(?=.*[\W_]).{8,}$/; | ||
|
|
||
| // Clear previous error message | ||
| errorMessage.innerText = ''; | ||
| if (!passwordRegex.test(password1)) { | ||
| errorMessage.innerText = 'Password must be at least 8 characters long, including uppercase, lowercase, and special characters.'; | ||
| return false; | ||
| } | ||
|
|
||
| // Check if fields are empty | ||
| if (!email || !password1) { | ||
| errorMessage.innerText = 'Please fill all the details.'; | ||
| return false; // Prevent form submission | ||
| if (password1 !== password2) { | ||
| errorMessage.innerText = 'Passwords do not match.'; | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| return true; // Allow form submission | ||
| return true; |
There was a problem hiding this comment.
Add checks for null elements to prevent runtime errors
In the validateForm function, accessing form and errorMessage without verifying their existence could lead to runtime errors if these elements are not found in the DOM.
Add checks to ensure that form and errorMessage are not null before using them:
const form = document.getElementById(formType === 'signup' ? 'form1' : 'form2');
+ if (!form) {
+ console.error(`Form with ID '${formType === 'signup' ? 'form1' : 'form2'}' not found.`);
+ return false;
+ }
const errorMessage = document.getElementById('error-message');
+ if (!errorMessage) {
+ console.error("Error message element not found.");
+ return false;
+ }Additionally, to prevent conflicts when both forms are present on the same page, consider using form-specific error messages:
- const errorMessage = document.getElementById('error-message');
+ const errorMessage = form.querySelector('.error-message');Ensure that each form contains an element with the class error-message to display validation errors.
Committable suggestion was skipped due to low confidence.
Issues Identification
Closes: #364
Description
This PR refactors our JavaScript codebase to improve efficiency and adopt modern practices. Key changes include:
These changes maintain existing functionality while making the code more maintainable, performant, and aligned with current JavaScript best practices. The refactoring reduces potential errors and improves overall code quality.
Testing
All existing functionalities have been manually tested
Unit tests updated to reflect new code structure
Cross-browser compatibility verified
Summary
Provide a brief description of what this pull request does and why it is needed.
This issue addresses the need to enhance our JavaScript codebase's efficiency and maintainability. The proposed changes include consolidating redundant functions, optimizing event listeners and DOM manipulations, implementing modern JavaScript features, enhancing error handling in forms, and streamlining calculations. These improvements aim to reduce code complexity and boost performance without altering functionality. Motivated by the codebase's organic growth and future scalability needs, this refactoring will reduce potential bugs, improve load times, and align with modern best practices. Specific issues to be fixed include duplicate form validation code, inefficient DOM operations, lack of error prevention in property access, suboptimal event listener usage, and overly complex UI calculations. While no new dependencies are introduced, the changes require ECMAScript 2020 support and modern browser compatibility. The implementation will be incremental, accompanied by updated unit tests and documentation. Potential impacts include improved page load times and developer onboarding, with some consideration needed for older browser compatibility. Next steps involve reviewing the changes, assigning developers, setting up a testing plan, scheduling code reviews, and planning a progressive rollout.
Details
Include any detailed information about the changes in this pull request.
Types of Changes
*Code Redundancy/ Efficiency and minimizing lines of code
Please check the boxes that apply
Checklist
Please check the boxes that apply
Screenshots
Before:
After:
You can see that before changes there were 279 lines of code which has is now 187 lines after making changes in it.
Please add #hacktoberfest-accpted, #hacktoberfest, #level2 labels
Additional Information
Please provide any other information that is relevant to this pull request.
Summary by CodeRabbit
New Features
Bug Fixes