navya-javascript-function-error-handling-assignments#544
navya-javascript-function-error-handling-assignments#544navya-1417 wants to merge 5 commits intoaptyInc:mainfrom
Conversation
Summary of ChangesHello @navya-1417, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request delivers a comprehensive collection of JavaScript assignments designed to reinforce understanding of core functional programming concepts and robust error handling. Each assignment provides a practical, interactive example, ranging from basic function definitions and argument handling to advanced topics like closures, higher-order functions, and various error management strategies. The structure allows for easy exploration and testing of each concept. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request adds a comprehensive set of JavaScript assignments covering functions and error handling. The implementations are mostly correct according to the assignment requirements. However, there are several areas for improvement regarding best practices, performance, and robustness. Key issues include the use of inline JavaScript and styles, inefficient algorithms, incomplete input validation, and some logical errors in error handling. Most critically, the main index.html file contains incorrect paths to the assignment files, which will prevent them from loading. My review includes suggestions to address these points and align the code with modern JavaScript best practices.
| <summary>Arguments</summary> | ||
|
|
||
| <details> | ||
| <summary>Assignment 1</summary> | ||
| <iframe src="./Parameters, Arguments, and Default Values/assignment-1/area-of-a-rectangle.html" | ||
| style="border: none" width="100%"></iframe> | ||
| </details> | ||
|
|
||
| <details> | ||
| <summary>Assignment 2</summary> | ||
| <iframe src="./Parameters, Arguments, and Default Values/assignment-2/rest-parameter.html" | ||
| style="border: none" width="100%"></iframe> | ||
| </details> | ||
|
|
||
| <details> | ||
| <summary>Assignment 3</summary> | ||
| <iframe src="./Parameters, Arguments, and Default Values/assignment-3/nested-function.html" | ||
| style="border: none" width="100%"></iframe> | ||
| </details> | ||
|
|
||
| <details> | ||
| <summary>Assignment 4</summary> | ||
| <iframe src="./Parameters, Arguments, and Default Values/assignment-4/callback-function.html" | ||
| style="border: none" width="100%"></iframe> | ||
| </details> | ||
| </details> | ||
|
|
||
| <details> | ||
| <summary>Closures and Lexical Scoping</summary> | ||
|
|
||
| <details> | ||
| <summary>Assignment 1</summary> | ||
| <iframe src="./Closures and Lexical Scoping/assignment-1/closure.html" style="border: none" | ||
| width="100%"></iframe> | ||
| </details> | ||
|
|
||
| <details> | ||
| <summary>Assignment 2</summary> | ||
| <iframe src="./Closures and Lexical Scoping/assignment-2/lexical-scoping.html" style="border: none" | ||
| width="100%"></iframe> | ||
| </details> | ||
|
|
||
| <details> | ||
| <summary>Assignment 3</summary> | ||
| <iframe src="./Closures and Lexical Scoping/assignment-3/caching.html" style="border: none" | ||
| width="100%"></iframe> | ||
| </details> | ||
| </details> | ||
|
|
||
| <details> | ||
| <summary>Intermediate Topics</summary> | ||
|
|
||
| <details> | ||
| <summary>Assignment 1</summary> | ||
| <iframe src="./Intermediate Topics/assignment-1/higher-order.html" style="border: none" | ||
| width="100%"></iframe> | ||
| </details> | ||
|
|
||
| <details> | ||
| <summary>Assignment 2</summary> | ||
| <iframe src="./Intermediate Topics/assignment-2/higherOrder-arrow.html" style="border: none" | ||
| width="100%"></iframe> | ||
| </details> | ||
| </details> | ||
|
|
||
| <details> | ||
| <summary>Error Handling</summary> | ||
|
|
||
| <details> | ||
| <summary>Assignment 1</summary> | ||
| <iframe src="./Error Handling/assignment-1/syntax-error.html" style="border: none" width="100%"></iframe> | ||
| </details> | ||
|
|
||
| <details> | ||
| <summary>Assignment 2</summary> | ||
| <iframe src="./Error Handling/assignment-2/runtime-error.html" style="border: none" width="100%"></iframe> | ||
| </details> | ||
|
|
||
| <details> | ||
| <summary>Assignment 3</summary> | ||
| <iframe src="./Error Handling/assignment-3/final-block.html" style="border: none" width="100%"></iframe> | ||
| </details> | ||
|
|
||
| <details> | ||
| <summary>Assignment 4</summary> | ||
| <iframe src="./Error Handling/assignment-4/custom-error.html" style="border: none" width="100%"></iframe> | ||
| </details> | ||
| <details> | ||
| <summary>Assignment 5</summary> | ||
| <iframe src="./Error Handling/assignment-5/error-object.html" style="border: none" width="100%"></iframe> | ||
| </details> | ||
| </details> |
There was a problem hiding this comment.
The src attributes for the <iframe> elements are pointing to incorrect file paths and directory names (e.g., Parameters, Arguments, and Default Values instead of arguments, and area-of-a-rectangle.html instead of index.html). This will cause all assignments to fail to load correctly. All paths should be updated to reflect the actual file structure in the repository.
| } finally { | ||
|
|
||
| result.textContent += ` (Default City: ${default_city})`; | ||
| } |
There was a problem hiding this comment.
The finally block executes regardless of whether an error was thrown. Currently, it appends the default city to the output message in both success and error cases, which can be confusing for the user (e.g., "City: Bangalore (Default City: Pune)"). The logic to display the default city should probably only run when an error occurs, so it would be better placed inside the catch block.
| function parseJSON(jsonString) { | ||
| if (!jsonString || typeof jsonString !== "string") { | ||
| return "Invalid parameter: Input must be a non-empty string."; | ||
| } | ||
|
|
||
| try { | ||
| const parsed = JSON.parse(jsonString); | ||
| return parsed; | ||
| } catch (err) { | ||
| return `JSON parsing error: ${err.message}. | ||
| Make sure keys and strings are quoted properly. Example: {"key": "value"}`; | ||
| } | ||
| } |
There was a problem hiding this comment.
Instead of returning an error string, it's better practice for a parsing function to throw an error on failure. This allows the calling function to handle the error using a try...catch block, which is a more standard error handling pattern in JavaScript. The initial check for a non-empty string is also redundant, as JSON.parse will throw an error for an empty string, which can be caught.
function parseJSON(jsonString) {
try {
const parsed = JSON.parse(jsonString);
return parsed;
} catch (err) {
throw new Error(`JSON parsing error: ${err.message}.
Make sure keys and strings are quoted properly. Example: {"key": "value"}`);
}
}| if (typeof output === "object") { | ||
| result.textContent = "Valid JSON → Parsed Object: " + JSON.stringify(output); | ||
| } else { | ||
| result.textContent = output; | ||
| } |
There was a problem hiding this comment.
Checking typeof output === "object" is not a reliable way to determine if JSON parsing was successful. Valid JSON can parse to other primitive types (e.g., "a string", 123, true), and typeof null is 'object'. A more robust approach is to have parseJSON throw an error on failure (as suggested in another comment) and wrap the call in handleParse with a try...catch block.
try {
const output = parseJSON(input);
result.textContent = "Valid JSON → Parsed Object: " + JSON.stringify(output);
} catch (err) {
result.textContent = err.message;
}| function fibonacci(n) { | ||
| if (n <= 1) return n; | ||
| return fibonacci(n - 1) + fibonacci(n - 2); | ||
| } |
There was a problem hiding this comment.
This recursive implementation of the Fibonacci sequence has an exponential time complexity (O(2^n)). This is very inefficient and will cause performance issues, potentially freezing the browser for even moderately large values of n. For generating a series, an iterative approach is much more efficient (O(n)). Alternatively, you could use memoization (caching results) to optimize the recursion.
| const name = document.getElementById("userInput").value.trim().toLowerCase(); | ||
|
|
||
| try { | ||
| const cityName = users[name].address.city; |
There was a problem hiding this comment.
|
|
||
| <br><br> | ||
|
|
||
| <button onclick="generateList()">Generate List</button> |
There was a problem hiding this comment.
Using onclick attributes in HTML for event handling is generally discouraged as it mixes behavior with structure. This violates the "Avoid mixing inline JavaScript with HTML" best practice mentioned in the pull request description. It's better to add an ID to the button and use addEventListener in your JavaScript file to attach the event listener. This improves code organization and maintainability.
Example:
HTML: <button id="generateListBtn">Generate List</button>
JS: document.getElementById('generateListBtn').addEventListener('click', generateList);
This comment applies to many other HTML files in this PR.
| <button onclick="generateList()">Generate List</button> | |
| <button id="generateListBtn">Generate List</button> |
| function calculate(a, b, operator) { | ||
|
|
||
|
|
||
| if (typeof a !== "number" || typeof b !== "number" || isNaN(a) || isNaN(b)) { |
There was a problem hiding this comment.
The typeof checks in this condition are redundant. Since a and b are the result of calling Number(), their type will always be 'number'. Checking for isNaN(a) || isNaN(b) is sufficient to validate that the inputs are valid numbers.
| if (typeof a !== "number" || typeof b !== "number" || isNaN(a) || isNaN(b)) { | |
| if (isNaN(a) || isNaN(b)) { |
|
|
||
| <button onclick="sortNumbers()">Sort</button> | ||
|
|
||
| <table border="1" id="resultTable" style="margin-top:15px; padding:5px;"> |
There was a problem hiding this comment.
Using inline style attributes is generally considered bad practice as it mixes presentation with content, making the code harder to maintain. This also goes against the "Avoid inline styles" best practice from the PR description. It's better to move styles to a separate CSS file or a <style> tag in the <head>.
| <table border="1" id="resultTable" style="margin-top:15px; padding:5px;"> | |
| <table border="1" id="resultTable"> |
| function filterArray(arr, callback) { | ||
| return callback(arr); | ||
| } |
…://github.com/navya-1417/apty-training into navya-function-and-error-handling-assignments
Terms and Conditions
HTML Best Practices
File Naming Convention:
Follow consistent and descriptive naming (e.g., dashboard.html, user-profile.html).
Use lowercase letters and hyphens instead of spaces.
Page Title:
Ensure the <title> tag is descriptive and aligns with the page content.
Include meaningful keywords for SEO if applicable.
Semantic Markup:
Use appropriate tags like <header>, <footer>, <section>, <article> for better readability and accessibility.
Accessibility Standards:
Ensure the use of alt attributes for images and proper labels for form elements.
Use ARIA roles where necessary.
Validation:
Ensure the code passes HTML validation tools without errors or warnings.
Structure and Indentation:
Maintain consistent indentation and proper nesting of tags.
Attributes:
Ensure all required attributes (e.g., src, href, type, etc.) are correctly used and not left empty.
CSS Best Practices
File Organization:
Use modular CSS files if applicable (e.g., base.css, layout.css, theme.css).
Avoid inline styles unless absolutely necessary.
Naming Conventions:
Use meaningful class names following BEM or other conventions (e.g., block__element--modifier).
Code Reusability:
Avoid duplicate code; use classes or mixins for shared styles.
Responsive Design:
Ensure proper usage of media queries for mobile, tablet, and desktop views.
Performance Optimization:
Minimize the use of unnecessary CSS selectors.
Avoid overly specific selectors and ensure selectors are not overly deep (e.g., avoid #id .class1 .class2 p).
Consistency:
Follow consistent spacing, indentation, and use of units (rem/em vs. px).
Maintain a single coding style (e.g., always use double or single quotes consistently).
Javascript Best Practices
File Organization:
Ensure scripts are modular and logically separated into files if needed.
Avoid mixing inline JavaScript with HTML.
Logic Optimization:
Check for redundancy and ensure the code is optimized for performance.
Avoid unnecessary API calls or DOM manipulations.
Solution Approach:
Confirm that the code solves the given problem efficiently.
Consider scalability for future enhancements.
Readability:
Use clear variable and function names.
Add comments for complex logic or algorithms.
Error Handling:
Ensure proper error handling for API calls or user input validation.
Code Quality:
Check for potential bugs (e.g., missing await, mishandling of null/undefined values).
Avoid unnecessary console.log statements in production code.
Security:
Avoid hardcoding sensitive data.
Sanitize user input to prevent XSS and other vulnerabilities.
Best Practices:
Use const and let instead of var.
Follow ES6+ standards where applicable.