Feature: Implemented calculator layout with responsive UI and functional arithmetic operations. #1052
Conversation
…nal arithmetic operations
WalkthroughAdds a new Calculator mini-project under Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Browser
participant Handler as EventHandler
participant State as currentExpression
participant Eval as Evaluator
participant UI as Display
User->>Browser: click button / press key
Browser->>Handler: dispatch event
alt input (digit/operator/percent)
Handler->>State: append token
Handler->>UI: update input display
else backspace
Handler->>State: remove last char
Handler->>UI: update input display
else clear
Handler->>State: reset expression
Handler->>UI: clear displays
else evaluate (= or Enter)
Handler->>Eval: sanitizeExpression(State)
Eval->>Eval: convert % to /100
Eval->>Eval: tokenize expression
Eval->>Eval: convert tokens to RPN (Shunting Yard)
Eval->>Eval: evaluate RPN -> numeric result
Eval-->>Handler: result or "Error"
Handler->>UI: show result
end
UI->>Browser: render updated values
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Hi! 👋 This PR adds a fully functional calculator with a responsive UI, percentage support, and error handling for division by zero. Screenshots are included for clarity. Happy to make adjustments if needed! 🚀 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
Projects/calculator-js/script.js (1)
1-5: Consider defensive DOM reference checks.While the current approach works for this simple project, adding null checks would prevent runtime errors if the HTML structure changes.
const display = document.getElementById('display'); const resultDisplay = document.getElementById('result'); const buttons = document.querySelectorAll('.buttons button'); + +if (!display || !resultDisplay || buttons.length === 0) { + console.error('Required DOM elements not found'); +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Projects/calculator-js/README.md(1 hunks)Projects/calculator-js/index.html(1 hunks)Projects/calculator-js/script.js(1 hunks)Projects/calculator-js/style.css(1 hunks)
🧰 Additional context used
🪛 Biome (2.1.2)
Projects/calculator-js/script.js
[error] 18-18: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().
(lint/security/noGlobalEval)
🔇 Additional comments (4)
Projects/calculator-js/index.html (1)
1-44: LGTM! Clean and semantic HTML structure.The HTML is well-structured with:
- Proper semantic markup using
<main>for the calculator container- Accessibility attributes (
aria-label,readonly) for screen readers- Clean data attribute strategy (
data-action,data-value) for JavaScript binding- Appropriate placeholder text for user guidance
Projects/calculator-js/style.css (1)
1-73: LGTM! Modern and well-organized CSS.The stylesheet demonstrates good practices:
- CSS custom properties for consistent theming
- Responsive design with flexbox and grid
- Smooth transitions for user interactions
- Clean visual hierarchy with the dual-display design
- Special styling for the equals and clear buttons enhances UX
Projects/calculator-js/script.js (2)
11-13: Good input validation, but eval() remains risky.The sanitization provides a whitelist of allowed characters, which is good practice. However, malicious expressions can still be constructed from these characters (e.g., deeply nested operations causing performance issues).
Note: This comment acknowledges the defensive measure while the eval() usage is addressed separately in the calculateExpression review.
29-57: LGTM! Button handlers are clean and functional.The event handling logic is well-structured:
- Clear action properly resets both displays
- Backspace removes the last character as expected
- Equals triggers evaluation and updates result display
- New input clears the result display, maintaining good UX flow
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Projects/calculator-js/script.js (1)
15-27: Blocker: Replace eval with safer evaluation; add fail-fast checks and length guard.Use Function constructor (stopgap) with strict mode and reject inputs that change after sanitization; cap expression length to avoid DoS. Prefer math.js if adding a dep.
function calculateExpression(expr) { - const withPercent = expr.replace(/(\d+(\.\d+)?)%/g, '($1/100)'); - try { - const result = eval(withPercent); + // Fail fast: refuse if sanitization alters input or it's too long + const sanitized = sanitizeExpression(expr); + if (sanitized !== expr) return 'Error'; + if (sanitized.length > 128) return 'Error'; + const withPercent = sanitized.replace(/(\d+(\.\d+)?)%/g, '($1/100)'); + try { + // Safer than eval, still arithmetic-only due to prior sanitization + const fn = new Function('"use strict"; return (' + withPercent + ')'); + const result = fn(); if (typeof result === 'number') { if (!isFinite(result)) return 'Error'; return String(Math.round(result * 1e10) / 1e10); } return 'Error'; } catch (e) { return 'Error'; } }Alternative (preferred if allowed): use math.js’
math.evaluate(withPercent)and remove both eval and Function. Based on static analysis hints.
🧹 Nitpick comments (1)
Projects/calculator-js/script.js (1)
37-37: Nit: DRY the repeated “clear result” lines.Extract a small helper to reduce repetition.
function updateDisplay() { display.value = currentExpression; } +function clearResult() { + resultDisplay.value = ''; +} + ... currentExpression = ''; updateDisplay(); - resultDisplay.value = ''; + clearResult(); ... currentExpression = currentExpression.slice(0, -1); updateDisplay(); - resultDisplay.value = ''; + clearResult(); ... currentExpression += val; updateDisplay(); - resultDisplay.value = ''; + clearResult(); ... currentExpression += e.key; updateDisplay(); - resultDisplay.value = ''; + clearResult(); ... currentExpression = currentExpression.slice(0, -1); updateDisplay(); - resultDisplay.value = ''; + clearResult(); ... currentExpression = ''; updateDisplay(); - resultDisplay.value = ''; + clearResult();Also applies to: 43-43, 55-55, 65-65, 77-77, 84-84
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Projects/calculator-js/script.js(1 hunks)
🧰 Additional context used
🪛 Biome (2.1.2)
Projects/calculator-js/script.js
[error] 18-18: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().
(lint/security/noGlobalEval)
🔇 Additional comments (1)
Projects/calculator-js/script.js (1)
1-6: No changes needed—elements are already readonly in HTML.Verification confirms both
displayandresultinput elements already have thereadonlyattribute set in the HTML markup (lines 11–12 ofindex.html). The suggested JavaScript modification is redundant since the requirement is already satisfied at the HTML level.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
Projects/calculator-js/script.js (1)
302-319: Major: Global keydown handler still steals keystrokes and has UX issues.This directly addresses a previous review concern that remains unresolved. The global
document.addEventListenerwill:
- Steal keystrokes from other text inputs on the page
- Cause duplicate characters if calculator fields gain focus
- Break normal typing behavior elsewhere in the DOM
Additional issues:
- Missing
preventDefault()for printable keys and Enter- Result display not cleared when user starts typing (UX inconsistency)
- 'c' key for clear is non-standard; Escape is preferable
Apply this comprehensive fix:
-document.addEventListener('keydown', (e) => { - if (e.key >= '0' && e.key <= '9' || '+-*/.()%'.includes(e.key)) { +document.addEventListener('keydown', (e) => { + // Only handle keys when calculator has focus or no other text input is active + const target = e.target; + const isTextInput = target && (target.tagName === 'INPUT' || target.tagName === 'TEXTAREA' || target.isContentEditable === true); + const isCalcField = target === display || target === resultDisplay; + if (isTextInput && !isCalcField) return; + + if (e.key >= '0' && e.key <= '9' || '+-*/.()%'.includes(e.key)) { + e.preventDefault(); currentExpression += e.key; updateDisplay(); + resultDisplay.value = ''; - } else if (e.key === 'Enter') { + } else if (e.key === 'Enter' || e.key === 'NumpadEnter') { + e.preventDefault(); const sanitized = sanitizeExpression(currentExpression); const result = calculateExpression(sanitized); resultDisplay.value = result; } else if (e.key === 'Backspace') { e.preventDefault(); currentExpression = currentExpression.slice(0, -1); updateDisplay(); + resultDisplay.value = ''; - } else if (e.key.toLowerCase() === 'c') { + } else if (e.key === 'Escape') { + e.preventDefault(); currentExpression = ''; resultDisplay.value = ''; updateDisplay(); } });
🧹 Nitpick comments (2)
Projects/calculator-js/script.js (2)
1-98: Remove commented dead code.These 98 lines of commented-out code and embedded review discussions serve no purpose in the final implementation and reduce readability.
-// const display = document.getElementById('display'); -// const resultDisplay = document.getElementById('result'); -// const buttons = document.querySelectorAll('.buttons button'); - -// let currentExpression = ''; - -// function updateDisplay() { -// display.value = currentExpression; -// } - -// function sanitizeExpression(expr) { -// return expr.replace(/[^0-9+\-*/().%]/g, ''); -// } - -// // function calculateExpression(expr) { -// // const withPercent = expr.replace(/(\d+(\.\d+)?)%/g, '($1/100)'); -// // try { -// // const result = eval(withPercent); -// // if (typeof result === 'number') { -// // if (!isFinite(result)) return 'Error'; -// // return String(Math.round(result * 1e10) / 1e10); -// // } -// // return 'Error'; -// // } catch (e) { -// // return 'Error'; -// // } -// // } - -// +// Option 1: Use math.js library (add to index.html: <script src="https://cdnjs.cloudflare.com/ajax/libs/mathjs/11.11.0/math.min.js"></script>) -// +// Or install via npm: npm install mathjs -// + - -// +// Option 1: Use math.js library (add to index.html: <script src="https://cdnjs.cloudflare.com/ajax/libs/mathjs/11.11.0/math.min.js"></script>) -// +// Or install via npm: npm install mathjs -// + - - - - - -// buttons.forEach(btn => { -// btn.addEventListener('click', () => { -// const val = btn.dataset.value; -// const action = btn.dataset.action; - -// if (action === 'clear') { -// currentExpression = ''; -// updateDisplay(); -// resultDisplay.value = ''; -// return; -// } -// if (action === 'back') { -// currentExpression = currentExpression.slice(0, -1); -// updateDisplay(); -// resultDisplay.value = ''; -// return; -// } -// if (action === 'equals') { -// const sanitized = sanitizeExpression(currentExpression); -// const result = calculateExpression(sanitized); -// resultDisplay.value = result; -// return; -// } - -// currentExpression += val; -// updateDisplay(); -// resultDisplay.value = ''; -// }); -// }); - -// document.addEventListener('keydown', (e) => { -// // Only digits, operators, and decimal -// if ("0123456789+-*/.%".includes(e.key)) { -// currentExpression += e.key; -// updateDisplay(); -// resultDisplay.value = ''; -// } -// // Evaluate on Enter -// else if (e.key === 'Enter') { -// e.preventDefault(); // Prevent form submission -// const sanitized = sanitizeExpression(currentExpression); -// resultDisplay.value = calculateExpression(sanitized); -// } -// // Delete last character -// else if (e.key === 'Backspace') { -// e.preventDefault(); // Prevent browser back navigation -// currentExpression = currentExpression.slice(0, -1); -// updateDisplay(); -// resultDisplay.value = ''; -// } -// // Clear using Escape key -// else if (e.key === 'Escape') { -// e.preventDefault(); -// currentExpression = ''; -// updateDisplay(); -// resultDisplay.value = ''; -// } -// }); - const display = document.getElementById('display');
145-152: Consider validating decimal point usage in number parsing.The tokenizer accepts multiple decimal points in a single number (e.g., "1.2.3").
parseFloatwill silently parse this as1.2, which may confuse users who mistype.Track whether a decimal point has been seen and reject additional ones:
// Handle numbers (including decimals) if (char >= '0' && char <= '9' || char === '.') { let num = ''; + let hasDecimal = false; while (i < expr.length && (expr[i] >= '0' && expr[i] <= '9' || expr[i] === '.')) { + if (expr[i] === '.') { + if (hasDecimal) break; // Stop at second decimal point + hasDecimal = true; + } num += expr[i]; i++; } tokens.push({ type: 'number', value: parseFloat(num) }); continue; }
1 participant
📌 Summary
Enhanced the Calculator UI/UX with a clear input/result separation and improved calculation handling for a smoother user experience.
✅ Features Added
50% → 0.5)"Error"(e.g.,7 ÷ 0)📸 Screenshots
🖼️ Screenshot 1 — Calculator UI
🖼️ Screenshot 1 — Normal Calculation
Example:
2.5*3 = 7.5🖼️ Screenshot 2 — Division by Zero
Example:
7 ÷ 0 = Error🖼️ Screenshot 3 — Percent (%) Feature
Example:
50% + 1 = 1.5✅ Files Updated
index.htmlstyle.cssscript.js🙌 Ready for Review
Let me know if any polishing is needed! Excited to collaborate. 🚀
Summary by CodeRabbit
New Features
Bug Fixes
Documentation