Delete round by MedusCode · Pull Request #3 · CS561-MedusCode/Exp_5.4

MedusCode · 2025-05-05T22:55:39Z

Description

Add "delete round" functionality to the SpeedScore app. A user can delete a round by clicking on the garbage can icon associated with the round in the "Rounds" table.

Related Issue(s)

Addresses #1

Type of Change

New Feature
Bug Fix
Documentation update
Other (describe)

Testing

I manually tested this functionality by creating and deleting rounds.

Pre-Submission Checklist

The code executes; my changes do not break the build
My changes do not generate new warnings
My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
I have added automated tests demonstrating that the feature works or that fix is correct
New and existing unit tests pass locally with my changes
Any changes on which this PR depends have been merged and published in upstream modules

MedusCode

Good job with HTML markup

MedusCode · 2025-05-05T23:36:10Z

+            <div class="modal-dialog">
+                <div class="modal-content">
+                    <div class="modal-header">
+                        <h5 class="modal-title">Delete Round?</h5>
+                        <button type="button" class="btn-close" data-bs-dismiss="modal" aria-label="Close"></button>
+                    </div>
+                    <div class="modal-body">
+                        <p>Do you really want to delete that round?</p>
+                    </div>
+                    <div class="modal-footer">
+                        <button type="button" class="btn btn-secondary" data-bs-dismiss="modal">
+                            No, Cancel
+                        </button>
+                        <button type="button" id="confirmDeleteBtn" class="btn btn-primary" onclick="">
+                            Yes, Delete Round
+                        </button>
+                    </div>
+                </div>
+            </div>


Good job. The HTML markup is well written, and most semantic tags are correctly used.

MedusCode · 2025-05-05T23:37:26Z

                </form>
            </div>
+            <div class="modal-dialog">
+                <div class="modal-content">


Consider using a semantic <section> tag for better structure and accessibility.

MedusCode · 2025-05-05T23:39:06Z

+                        <button type="button" class="btn btn-secondary" data-bs-dismiss="modal">
+                            No, Cancel
+                        </button>
+                        <button type="button" id="confirmDeleteBtn" class="btn btn-primary" onclick="">


onClick="" doesn't do anything and might confuse. Consider removing it since the event listener is set in the JS.

MedusCode · 2025-05-05T23:44:15Z

+ * @function confirmDelete
+ * @desc
+ * Present pop-up modal dialog asking user to confirm delete operation
+ * @param roundId -- the unique id of the round to be deleted
+ * @returns -- true if user confirms delete, false otherwise
+ *************************************************************************/
+function confirmDelete(roundId) {
+  //TO DO: Present modal dialog prompting user to confirm delete
+  //Return true if user confirms delete, false otherwise
+  let modal = new bootstrap.Modal(
+    document.getElementById("confirmDeleteRoundModal")
+  );
+  let confirmBtn = document.getElementById("confirmDeleteBtn");
+  confirmBtn.addEventListener("click", function (event) {
+    event.preventDefault();
+    console.log("deleting round with id " + roundId);
+    for (var i = 0; i < GlobalRoundsTable.rows.length; i++) {
+      let row = GlobalRoundsTable.rows[i];
+      // Check if the id of the row matches the id you're looking for
+      if (row.id === "r-" + roundId) {
+        GlobalRoundsTable.deleteRow(i);
+        break;
+      }
+    }
+    deleteRound(roundId);
+    localStorage.setItem(
+      GlobalUserData.accountInfo.email,
+      JSON.stringify(GlobalUserData)
+    );
+    GlobalRoundsTableCaption.textContent =
+      "Table displaying " +
+      (GlobalRoundsTable.rows.length - 1) +
+      " speedgolf rounds";
+    modal.hide();
+  });
+  modal.show();
+}


Well implemented function, it's easy to follow and the logic is clear. Just a couple of things to consider below

MedusCode · 2025-05-05T23:45:29Z

+ * @desc
+ * Present pop-up modal dialog asking user to confirm delete operation
+ * @param roundId -- the unique id of the round to be deleted
+ * @returns -- true if user confirms delete, false otherwise


The function doesn't return anything, but the comment says it returns a boolean. To avoid confusion, it might be a good idea to either update the comment or return a boolean.

MedusCode · 2025-05-05T23:46:47Z

+ * @param roundId -- the unique id of the round to be deleted
+ * @returns -- true if user confirms delete, false otherwise
+ *************************************************************************/
+function confirmDelete(roundId) {


Would it make sense to add a quick check for roundId type? This could help prevent issues with unexpected input.

MedusCode · 2025-05-05T23:48:24Z

+  let modal = new bootstrap.Modal(
+    document.getElementById("confirmDeleteRoundModal")
+  );
+  let confirmBtn = document.getElementById("confirmDeleteBtn");


Consider moving HTML elements to global constants in main.js and checking they're not undefined before use for better reliability.

MedusCode · 2025-05-05T23:50:20Z

+    document.getElementById("confirmDeleteRoundModal")
+  );
+  let confirmBtn = document.getElementById("confirmDeleteBtn");
+  confirmBtn.addEventListener("click", function (event) {


Each time confirmDelete runs, a new click handler is added. To prevent duplicates, consider using { once: true } when adding the listener.

MedusCode · 2025-05-05T23:51:51Z

+  confirmBtn.addEventListener("click", function (event) {
+    event.preventDefault();
+    console.log("deleting round with id " + roundId);
+    for (var i = 0; i < GlobalRoundsTable.rows.length; i++) {


Consider switching to let for better block scoping and modern JS practice.

You could also use findIndex() instead of the loop to make the code shorter and more readable.

MedusCode

Good job with deleteRound function. Check my comments for improvement.

MedusCode · 2025-05-05T23:57:25Z

+/*************************************************************************
+ * @function deleteRound
+ * @desc
+ * Deletes a round from the "Rounds" table and from local storage
+ * @param roundId -- the unique id of the round to be deleted
+ * @returns -- true if round could be deleted, false otherwise
+ *************************************************************************/
+function deleteRound(roundId) {
+  GlobalUserData.rounds = GlobalUserData.rounds.filter(function (round) {
+    return round.roundNum !== roundId;
+  });
+}
+


Good function implementation. Using filter to remove the round is a good idea.

MedusCode · 2025-05-05T23:59:42Z

+/*************************************************************************
+ * @function deleteRound
+ * @desc
+ * Deletes a round from the "Rounds" table and from local storage


The docstring says this deletes from both a "Rounds" table and local storage, but it looks like it only updates the GlobalUserData.rounds array. You might want to tweak the description so it doesn't cause confusion later.

MedusCode · 2025-05-06T00:01:14Z

+ * @desc
+ * Deletes a round from the "Rounds" table and from local storage
+ * @param roundId -- the unique id of the round to be deleted
+ * @returns -- true if round could be deleted, false otherwise


The function doesn't return anything, but the comment says it returns a boolean. To avoid confusion, it might be a good idea to either update the comment or return a boolean.

MedusCode · 2025-05-06T00:01:43Z

+ * @param roundId -- the unique id of the round to be deleted
+ * @returns -- true if round could be deleted, false otherwise
+ *************************************************************************/
+function deleteRound(roundId) {


Would it make sense to add a quick check for roundId type? This could help prevent issues with unexpected input.

MedusCode · 2025-05-06T00:02:38Z

+  GlobalUserData.rounds = GlobalUserData.rounds.filter(function (round) {
+    return round.roundNum !== roundId;
+  });


Consider using an arrow function for brevity and modern JS style.

MedusCode added 3 commits May 6, 2025 02:53

Create the HTML for a 'Confirm Delete' modal dialog box (#1)

580b709

Add confirmDelete function (#1)

4ad4ebc

Add deleteRound function (#1)

42f2b38

MedusCode requested review from a team as code owners May 5, 2025 22:55

MedusCode commented May 5, 2025

View reviewed changes

MedusCode commented May 6, 2025

View reviewed changes

Conversation

MedusCode commented May 5, 2025

Description

Related Issue(s)

Type of Change

Testing

Pre-Submission Checklist

Uh oh!

MedusCode left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MedusCode left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant