Skip to content

Create 125. Valid Palindrome.md#5

Open
kzhra wants to merge 3 commits into
mainfrom
125.-Valid-Palindrome
Open

Create 125. Valid Palindrome.md#5
kzhra wants to merge 3 commits into
mainfrom
125.-Valid-Palindrome

Conversation

@kzhra

@kzhra kzhra commented Jul 4, 2024

Copy link
Copy Markdown
Owner

Comment thread 2nd
}
if( std::tolower(s[start_index]) != std::tolower(s[end_index]) ) {
return false;
} else {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

私は、この else はなくして、中身を下に下げます。インデントを深くしたくないためです。

Comment thread 2nd
int start_index = 0; //from_start?, start_to_end_index,
int end_index = s.length() - 1;

while(start_index <= end_index) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

== のときは、もう抜けていい気がします。

Comment thread 2nd
--end_index;
continue;
}
if( std::tolower(s[start_index]) != std::tolower(s[end_index]) ) {

@NobukiFukui NobukiFukui Jul 5, 2024

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

C++はあまり触っていないので,間違いがあるかもしれませんが,こちらの方法はいかがでしょうか.

 if( std::tolower(s[start_index]) == std::tolower(s[end_index]) ) {
    ++start_index;
    --end_index;
   continue
}
return False

参考
https://github.com/t-ooka/leetcode/pull/6/files
のstep4です.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

whileの外にreturn Trueでしょうか

Comment thread 125. Valid Palindrome.md

vector<char> transformed_string; // normalized, standar~

for(char c: s) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

キーワードのあとに空白を空けることをお勧めいたします。
https://google.github.io/styleguide/cppguide.html#Horizontal_Whitespace

if (b) { // Space after the keyword in conditions and loops.

また、 : の前後に空白を空けることをお勧めいたします。
https://google.github.io/styleguide/cppguide.html#Horizontal_Whitespace

// Range-based for loops always have a space before and after the colon.
for (auto x : counts) {

Comment thread 125. Valid Palindrome.md
vector<char> transformed_string; // normalized, standar~

for(char c: s) {
if('a' <= c && c <= 'z'){

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

{ の前に空白を空けることをお勧めいたします。

https://google.github.io/styleguide/cppguide.html#Horizontal_Whitespace

void f(bool b) { // Open braces should always have a space before them.

Comment thread 125. Valid Palindrome.md

for(char c: s) {
if('a' <= c && c <= 'z'){
transformed_string.push_back(c);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

std::tolower() は 'a'~'z'、'0'~'9' に対しては、入力をそのまま返します。これを利用し、 if 文をまとめてしまったほうが読みやすくなると思います。

for (char c : s) {
    if (isalnum(c)) {
        transformed_string.push_back(std::tolower(c));
    }
}

Comment thread 125. Valid Palindrome.md
transformed_string.push_back(c);
}else if('A' <= c && c <= 'Z') {
transformed_string.push_back(std::tolower(c));
continue;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

if else 文で continue が入っているのと入っていないのとで一貫性が取れていない点が気になりました。この書き方であれば、 continue はすべて省き、 else 文を消したほうが読みやすくなると思います。

Comment thread 125. Valid Palindrome.md

int n = transformed_string.size();
for(int i = 0; i < n / 2; ++i) {
if(transformed_string[i] == transformed_string[n - 1 - i]) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

条件を反転して early return したほうが読みやすく感じます。

for (int i = 0; i < n / 2; ++i) {
    if (transformed_string[i] != transformed_string[n - 1 - i]) {
        return false;
    }
}

Comment thread 125. Valid Palindrome.md
} else {
return false;
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

インデントがそろっていないように見えます。

Comment thread 2nd
int end_index = s.length() - 1;

while(start_index <= end_index) {
if( !isalnum(s[start_index]) ) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

() の内側に空白を空けても大丈夫だと思うのですが、他の箇所と一貫性を取る事をお勧めいたします。
https://google.github.io/styleguide/cppguide.html#Horizontal_Whitespace

// Loops and conditions may have spaces inside parentheses, but this
// is rare. Be consistent.
switch ( i ) {
if ( test ) {
for ( int i = 0; i < 5; ++i ) {`

Comment thread 2nd
++start_index;
continue;
}
if( !isalnum(s[end_index]) ) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

ここはifではなく、whileにして、ブロック内のcontinueをなくすと良さそうです。
コードを読む時に親ブロックのwhileまで戻らずに済むため、読みやすくなります。

Comment thread 125. Valid Palindrome.md
continue;
} else {
// c が英数字以外の場合の処理
continue;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

https://github.com/kzhra/Grind41/pull/5/files#r1667239326
の指摘と被りますが
あってもなくても挙動がおなじになるcontinueが入っているのが気になりました。
書くべき理由がなさそうなので、消したほうが読みやすくなりそうです。

Comment thread 2nd
class Solution {
public:
bool isPalindrome(string s) {
int start_index = 0; //from_start?, start_to_end_index,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

僕のレビューでも頂いたのですが、ポインタの変数名についてはleft,rightのほうがわかりやすいというアイデアもあります。

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.

6 participants