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

feat: add ts solution to lc problem: No.323 #2990

Merged
merged 1 commit into from
Jun 1, 2024

Conversation

rain84
Copy link
Contributor

@rain84 rain84 commented Jun 1, 2024

No description provided.

@idoocs
Copy link
Member

idoocs commented Jun 1, 2024

🤭 感谢你的提交,请检查你的改动是否符合以下项目规范。

1. 格式化

我们项目中各种编程语言代码(包括文档)所采用的格式化工具不同,提交 pr 之前必须确保代码、文档正确格式化。

  • .{md,js,ts,php,sql,rs} 采用 prettier
  • .{c,cpp,java} 采用 clang-format
  • .{py} 采用 black
  • .{go} 采用 gofmt
  • 其它待完善

2. Git 提交信息

我们项目遵循 AngularJS Git Commit Message Conventions 规范,我们希望你的提交信息尽可能与项目保持一致。

  • 新增或修改题解:feat: add/update solution(s) to lc problem(s): No.xxxx
  • 修复错误:fix: xxxx
  • 日常维护:chore: xxx

3. 其它补充

新增题解及代码时,需要创建 Solution.xxx 源代码文件(如果已存在,请确认算法是否更优,是则覆盖已有算法代码),同时,需要在 README.md 以及 README_EN.md 中添加对应的代码片段(英文文件中不要出现中文注释)
另外,编码风格(比如变量、函数的命名),尽量跟项目已有代码保持一致。


🤭 Thank you for your contribution. Please check if your changes comply with the following project specifications.

1. Formatting

We use different formatting tools for various programming languages (including documentation) in our project. You must ensure that the code and documentation are correctly formatted before submitting a pr.

  • .{md,js,ts,php,sql,rs} use prettier
  • .{c,cpp,java} use clang-format
  • .{py} use black
  • .{go} use gofmt
  • Others to be improved

2. Git Commit Message

Our project follows the AngularJS Git Commit Message Conventions. We hope that your submission information is as consistent as possible with the project.

  • Add or modify solutions: feat: add/update solution(s) to lc problem(s): No.xxxx
  • Fix errors: fix: xxxx
  • Routine maintenance: chore: xxx

3. Other notes

When adding solutions and code, you need to create a Solution.xxx source code file (if it already exists, please confirm whether the algorithm is better, if yes, overwrite the existing algorithm code), and at the same time, you need to add the corresponding code snippets in README.md and README_EN.md (do not have Chinese comments in the English file)
In addition, the coding style (such as the naming of variables and functions) should be as consistent as possible with the existing code in the project.

@idoocs idoocs added md Issues or Pull requests relate to .md files ts Issues or Pull requests relate to .ts code labels Jun 1, 2024
}
};

for (const i of Object.keys(g)) {
Copy link
Member

Choose a reason for hiding this comment

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

In a for loop, there is no need to use Object.keys(g). Since the length of g is n, you can just use for (let i = 0; i < n; i++).

The advantage of this is that you can not use + to turn the number type, easier to read for newcomers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice advice, thanks a lot!
Sometimes old and time-tested solutions begin to be forgotten )

Copy link
Member

Choose a reason for hiding this comment

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

It's nice to meet TS developers in this repository to discuss more elegant code together. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It’s mutual )

Copy link
Member

Choose a reason for hiding this comment

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

We're both React users as well, so maybe there's a lot more to discuss in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, and I am with React ecosystem since Nov. 2016, but with a few interruptions )

Comment on lines 518 to 523
for (const i of Object.keys(g)) {
if (vis.has(+i)) continue;

dfs(+i);
ans++;
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
for (const i of Object.keys(g)) {
if (vis.has(+i)) continue;
dfs(+i);
ans++;
}
for (let i = 0; i < n; i++) {
if (!vis.has(i)) {
dfs(i);
ans++;
}
}

Copy link
Member

@thinkasany thinkasany Jun 1, 2024

Choose a reason for hiding this comment

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

From a code readability point of view, if (!vis.has(i)) { dfs(i); ans++; } might be better. This is because it explicitly expresses the logic of “if node i has not been visited, perform DFS and increase the number of connected components”, instead of using continue to skip nodes that have already been visited. This makes the logic of the code clearer and easier to understand.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From the 1st glance yes, but... let't look from another point of view.
Every algorithm in most case is transformation input data into output.
And all the data can be considered as valid and invalid (simple binary choiсe, like in the "Matrix" movie))) And when invalid data rejected as early as possible common structure of the algorithm becomes more "flat", and cyclomatic and cognitive complexity is reduced. Also it is possible to draw an analogy with the domain of definition of a function from the school course of math. That is why usually I am trying to throw away wrong cases as early as possible. Also this approach is called as "eager return", and it shows itself in all its glory in the case of a large amount of code.
And I am agree, in the current task it's advantages are not so obvious. But I am using it out of habbit over the years.
And also it make sense to add that you shouldn't make every good rule in absolut. :)

Copy link
Member

Choose a reason for hiding this comment

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

I agree with you that continue is indeed pretty much the same whether to change it or not, there are pros and cons to each, and it's arguably a bit simpler to show if you don't use it over here. But Object.keys(g) needs to be changed, and I see that you've changed it as well.

The reason is as follows:

Performance: object.keys(g) creates a new array containing all the keys of g, which takes extra time and space. In contrast, for (let i = 0; i < n; i++) simply traverses a range, which is better performance.

Readability: for (let i = 0; i < n; i++) clearly expresses the range you want to traverse (from 0 to n-1), whereas const i of Object.keys(g) requires the reader to know that g is an object whose keys are the elements you want to traverse.

Type safety: in const i of Object.keys(g), i is a string, even though the keys of g are actually numbers. This can lead to type errors; for example, you may need to use +i to convert i to a number. In contrast, i in for (let i = 0; i < n; i++) is always a number, and there are no type problems.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am agree with you, and thank you for explanation.

@thinkasany
Copy link
Member

thinkasany commented Jun 1, 2024

Thanks for your contribution, can you help us to add the README.md part of the documentation, which will help us to switch to the internationalization of the repository!

https://github.com/doocs/leetcode/blob/main/solution/0300-0399/0323.Number%20of%20Connected%20Components%20in%20an%20Undirected%20Graph/README.md

image

Comment on lines 519 to 521
if (vis.has(+i)) continue;

dfs(+i);
Copy link
Member

Choose a reason for hiding this comment

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

i is a number, so you don't need to use + to convert it.

Suggested change
if (vis.has(+i)) continue;
dfs(+i);
if (vis.has(i)) continue;
dfs(i);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, just forgot to remove

@rain84
Copy link
Contributor Author

rain84 commented Jun 1, 2024

Thanks for your contribution, can you help us to add the README.md part of the documentation, which will help us to switch to the internationalization of the repository!

https://github.com/doocs/leetcode/blob/main/solution/0300-0399/0323.Number%20of%20Connected%20Components%20in%20an%20Undirected%20Graph/README.md

image

Why not

Thanks for your contribution, can you help us to add the README.md part of the documentation, which will help us to switch to the internationalization of the repository!

https://github.com/doocs/leetcode/blob/main/solution/0300-0399/0323.Number%20of%20Connected%20Components%20in%20an%20Undirected%20Graph/README.md

image

Why not?
But I am the best in russian lang, not in chinese )
What kind of help do you need?

@thinkasany
Copy link
Member

Why not?
But I am the best in russian lang, not in chinese )
What kind of help do you need?

You don't need to care about the language section, just look at the code section and fill in the same positions

@thinkasany
Copy link
Member

@thinkasany
Copy link
Member

It turns out that it was a member who dealt with this before, so if you can help us add to it, it will help both Chinese users and repository members.
image

@rain84
Copy link
Contributor Author

rain84 commented Jun 1, 2024

I did (I hope). Please check PR again.

@thinkasany thinkasany merged commit 5eb7f4c into doocs:main Jun 1, 2024
4 checks passed
@yanglbme
Copy link
Member

yanglbme commented Jun 1, 2024

Hi @rain84 @thinkasany

The submitted PR actually uses the DFS method, not BFS. A DFS solution is already provided in Solution 1. Therefore, I am considering reverting this PR.

Thank you for your contribution. @rain84

@rain84 rain84 deleted the feature/lc-323 branch June 1, 2024 11:22
@rain84
Copy link
Contributor Author

rain84 commented Jun 1, 2024

Hi @yanglbme

No problem )

But what makes you think it's DFS?
From my point of view we add all the neighbours, layer by layer, step by step
q.push(...graph.get(j)!)

@yanglbme
Copy link
Member

yanglbme commented Jun 1, 2024

@rain84

BFS usually uses a queue and is implemented iteratively, whereas DFS typically uses recursion and requires the system to allocate stack space.

@yanglbme
Copy link
Member

yanglbme commented Jun 1, 2024

I added a new BFS implementation in PR #2991

@yanglbme
Copy link
Member

yanglbme commented Jun 1, 2024

image

Your code uses DFS recursion instead of a BFS queue.

@rain84
Copy link
Contributor Author

rain84 commented Jun 1, 2024

@rain84

BFS usually uses a queue and is implemented iteratively, whereas DFS typically uses recursion and requires the system to allocate stack space.

Ohh, I see.
I didn't sleep well last night, so I'm a little distracted today

@rain84
Copy link
Contributor Author

rain84 commented Jun 1, 2024

Let me simplify your solution )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
md Issues or Pull requests relate to .md files ts Issues or Pull requests relate to .ts code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants