fetch_msg_with_id()の返り値

[ikngtty]

https://github.com/Huyu2239/ExpandBot/blob/f6b8782549ae27aa0e51049d9e04fc85a2ba42fe/cogs/on_message.py#L53-L61
「成功したらメッセージを返す、失敗したらエラーの種別を示す文字列を返す」って感じだけど、違う型のものを無理やり同じ枠で返すのはだいぶぎこちないかな。
あと、そもそも種別を文字列で扱うのがぎこちない。普通は enum。
ただ、この場合はエラーに関する情報なので、そもそも exception 吐くのが一番良い。成功時と違う枠で返すってのもこれで達成できるし。

[ikngtty]

今回は exception が一番良いと思うけど、そうじゃない時はどんな感じにするかっていうと、

class FetchMessageResult:
    def __init__(self, is_success, message, error):
        self.is_success = is_success
        self.message = message
        self.error = error

class Expand(commands.Cog):
    def fetch_msg_with_id(self):
        # 成功時
        return FetchMessageResult(True, msg, None)
        # 失敗時
        return FetchMessageResult(False, "", err)

    def find_msgs(self):
        result = self.fetch_msg_with_id()
        if not result.is_success:
            raise result.error
        # 以降、`result.message` を使う

みたいに、返り値を一つのクラスでまとめつつ、成功時の情報と失敗時の情報を別々のプロパティで表現するのが行儀の良いやり方かな。

まあ、ここまで書かなきゃいけなくなることは少ないとは思う。なるべく関数の役割を絞るようにすれば、一度にいろんな情報を返さなきゃいけないことも減るし。違う種類の情報送りたくなることなんてだいたいエラー関係しか無いし、そうすると大体 exception で事足りる。

[ikngtty]

行儀が良い方は、

result = self.fetch_msg_with_id()
if not result.is_success:
    raise result.error

コードを見ただけで「これはフェッチしようとして失敗した時の処理だな」と分かる。

https://github.com/Huyu2239/ExpandBot/blob/f6b8782549ae27aa0e51049d9e04fc85a2ba42fe/cogs/on_message.py#L33-L40

この場合、「フェッチした msg が文字列かどうか」っていうのが「フェッチに失敗したかどうか」って意味なのは、fetch_msg_with_id() の中身までちゃんと見ないと全然分からない。初見殺し。

[ikngtty]

加えて言えば、msg の中身がエラー種別だった時はそれ "msg" じゃないしね。嘘ついてるもんねこれ。正確に書くなら msg_or_error_type みたいな感じ。まだそう書いた方がマシ。

[Huyu2239]

分かりました

[ikngtty]

あと一般的に言って、isinstance() を使う羽目になってるコードは 95 割方何かミスってると思っていい。

このことについて良い記事ないか探したけど思ったより見つかんないや。
まあ isinstance() 使ってるコード持ってきてくれれば、どうダメでどう直すべきか都度説明するよ。

[Huyu2239]

ありがとうございます