Repository navigation
Cape Town | sep-2026-ITP | Matthaus Bause | Sprint 3 | Dead Code #1755
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
base: main
Are you sure you want to change the base?
Changes from all commits
2ec464d
0795731
7753fc4
f357b30
e2e47e3
ca27335
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,17 +1,9 @@ | ||
| // Find the instances of unreachable and redundant code - remove them! | ||
| // The sayHello function should continue to work for any reasonable input it's given. | ||
|
|
||
| let testName = "Jerry"; | ||
| let testName = "Aman"; | ||
| const greeting = "hello"; | ||
|
|
||
| function sayHello(greeting, name) { | ||
| const greetingStr = greeting + ", " + name + "!"; | ||
| return `${greeting}, ${name}!`; | ||
| console.log(greetingStr); | ||
| } | ||
|
|
||
| testName = "Aman"; | ||
|
|
||
| const greetingMessage = sayHello(greeting, testName); | ||
|
|
||
| console.log(greetingMessage); // 'hello, Aman!' | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,17 +1,9 @@ | ||
| // Remove the unused code that does not contribute to the final console log | ||
| // The countAndCapitalisePets function should continue to work for any reasonable input it's given, and you shouldn't modify the pets variable. | ||
|
|
||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well done for removing capitalisedPets and logPets, and for keeping petsStartingWithH, which the final output depends on. Line 3 hasn’t changed, but because it has moved, Git shows it as deleted and re-added, so reviewers have to check that it wasn’t altered. When removing code, try to leave the remaining lines where they were so the diff shows only your intended changes. Checking your diff before pushing is a good way to catch this. |
||
| const pets = ["parrot", "hamster", "horse", "dog", "hamster", "cat", "hamster"]; | ||
| const capitalisedPets = pets.map((pet) => pet.toUpperCase()); | ||
| const petsStartingWithH = pets.filter((pet) => pet[0] === "h"); | ||
|
|
||
| function logPets(petsArr) { | ||
| petsArr.forEach((pet) => console.log(pet)); | ||
| } | ||
| const petsStartingWithH = pets.filter((pet) => pet[0] === "h"); | ||
|
|
||
| function countAndCapitalisePets(petsArr) { | ||
| const petCount = {}; | ||
|
|
||
| petsArr.forEach((pet) => { | ||
| const capitalisedPet = pet.toUpperCase(); | ||
| if (petCount[capitalisedPet]) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,11 +1,24 @@ | ||
| function formatAs12HourClock(time) { | ||
|
|
||
| const [hoursStr, minutesStr] = time.split(':'); | ||
| const hours = Number(hoursStr); | ||
| const minutes = minutesStr; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This line creates a second variable holding exactly the same value as minutesStr. Is there a way you could avoid the extra variable while keeping the code readable? |
||
|
|
||
| const hours = Number(time.slice(0, 2)); | ||
| if (hours === 0 || hours === 24) { | ||
| return `12:${minutes} am`; | ||
| } | ||
|
|
||
| if (hours === 12) { | ||
| return `12:${minutes} pm`; | ||
| } | ||
|
|
||
| if (hours > 12) { | ||
| return `${hours - 12}:00 pm`; | ||
| const formattedHours = String(hours - 12).padStart(2, '0'); | ||
| return `${formattedHours}:${minutes} pm`; | ||
| } | ||
| return `${time} am`; | ||
|
|
||
| const formattedHours = String(hours).padStart(2, '0'); | ||
| return `${formattedHours}:${minutes} am`; | ||
|
Comment on lines
+11
to
+21
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Great work handling the edge cases. |
||
| } | ||
|
|
||
| export {formatAs12HourClock}; | ||
| export {formatAs12HourClock}; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The icon here means the file no longer ends with a newline, which is why this unchanged line shows as modified. Most editors and tools expect files to end with a newline, so it’s worth checking your editor settings (Prettier on save should handle this for you). |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,10 +2,94 @@ import {formatAs12HourClock} from "./timeConverter.js"; | |
| import assert from "node:assert"; | ||
| import test from "node:test"; | ||
|
|
||
| test("correctly convert time after 12:00", function(){ | ||
| test("correctly convert hours after 12:00", () => { | ||
| assert.equal(formatAs12HourClock("23:00"), "11:00 pm"); | ||
| }); | ||
|
|
||
| test("can correctly convert morning time", function() { | ||
| test("correctly convert minutes after 12:00", () => { | ||
| assert.equal(formatAs12HourClock("23:53"), "11:53 pm"); | ||
| }); | ||
|
|
||
| test("can correctly convert morning time", () => { | ||
| assert.equal(formatAs12HourClock("08:00"), "08:00 am"); | ||
| }); | ||
|
|
||
| test("can correctly convert morning time minutes", () => { | ||
| assert.equal(formatAs12HourClock("08:30"), "08:30 am"); | ||
| }); | ||
|
|
||
| test("can correctly convert morning time minutes", () => { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This test has the same name as the one on line 17. Unique names make it much easier to tell which test failed when something breaks. |
||
| assert.equal(formatAs12HourClock("08:34"), "08:34 am"); | ||
| }); | ||
|
|
||
| test("can successfully convert midnight time", () => | ||
| assert.equal(formatAs12HourClock("24:00"), "12:00 am")); | ||
|
Comment on lines
+25
to
+26
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This test is written differently from the others (no curly braces or semicolons). Keeping a consistent style makes the file easier to read. |
||
|
|
||
| test("correctly convert midnight 00:00", () => { | ||
| assert.equal(formatAs12HourClock("00:00"), "12:00 am"); | ||
| }); | ||
|
|
||
| test("correctly convert noon 12:00", () => { | ||
| assert.equal(formatAs12HourClock("12:00"), "12:00 pm"); | ||
| }); | ||
|
|
||
| test("correctly convert boundary time around noon", () => { | ||
| assert.equal(formatAs12HourClock("11:59"), "11:59 am"); | ||
| assert.equal(formatAs12HourClock("12:01"), "12:01 pm"); | ||
| }); | ||
|
|
||
| test("correctly convert early afternoon hours", () => { | ||
| assert.equal(formatAs12HourClock("13:00"), "01:00 pm"); | ||
| }); | ||
|
|
||
| test("correctly convert single-digit morning hours if supported", () => { | ||
| assert(formatAs12HourClock("08:30"), "08:30am"); | ||
| }); | ||
|
Comment on lines
+45
to
+47
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This test uses assert(...) rather than assert.equal(...) like the others. Have a look at what the second argument to assert() actually does, and whether this test could ever fail. It’s also worth checking that the input matches the test name (is "08:30" a single-digit hour?) and that the expected output is formatted the same way as your other tests. |
||
|
|
||
| /* | ||
| Edge cases: | ||
| Correctly convert hours after 12:00 | ||
| Input | Expected | ||
| 23:00 11:00 pm | ||
|
|
||
| Correctly convert afternoon hours and minutes | ||
| Input | Expected | ||
| 23:00 11:53 pm | ||
|
|
||
| Correctly convert morning time and minutes | ||
| Input | Expected | ||
| 08:00 08:00 am | ||
|
|
||
| Correctly convert morning time and minutes | ||
| Input | Expected | ||
| 08:30 08:30 am | ||
|
|
||
| Correctly convert morning time and minutes | ||
| Input | Expected | ||
| 08:34 08:34 am | ||
|
|
||
| Correctly convert midnight time | ||
| Input | Expected | ||
| 24:00 12:00 am | ||
|
|
||
| Correctly convert midnight | ||
| Input | Expected | ||
| 00:00 12:00 am | ||
|
|
||
| Correctly convert noon | ||
| Input | Expected | ||
| 12:00 12:00 pm | ||
|
|
||
| Correctly convert boundary time around noon | ||
| Input | Expected | ||
| 11:59 11:59 am | ||
| 12:01 12:01 pm | ||
|
|
||
| Correctly convert early afternoon hours | ||
| Input | Expected | ||
| 13:00 1:00 pm | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The comment here expects 1:00 pm, but the test on line 42 expects 01:00 pm. Which format does your function produce? It’s worth making the comment and test agree so they don’t confuse. |
||
|
|
||
| Correctly convert single-digit morning hours if supported | ||
| Input | Expected | ||
| 08:30 8:30 am | ||
| */ | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Check the comment on line R24 as it applies here. |
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work spotting the unreachable console.log and the unused greetingStr, and setting testName correctly from the start is a neat simplification. Since testName is no longer reassigned anywhere, have another look at how it’s declared and think about which keyword best signals that its value won’t change.