Skip to content

Conversation

aoshi2025s
Copy link
Owner

@aoshi2025s
Copy link
Owner Author

webhook test comment

Comment on lines +21 to +23
ans.push_back(i);
ans.push_back(j);
break ;
Copy link

Choose a reason for hiding this comment

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

シンプルに return {i, j}; でも良さそうです。

Choose a reason for hiding this comment

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

このbreakのみだと、外側のループから抜けられていないですね。

Copy link
Owner Author

Choose a reason for hiding this comment

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

breakは全部のループ抜けるものだと勘違いしていました。。。

unordered_map<int, int> mp;

for (int i = 0; i < nums.size(); i++) {
if (mp.count(target - nums[i]) > 0) {
Copy link

Choose a reason for hiding this comment

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

unordered_map<int, int>::find を使ってもいいですね。
https://en.cppreference.com/w/cpp/container/unordered_map/find

Choose a reason for hiding this comment

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

他にもcontainsを使えばシンプルに書けると思いました。
https://en.cppreference.com/w/cpp/container/unordered_map/contains

}
numsMap[nums[i]] = i;
}
return {};
Copy link

Choose a reason for hiding this comment

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

適切な引数が渡されればここはunreachableなので、選択肢としては例外を投げてもいいですね。
参考 https://discord.com/channels/1084280443945353267/1218740927120674977/1223967037588635658

Choose a reason for hiding this comment

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

他の選択肢としては、空配列を返しつつエラーログを吐くという手もあります

どういう選択肢を取るかはこの関数が誰のためにどのような場面で使われるか(ユースケース)によって変わってくるので、なにか前提があって書いたときはコメントに残しておくと良さそうです

参考: Yoshiki-Iwasa/Arai60#13 (comment)

class Solution {
public:
vector<int> twoSum(vector<int>& nums, int target) {
unordered_map<int, int> numsMap;

Choose a reason for hiding this comment

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

命名はもう少し工夫しても良いかなと思います。mapということは型を見れば分かるので実質変数名の情報がnumsしかなくて引数のnumsと何が違うんだろうと思いました。

class Solution {
public:
vector<int> twoSum(vector<int>& nums, int target) {
unordered_map<int, int> numsMap;
Copy link

@Yoshiki-Iwasa Yoshiki-Iwasa Jul 5, 2024

Choose a reason for hiding this comment

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

補数(complement)とindexの対応を取ってる と思うのでそれを表現した方がいいかもしれません

あ、補数はこのmapにいれてないや
失礼しました

unordered_map<int, int> numsMap;

for(int i = 0; i < nums.size(); i++) {
if (numsMap.count(target - nums[i]) > 0) {

Choose a reason for hiding this comment

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

containsでも良さそうです

Choose a reason for hiding this comment

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

このようにもできます。

if (auto it = numsMap.find(target - nums[i]); it != numsMap.end()) {
  return {it->second, i};
}

class Solution {
public:
vector<int> twoSum(vector<int>& nums, int target) {
unordered_map<int, int> mp;
Copy link

Choose a reason for hiding this comment

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

変数名 mp ですと、中にどのような値が含まれているか想像しにくく感じます。 num とその index がはいっていますので、 num_to_index といった名前はいかがでしょうか?

class Solution {
public:
vector<int> twoSum(vector<int>& nums, int target) {
unordered_map<int, int> numsMap;

Choose a reason for hiding this comment

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

念のためですが、googleのスタイルガイドではスネークケースが推奨されております。
https://google.github.io/styleguide/cppguide.html#Variable_Names

Choose a reason for hiding this comment

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

@hayashi-ay さんも指摘されておりますが、変数名を見て何をいれているのかわかる命名がいいと思います。mapは型定義から分かるので変数名に加える必要はないと思いました。

@rihib rihib mentioned this pull request Aug 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants