Skip to content

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

6. Zigzag Conversion #58

wants to merge 1 commit into from

Conversation

fhiyo
Copy link
Owner

@fhiyo fhiyo commented Aug 25, 2024

direction = -1
zigzag_rows[row].append(c)
row += direction
return ''.join(map(lambda row: ''.join(row), zigzag_rows))

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文で書いてしまってもいい気がしています。
これぐらいだったら書いても大丈夫なのかな?(コメントあった方が分かりやすい気も, します)

Copy link

Choose a reason for hiding this comment

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

個人的には for 文でストレートフォワードに書いたほうが読みやすいように感じました。

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せずに文字列を入れておくというのもあるかなと思います。その方が全体的にちょっと読みやすくなるかもしれません。

Copy link

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)

Copy link
Owner Author

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の最適化を考えなければ文字列の構築にコストがかかる気がしました (何か勘違いしてたらすみません)。

Copy link
Owner Author

Choose a reason for hiding this comment

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

(generatorの話被っちゃいました)

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

Choose a reason for hiding this comment

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

こっちの書き方であれば、numRows == 1のケースを特別扱いしなくても良さそうですね。

Copy link
Owner Author

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

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とかですかね。

Copy link
Owner Author

Choose a reason for hiding this comment

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

i -> indexで分かりやすくなるのか自分ははっきりとは分からなかったですが、j -> rowはたしかにと思いました

Comment on lines +26 to +29
if row == 0:
direction = 1
elif row == numRows - 1:
direction = -1

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

でも書けそうですね。

Copy link
Owner Author

Choose a reason for hiding this comment

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

その書き方も迷ったんですよね...どちらでも良いと思いましたが、反転させるやり方だと今のdirectionがどっちなのか一応意識しないとダメかなと思って選ばなかった感じでした。自分の書いた方だと最上段に行ったら下に、最下段に行ったら上に行くことが分かりやすいかもと思い。

Comment on lines +181 to +182
if numRows == 1:
return s

Choose a reason for hiding this comment

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

len(s) <= numRow の場合にもそのまま s を返してあげると余計な計算が発生しないのでよさそうです。

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.

8 participants