Skip to content
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

West Midlands| Danial Bashirzadeh | Module-Data-Flows | Week2 | Debugging | book-library #134

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

bashirzadehdanial
Copy link

@bashirzadehdanial bashirzadehdanial commented Jan 10, 2025

Learners, PR Template

Self checklist

  • I have committed my files one by one, on purpose, and for a reason
  • I have titled my PR with COHORT_NAME | FIRST_NAME LAST_NAME | REPO_NAME | WEEK
  • I have tested my changes
  • My changes follow the style guide
  • My changes meet the requirements of this task

Changelist

Briefly explain your PR.

Questions

Ask any questions you have for your reviewer.

@bashirzadehdanial bashirzadehdanial added the Needs Review Participant to add when requesting review label Jan 10, 2025
Copy link

@cjyuan cjyuan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In terms of input validation,

  1. Are all input properly checked?
  2. Can .value be null?
  3. What if a user enters only space characters in the "title" input field?
  4. What if a users enters -1 or 3.1416 in the "pages" input field?

@@ -54,7 +54,7 @@ function render() {
let table = document.getElementById("display");
let rowsNumber = table.rows.length;
//delete old table
for (let n = rowsNumber - 1; n > 0; n-- {
for (let n = rowsNumber - 1; n > 0; n--) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you think of a more efficient way to remove all rows (except the heading) in the table?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A more efficient way to remove all rows except the heading is to clear the tbody directly.

delBut.className = "btn btn-warning";
delBut.innerHTML = "Delete";
delBut.addEventListener("clicks", function () {
delButton.id = i + 5;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Do you think the value assigned to this id attribute is unique?
  • Do you think there is a need to assign any id attribute to delButton?
  • Do you think there is a need to assign any id attribute to changeBut (at line 75)?
  • Can you think of a more consistent way to name the variables representing the two buttons?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1- No, the id values aren't guaranteed to be unique.
2- No, id for delButton is unnecessary.
3- No, id for changeBut isn't needed.
4- More descriptive names like deleteButton and toggleReadButton.

@@ -17,6 +17,8 @@ Take a look at the following code:

Explain why line 5 and line 8 output different numbers.

The variable at line 5 is global, so it returns 1, while the variable at line 8 is local to the function. Thus, when the function is called, it logs the x variable from its scope.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The variable at line 5 is definite not global.
I wonder how ChatGPT would answer this question.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I provided the code to chatGPT and received "Line 5 logs 2, Line 8 logs 1", which I believe is wrong as the output logs 2 for both because the variable is updated.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant line 1 is global, not line 5, it was my bad.

@@ -35,6 +37,8 @@ console.log(y);

What will be the output of this code. Explain your answer in 50 words or less.

The output will be 10 and undefined, as the y variable is scoped within the function and can't be accessed globally.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you try executing the code to verify your answer?

The second statement is correct about y, but it does not explain why the first output is 10.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Output is 10 because it accesses the global variable x.

@@ -62,3 +66,5 @@ console.log(y);
```

What will be the output of this code. Explain your answer in 50 words or less.

The output will be 9 and {x: 10} because of the different ways the variables are passed. In f1, x is passed by value, so the function doesn't alter its original value. In f2, y is passed by reference, so changes within the function modify the original value of y.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spot on.

@cjyuan cjyuan added Reviewed Volunteer to add when completing a review and removed Needs Review Participant to add when requesting review labels Jan 16, 2025
@bashirzadehdanial bashirzadehdanial added the Complete Participant to add when work is complete and review comments have been addressed label Jan 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Complete Participant to add when work is complete and review comments have been addressed Reviewed Volunteer to add when completing a review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants