This repository has been archived by the owner on Apr 12, 2023. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
DebugPage の実装 #178
DebugPage の実装 #178
Changes from 2 commits
92fb986
c22df7f
47f32e3
baa112f
516d974
adc6bfe
77209c1
e9bd943
239f33c
39e03a3
3857b4a
b8f90a3
9273bcb
80128f5
7d64bc3
4f594a2
8fbbc10
2185ec7
857bc1c
da3f2c1
051ab33
243a3d3
de97c2e
0bda3cf
489700c
24be80f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
[提案] Debug Page 関連は他の画面と少し毛色が違うのでディレクトリ切ってそこに入れませんか?
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.
このページだけのディレクトリを作るのは避けたいのですが、既存のもの(HelpPage, Settings..)だとどれが良いでしょうか?現状のページ配置も、それほど意味を見出せないのですが・・
・・・と、私は悩んで、どれでもいいかとなってしまっています。
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.
DebugはDebugというだけでドメインが違うので単体でフォルダを切る価値があると思いました。
下手にフォルダの中に入れるのではなく、Viewの直下においても良いかもですね。
それな...
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.
もし開発チームに意向が
あればこちらに届けば反映する、という方針にして、ここは見えるようにおいておきます。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.
これは要検討ですね。
itemGroupのような仕掛けでReleaseビルドに含めないようにするのなら、1つのディレクトリ(たとえば
DebugPage
など)に集約されていることに意味はあると思います。一方で、今の名前空間が
View
やViewModel
といった分け方なので、そこにいきなり機能単位のDebugPage
が来るのは違和感が拭えません。となると、
View/DebugPage
とViewModel/DebugPage
のようにするのが納得感がある反面、ディレクトリが深くなっただけで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.
参考までに、元々あったデバッグページは設定ページと同じディレクトリに置いてありました。
https://github.com/cocoa-mhlw/cocoa/tree/4c4aca39b02dcb1bd89569674b65b0ef6a357bae/Covid19Radar/Covid19Radar/Views/Settings
同じ場所に置くと GitHub の History ボタンから履歴が見やすくなると思います。
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.
少なくとも HomePage/ よりは、まともな根拠と思いますので、Settings/ に変え
たいですね。ました。80128f5
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.
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.
他にも、COCOAでは公開関数の名前は大文字から始める様になっています(僕はそう認識しています)ので、そちらに合わせるべきでしょう。(Visual Studio からは Ctrl+R, Ctrl+R で名前を変更できます)
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.
元々Stringで
Android
,iOS
と取得できるものを再度変換かけてString 取得している理由は何ですか?現状、iOS, Android しかサポートしてないのでそこまで考える必要はなさそう
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.
この switch 文が別ファイルにあったので、安全のためそれに倣いました。将来を考えて、 default を付けています。
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.
こちらの8個目にあるように必要以上に安全な振りすぎてると思いますが、どうでしょう?
https://medium.com/edge-coders/the-mistakes-i-made-as-a-beginner-programmer-ac8b3e54c312
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.
ここはもう、次のようにまとめてしまうのでどうでしょう。iOSとAndroid以外への対応は、要件が出てきたときに考えましょう。
ちなみに、このページは
DEBUG
が有効の時にしか見えないものなので、if DEBUG
はなくてもいいかなと思いました。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 DEBUG はつけておきたいのですが良いでしょうか?手元では、 Release ビルドも実施しているので。
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.
あまり詳しくないのですが、UserDalogs へ直接参照を持つのではなく、代わりに IUserDialogs を参照するようにするのはどうですか?
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.
UserDialogsのInstanceを使っているのは
Acr.UserDialogs
(外部イブラリでCOCOAの範疇外という認識)の通常の使い方のようです。指摘の意図としては、UserDialogsのInstanceを一旦、IUserDialogsのプロパティやメンバ変数としてクラスに持った方がいいということでしょうか。InstanceがSingletonであればこのままでもいいような気がします。
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.
これも同じくInterfaceへ依存するようにすると良さそう。
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.
本質的ではないのですが、虫Icon があるようですね。そちらにしたくなってきました。
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.
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.
デバッグ画面のカラムは一番下に置きましょう(あまり主張したくないので)。