-
Notifications
You must be signed in to change notification settings - Fork 0
6. Zigzag Conversion #58
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
base: main
Are you sure you want to change the base?
Conversation
direction = -1 | ||
zigzag_rows[row].append(c) | ||
row += direction | ||
return ''.join(map(lambda row: ''.join(row), zigzag_rows)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
いいと思います。
最後のreturnだけ少し気になっているところで, 自分が昔書いた時は
return "".join(row_char for row_string in row_strings for row_char in row_string)
と書いていました。これに比べたらまだ
return ''.join(map(lambda row: ''.join(row), zigzag_rows))
の方が読みやすい気もしますが, これでも認知負荷がかかるなら普通にfor文で書いてしまってもいい気がしています。
これぐらいだったら書いても大丈夫なのかな?(コメントあった方が分かりやすい気も, します)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
個人的には for 文でストレートフォワードに書いたほうが読みやすいように感じました。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
map(lambda
よりは個人的には内包表記の方が好きですが、内包表記は結構好みが分かれる気はします。
普通に書いた方がわかりやすいですかね。
return ''.join([''.join(row) for row in zigzag_rows])
あとは、zigzag_rows: list[list[str]]ではなくてzigzag_rows: list[str]にして、rowにappendせずに文字列を入れておくというのもあるかなと思います。その方が全体的にちょっと読みやすくなるかもしれません。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[] なくても、Generator が返るので動きます。
''.join(''.join(row) for row in zigzag_rows)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
map, 結構分かりにくいですかね。(lambdaという文字列が長いのは微妙に思います)
内包表記的に自分が書くなら、中間状態でメモリを使うlistより generator expressionにしそうです。
for文で作るのも同様にメモリ使いそうですね。もちろんユースケースにも依りますが、今回のmapくらいなら可読性の低下よりメモリ効率取っていいんじゃないか?という感覚でした。
rowにappendせずに文字列を入れておく
これは文字列がimmutableだから、CPythonの最適化を考えなければ文字列の構築にコストがかかる気がしました (何か勘違いしてたらすみません)。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(generatorの話被っちゃいました)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generatorはそうですね。文字列の構築も、多分id(等)のlistを保持するより高コストなんじゃないかなと思います。
(Leetcodeのテストケースぐらいでは初期化の方が支配的でした)
このコメントの趣旨は、関数のネストが少なくなるので、ちょっと読みやすくなるかも?ぐらいでした。
return ''.join(zigzag_rows)
def convert(self, s: str, numRows: int) -> str: | ||
assert numRows > 0 | ||
if numRows == 1: | ||
return s |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
こっちの書き方であれば、numRows == 1のケースを特別扱いしなくても良さそうですね。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
あーたしかにこの実装だとそうなりますね...ありがとうございます 🙏
if numRows == 1: | ||
return s | ||
zigzag_rows = [[] for _ in range(numRows)] | ||
i = 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i, jはもう少しわかりやすくできる気もしました。i -> index, j -> rowとかですかね。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i -> indexで分かりやすくなるのか自分ははっきりとは分からなかったですが、j -> rowはたしかにと思いました
if row == 0: | ||
direction = 1 | ||
elif row == numRows - 1: | ||
direction = -1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
direction = -1
で初期化している場合、
if row == 0 or row == numRows - 1:
direction = -direction
でも書けそうですね。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
その書き方も迷ったんですよね...どちらでも良いと思いましたが、反転させるやり方だと今のdirectionがどっちなのか一応意識しないとダメかなと思って選ばなかった感じでした。自分の書いた方だと最上段に行ったら下に、最下段に行ったら上に行くことが分かりやすいかもと思い。
if numRows == 1: | ||
return s |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
len(s) <= numRow
の場合にもそのまま s
を返してあげると余計な計算が発生しないのでよさそうです。
https://leetcode.com/problems/zigzag-conversion/description/