search

hyudai28 / ft_irc

0 stars 0 forks source link

selectからpollにした。一部serverの中でやっていたものをuserクラスに投げた。commandクラスの作成 #23

Closed fujishit closed 1 year ago

fujishit commented 1 year ago

issue URL

対応内容・対応背景・妥協点

コマンドのパースを作りたいとのことなので、環境整備です。

やったこと

select()からpoll()にした、詳しい違いは分からん。 recvをServer::loop()でやってたけど、User::receiveに移動。併せてUserクラスにCommandクラスを持たせる実装にした。

最後に接続してきた奴が抜けるとexit()するけど、たぶんUser.cppの36行目をコメントアウトすれば落ちなくなりますが、死ぬほどdisconnectって言ってきます、改善の余地あり。 やりました。

やってないこと

当然パースはしてないです。 基本的にパースは

void    User::receive()
{
    /* 省略 */
    command.parse(buffer);
}

でやるか

void    Server::loop()
{
    /* 省略 */

    this->users[accept_fd]->receive();
    this->users[accept_fd]->get_command().parse();
}

みたいな風にするのがいいと思います。 前者は外からコードを見たときに不透明になること。 後者は冗長だしわかりづらい。 が欠点としてあげられます。

disconnectが無限に出力される問題があります。 ないです。

テスト

前回と動作はほぼ変わりません。 違うのは切断するとexit()することだけです。 しません。

レビュー観点

あくまで目安です。

補足

fujishit commented 1 year ago

死ぬほどdisconnectって言ってきます、改善の余地あり。

これユーザーが抜けたフラグを持たせておいて、抜けたらちゃんと消せばなおりそう。

fujishit commented 1 year ago

新しいのpushしたので新しいやつ見てください

hyudai28 commented 1 year ago

接続テストは完了（Limechat2）

fujishit commented 1 year ago
  • [ ] 他の部分と書き方・命名・ディレクトリ構成等が異なっていないか？ Userディレクトリの中にCommandディレクトリを作成するのはどう？

いいと思う。1クラスに1ディレクトリあるぐらいの感じでもいい気もしてる。

  • [ ] 関数、コンポーネントの粒度は適切か？ これに関してもloopが仕方ないとはいえ、さすがにでかすぎるっていう指摘は多分必要。だけど修正は今回にまとめるよりも、この次のPRで取り組むべき課題だと思う。問題点としては修正箇所の影響がでかいのが悩みどころ。

とりあえずaccept()は分けたほうがいいと考えてる。 accept_fdを持ち帰れないから困ってたけど、userクラスを実装したことによって実装できそう。

メンバー関数というものの役割として、そのクラスによくアクセスしたほうがいいものを用意しておくべきだとも思っていて、一回しか呼ばないのであれば、Server::start()はコンストラクタで呼んで、Server::loop()は全部関数に分けてmain()とかで呼んでしまう方がいいんじゃないかなとも思う。ちょっと思想としては過激な気がするのでいい落としどころを見つけたい。

  • [ ] パースについて 　個人的には前者のほうがいいとは思う。ただreceiveっていう命名からは中でパース指定ること、コマンドの処理をしていることが分かりにくい。どうしようかね、これ

俺も前者の方が好き。receiveっていう名前を変えるべきだとは思う。 明らかに責任過多であることは言えるんだけど、CommandクラスをUserクラスのメンバーにしている以上、Serverクラスのメンバとしていじるべきではないというのが個人的な思想。

fujishit commented 1 year ago

Commandディレクトリ作ったのと、makefileをそれ用に少しいじった。 Commandsディレクトリがいるけど、これをそっくり移植していいのかわからんかったからいったん放置した。

hyudai28 commented 1 year ago

Server::start()はコンストラクタで呼んで
CommandクラスをUserクラスのメンバーにしている以上、Serverクラスのメンバとしていじるべきではない

基本的にUserクラスの中にCommandsを入れることに関しては可読性と拡張性が増すから賛成。ただ構造が若干独特だよねってなるから”ft_ircの歩き方”みたいなドキュメント作ってどんな構造かまとめると今後の開発とかレビューとかでやりやすいかもね。 どのクラスがどこまで責任持ってるのか明文化しておきたい

hyudai28 commented 1 year ago

mainが今のところかなりしょうもないからloopをmainにおいてもよくね？ってのはあるよね
loopどうする問題あるけど、とりあえずちょっとずつ切り離して行く方向でいいと思う