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

Add graph_adjacency_matrix for javascript and typescript #355

Merged
merged 5 commits into from
Feb 10, 2023
Merged

Add graph_adjacency_matrix for javascript and typescript #355

merged 5 commits into from
Feb 10, 2023

Conversation

zhuoqinyue
Copy link
Contributor

@zhuoqinyue zhuoqinyue commented Feb 8, 2023

If this PR is related to coding or code translation, please read the contribution guideline, fill out the checklist, and paste the console outputs to the PR.

  • I've tested the code and ensured the outputs are the same as the outputs of reference codes.
  • I've checked the codes (formatting, comments, indentation, file header, etc) carefully.
  • The code does not rely on a particular environment or IDE and can be executed on a standard system (Win, macOS, Ubuntu).

@vercel
Copy link

vercel bot commented Feb 8, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated
hello-algo ⬜️ Ignored (Inspect) Feb 9, 2023 at 4:27PM (UTC)

@krahets
Copy link
Owner

krahets commented Feb 9, 2023

Pending for review by @justin-tse

@zhuoqinyue
Copy link
Contributor Author

zhuoqinyue commented Feb 9, 2023

emm...
I found there existed a repeated edge (0, 2) in the Java code last night.
May I know whether this is a repeated example? @krahets

@krahets
Copy link
Owner

krahets commented Feb 9, 2023

emm... I found there exited a repeated edge (0, 2) in the Java code last night. May I know whether this is a repeated example? @krahets

Thanks for the fix! Please check the commit b973c86

Copy link
Contributor

@justin-tse justin-tse left a comment

Choose a reason for hiding this comment

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

Awesome! Thank you.@zhuoqinyue

/* 删除顶点 */
removeVertex(index) {
if (index >= this.size()) {
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return;
throw new RangeError("Index Out Of Bounds Exception");

Copy link
Contributor

Choose a reason for hiding this comment

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

TypeScript has the same problem. Please address them, thank you!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A question.
why not use return; but use Error
return; isn't equivalent to void?
@krahets @justin-tse

Copy link
Owner

@krahets krahets Feb 9, 2023

Choose a reason for hiding this comment

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

We should let the user know the operation can not be executed.

If using return, it is better to make the function return the removed Node and return None when the index is out of range.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK. I got it.
Thanks for your answer! @krahets

Copy link
Contributor

Choose a reason for hiding this comment

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

If the function is only used to throw the error, then we will use type: never.
Otherwise, we will use the remaining type. Because anything | never is anything. So we will use void here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks @justin-tse

codes/javascript/chapter_graph/graph_adjacency_matrix.js Outdated Show resolved Hide resolved
codes/javascript/chapter_graph/graph_adjacency_matrix.js Outdated Show resolved Hide resolved
@zhuoqinyue
Copy link
Contributor Author

OK. I've fixed all.

Copy link
Contributor

@justin-tse justin-tse left a comment

Choose a reason for hiding this comment

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

@zhuoqinyue Thank you!

@krahets krahets merged commit 321ea1d into krahets:main Feb 10, 2023
@krahets krahets added the code Code-related label Feb 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code Code-related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants