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: PalindromeNumber Solution Added #21

Open
wants to merge 21 commits into
base: master
Choose a base branch
from

Conversation

tiazahmd
Copy link

@tiazahmd tiazahmd commented Feb 24, 2020

Left space and complexity analysis as to-do since I don't know how to do them yet.


View rendered src/PalindromeNumber/README.md

@MrHeer MrHeer self-requested a review February 25, 2020 00:12
@MrHeer MrHeer added the 💻 WIP Work in progress label Feb 25, 2020
@MrHeer MrHeer linked an issue Feb 25, 2020 that may be closed by this pull request
Copy link
Member

@MrHeer MrHeer left a comment

Choose a reason for hiding this comment

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

  1. test code with Test.h
  2. update README.md
  3. format code with clang-format
  4. check markdown documentions with markdownlint

src/PalindromeNumber/README.md Outdated Show resolved Hide resolved
src/PalindromeNumber/Solution.cpp Outdated Show resolved Hide resolved
@tiazahmd
Copy link
Author

All of the following requested changes are made:

  • test code with Test.h
  • update README.md
  • format code with clang-format
  • check markdown documentions with markdownlint

Copy link
Member

@MrHeer MrHeer left a comment

Choose a reason for hiding this comment

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

you're not update README.md

.clang-format Outdated Show resolved Hide resolved
src/PalindromeNumber/Solution.cpp Outdated Show resolved Hide resolved
src/PalindromeNumber/README.md Outdated Show resolved Hide resolved
src/PalindromeNumber/README.md Outdated Show resolved Hide resolved
src/PalindromeNumber/Solution.cpp Show resolved Hide resolved
src/PalindromeNumber/Solution.cpp Outdated Show resolved Hide resolved
src/PalindromeNumber/Solution.cpp Outdated Show resolved Hide resolved
@tiazahmd
Copy link
Author

The following changes have been made to the new commit:

  • Updated README.md with time and space complexity
  • Function now returns bool instead of string
  • Removed ct and digCount and made filling vector easier and faster
  • Added one more test cases, with updated expectation and output & expectation type

@MrHeer MrHeer requested review from a team, YinHanbing and Zhongnibug and removed request for a team February 27, 2020 05:39
@MrHeer
Copy link
Member

MrHeer commented Feb 27, 2020

@tiazahmd Can you help update https://github.com/XFreeCoder/LeetCode/blob/master/README.md

Please reset .clang-format

@tiazahmd
Copy link
Author

Updated master README and reset .clang-format.

.clang-format Outdated Show resolved Hide resolved
src/PalindromeNumber/Solution.cpp Outdated Show resolved Hide resolved
@Zhongnibug
Copy link
Collaborator

I think if you could define that a number,xr, which read forward, equal to x reading backward,you will omit vector.And when xr is equal to x,x is a Palindrome Number.

.clang-format Outdated Show resolved Hide resolved
@MrHeer MrHeer changed the title Imtiaz branch feat: PalindromeNumber Solution Added Feb 28, 2020
@tiazahmd
Copy link
Author

I will be out of town until Tuesday. I’ll make the suggested changes once I’m back.

.clang-format Outdated Show resolved Hide resolved
src/PalindromeNumber/Solution.cpp Outdated Show resolved Hide resolved
src/PalindromeNumber/README.md Outdated Show resolved Hide resolved
src/PalindromeNumber/Solution.cpp Outdated Show resolved Hide resolved
@tiazahmd
Copy link
Author

tiazahmd commented Mar 4, 2020

Made suggested changes. I couldn't get @Zhongnibug 's solution yet. Let me know if you guys still want me to work on that, or if the current solution suffices.

}

return false;
};
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
};
}

Comment on lines +28 to +39
for (int i = 0; i < vec.size(); i++) {
if (i == vec.size() - 1)
return true;
else if (vec[i] != vec[vec.size() - 1 - i])
return false;
}

} else {
return false;
}

return false;
Copy link
Member

Choose a reason for hiding this comment

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

@tiazahmd Do you think this is okay, I have tested AC on leetcode.

Suggested change
for (int i = 0; i < vec.size(); i++) {
if (i == vec.size() - 1)
return true;
else if (vec[i] != vec[vec.size() - 1 - i])
return false;
}
} else {
return false;
}
return false;
for (int i = 0; i < vec.size() / 2; i++) {
if (vec[i] != vec[vec.size() - 1 - i])
return false;
}
return true;
} else {
return false;
}

@MrHeer
Copy link
Member

MrHeer commented Mar 5, 2020

Made suggested changes. I couldn't get @Zhongnibug 's solution yet. Let me know if you guys still want me to work on that, or if the current solution suffices.

Your solution is okey, but @Zhongnibug 's solution is better. Of course, for the best code, if you want to continue to optimize, you can refer to the official solution

Copy link
Member

@MrHeer MrHeer left a comment

Choose a reason for hiding this comment

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

Ping @tiazahmd

Comment on lines +47 to +54
test("Test case: ", s.isPalindrome(121), true);
test("Test case: ", s.isPalindrome(120), false);
test("Test case: ", s.isPalindrome(-121), false);
test("Test case: ", s.isPalindrome(11), true);
test("Test case: ", s.isPalindrome(9), true);
test("Test case: ", s.isPalindrome(0), true);
test("Test case: ", s.isPalindrome(10022001), true);
test("Test case: ", s.isPalindrome(1001), true);
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
test("Test case: ", s.isPalindrome(121), true);
test("Test case: ", s.isPalindrome(120), false);
test("Test case: ", s.isPalindrome(-121), false);
test("Test case: ", s.isPalindrome(11), true);
test("Test case: ", s.isPalindrome(9), true);
test("Test case: ", s.isPalindrome(0), true);
test("Test case: ", s.isPalindrome(10022001), true);
test("Test case: ", s.isPalindrome(1001), true);
test("Test case 1", s.isPalindrome(121), true);
test("Test case 2", s.isPalindrome(120), false);
test("Test case 3", s.isPalindrome(-121), false);
test("Test case 4", s.isPalindrome(11), true);
test("Test case 5", s.isPalindrome(9), true);
test("Test case 6", s.isPalindrome(0), true);
test("Test case 7", s.isPalindrome(10022001), true);
test("Test case 8", s.isPalindrome(1001), true);

@MrHeer MrHeer self-requested a review July 21, 2020 02:17
Copy link
Member

@MrHeer MrHeer left a comment

Choose a reason for hiding this comment

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

@tiazahmd Hello, Is there problem?

@tiazahmd
Copy link
Author

@tiazahmd Hello, Is there problem?

No problem. Is there something I have to do to finish this?

@MrHeer
Copy link
Member

MrHeer commented Jul 31, 2020

@tiazahmd I hope you can adopt my suggestion.
After update, you can Resolve conversation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💻 WIP Work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Palindrome Number
3 participants