Refactoring/optimization (#114) #151

Takym · 2021-05-04T12:29:34Z

Issue 番号 / Issue ID

アプリ（リリースビルド）のサイズを削減する #114

目的 / Purpose

使われていないコードを一部削除しました。
Android でのみ利用されているクラスを移動しました。
JSON 関連で冗長な関数を削除しました。

破壊的変更をもたらしますか / Does this introduce a breaking change?

[x] Yes
[ ] No

Pull Request の種類 / Pull Request type

[ ] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[x] Refactoring (no functional changes, no api changes) (API は一部変更されています)
[ ] Documentation content changes
[ ] Other... Please describe:

検証方法 / How to test

コードの入手 / Get the code

git clone https://github.com/Takym/cocoa
cd cocoa
git checkout refactoring/optimization
dotnet restore

コードの検証 / Test the code

確認事項 / What to check

正しく動作する。

その他 / Other information

プロジェクトファイルの DefineConstants に REMOVE を指定するとおおよそ元の状態に戻ります。

keiji

ありがとうございます。
まず、masterとの違いを確認したいのであればブランチ切り替えで対応できるので、ifディレクティブで切り替え可能にしなくてもいいかなと思いました。

リファクタリングという観点からは、不要な記述は削除してしまった方がすっきりしますし、C#の流儀をよくわかっていないのですが、プリプロセッサディレクティブは可読性を落とすイメージがあり、可能な限り使いたくないという気持ちがあります。

AndroidExtensionsはAndroidでしか使わないのでと言うのはご指摘の通りです。いっそファイル名もExtensions.csに変えてしまってもいいかもしれません。

Takym · 2021-05-04T17:30:51Z

#if REMOVED と #if !REMOVED は全て削除しました。REMOVED は元のコードに素早く切り替える為に残しておきましたが、不要との事でしたので削除しました。

また、AndroidExtensions は DiagnosisSubmissionParameter に対する操作を行っているので DSPExtensions に改名しました。DSPExtensions で公開している関数は GetAndroidNonce のみであり、 DeviceCheckService.VerifyAsync からのみ参照されているので、全て DeviceCheckService へ移動してもいいのかもしれません。

keiji

確認しました。
いくつかレビューで指摘しましたが、基本的に不要コードの削除とJsonConvertの展開で挙動やテストの範囲が大きく変わると言ったことはなさそうだなと思いました。

今日あたりにユニットテストと、iOSでのビルドチェックしてみます。

Covid19Radar/Covid19Radar.Android/DSPExtensions.cs

Covid19Radar/Covid19Radar/Common/Utils.cs

keiji · 2021-05-05T16:43:34Z

iOSとAndroidでビルドチェックしました。
将来的にはユニットテストも書きたいですね。

`DiagnosisSubmissionParameterExtensions` を `DeviceCheckService` に結合

keiji

LGTM

fumiya-kume · 2021-05-14T18:17:10Z

Covid19Radar/Covid19Radar.Android/Services/DeviceCheckService.cs

@@ -42,5 +34,31 @@ async Task<string> GetSafetyNetAttestationAsync(byte[] nonce)
            using var response = await client.AttestAsync(nonce, AppSettings.Instance.AndroidSafetyNetApiKey);
            return response.JwsResult;
        }
+
+        public static byte[] GetNonce(DiagnosisSubmissionParameter submission)


[質問] GetNonce を Public にしている理由は何故ですか？

元々 public だったものをそのまま引き継いだ為です。元は別のプロジェクトに入っていたので public になっていたのだろうと思います。また、外部に公開しておけば単体テスト処理も書き易いでしょう。（この PR はリファクタリングが目的ですので、ここでは単体テストは書いていません）

せっかく指摘いただきましたし、このあたりはprivateにしておきましょうか。
単体テスト処理を書く必要が出てきたらアクセスコープを見直しましょう。個人的にテストはpublicなメソッドだけで良いと思っているので、DeviceCheckの先をモック化してテストを書くことになるかと思います。

もしこれらのメソッドが壊れやすいもので、テストの柔軟性を最大限確保するなら、このあたりは別のstaticクラスにして置いておくのも一案と思います（それをすると元に戻すことになるのですが…

GetNonce を修正し、付け加えて GetSafetyNetAttestationAsync に private static を書き加えました。

Takym · 2021-06-07T08:20:28Z

@cocoa-devさん、@keijiさん、@heykuroさん、@halskさん

8052045 にて新しいクラス DateTimeUtility が作成されています。これはテストの為に作成されたと思われますが、この PR で削除すべきでしょうか？もしくは、別の PR にてサービス化した方が良いのでしょうか？現在はサービスになっておらず、静的変数を書き換える事で他のクラスへインスタンスを注入しています。

Takym · 2021-06-07T10:25:03Z

https://github.com/Takym/cocoa/tree/refactoring/optimization2 にて更にリファクタリングを行いました。必要であればこのPRへマージします。

keiji

コンフリクトの解消お願いします！

keiji · 2021-06-11T02:28:39Z

ご指摘の DateTimeUtility、テストの時にInstanceを差し替えてダミーの日付を取得するのに使っているんですね。

これはこのままでも動きますが、統一感を考えれば他のServiceと合わせてmockRepositoryから取得する方がきれいですね。ただ、そこまでする必要性があるかと言われると…個人的には迷っていますが、対応するとしても別途にした方が良さそうです。

optimization2 ありがとうございます。そちらはこのPRに依存がないようであれば別Pull Requestでいきましょう！

Takym · 2021-06-11T03:06:22Z

#222 にて新しいPRを建てました。

Takym · 2021-06-11T03:08:53Z

~~本来は新しく Issue を作成してから言うべきですが、DateTimeUtility.Instance は外部から簡単に書き変える事ができるので不具合の原因になりそうです。~~ →#223、#224 にて Issue と PR を作成しました。

keiji · 2021-06-14T01:11:51Z

コンフリクト解消お願いしますー

Takym · 2021-06-14T01:44:08Z

解消しました。

cocoa-dev · 2021-07-15T05:03:34Z

Covid19Radar/Covid19Radar.Android/Services/DeviceCheckService.cs

            return GetSafetyNetAttestationAsync(nonce);
        }

        /// <summary>
        /// Verification device information required for positive submissions
        /// </summary>
        /// <returns>Device Verification Payload</returns>
-        async Task<string> GetSafetyNetAttestationAsync(byte[] nonce)
+        private static async Task<string> GetSafetyNetAttestationAsync(byte[] nonce)


@Takym
質問のみですが、staticに変えた理由はありますか？

Visual Studio から static にして仮想化呼び出しを避ける様警告されたためです。

@Takym
私の環境(Mac)では警告が表示されずどのような意味を持った警告なのかわかりませんでした。
すみませんがその警告がどのような意味か教えていただいてもよろしいでしょうか？

そのメソッドがクラスのインスタンスに依存しないのであれば、staticを指定することで「インスタンスに依存しないこと」を明示できることに加えて、コンパイラがinline展開できるので実行パフォーマンスが上がったりして都合が良い。という話だと記憶しています。

少し前の情報ですが、それっぽい記事を見つけました。
https://ufcpp.net/blog/2018/12/devirtualization/

static を付け静的関数にした場合、コンパイラは直接関数を呼び出す様にコードを生成します。
static を外し動的関数にした場合、コンパイラは関数を仮想呼び出しするコードを生成します。

最近の Visual Studio では静的関数に変更できる関数は極力変更するよう警告を出してくれる様になりました。

また、static を付ければ動的メンバーを参照しないという意思表示にもなります。

参考

https://docs.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1822

https://ufcpp.net/study/csharp/misc_construct.html

https://ufcpp.net/study/csharp/oo_vftable.html

ありがとうございます
私自身がstaticを積極的に使ってこなかったというのもあり疑問に思ってしまいました。
腹落ちしました。

cocoa-dev · 2021-07-15T06:48:24Z

すみませんがコンフリクトの解消もお願いします

Takym · 2021-07-15T07:42:11Z

コンフリクト解消しました。

Takym added 5 commits May 4, 2021 20:56

Common と Model の使われていないコードを削除

9266315

AndroidExtensions を Android 専用プロジェクトに移して復帰

8ba220c

Utils.SerializeToJson と Utils.DeserializeFromJson を削除

a998458

AndroidExtensions.cs の変更を REMOVE で切り替え可能にした。

0ede037

Covid19Radar.Android.csproj を修復

7740638

Takym changed the title ~~Refactoring/optimization~~ Refactoring/optimization (#114) May 4, 2021

keiji reviewed May 4, 2021

View reviewed changes

Takym added 2 commits May 5, 2021 02:22

REMOVED ディレクティブ廃止

8ab6188

クラス AndroidExtensions を DSPExtensions へ改名

b1acbb8

keiji suggested changes May 4, 2021

View reviewed changes

Takym added 3 commits May 5, 2021 04:47

#region の位置ズレを修正。

99712d4

DSPExtensions を改名

e65bd26

DiagnosisSubmissionParameterExtensions を DeviceCheckService に結合

5d0d031

Merge pull request #2 from Takym/refactoring/optimization-dsp

a8a02f5

`DiagnosisSubmissionParameterExtensions` を `DeviceCheckService` に結合

keiji approved these changes May 6, 2021

View reviewed changes

keiji requested a review from cocoa-dev May 6, 2021 01:03

keiji assigned Takym May 6, 2021

fumiya-kume reviewed May 14, 2021

View reviewed changes

Takym added 2 commits May 15, 2021 15:09

アクセス修飾子修正

401608a

Update DeviceCheckService.cs

47d7600

keiji changed the base branch from master to develop June 7, 2021 07:21

keiji self-requested a review June 11, 2021 02:13

keiji suggested changes Jun 11, 2021

View reviewed changes

Takym mentioned this pull request Jun 11, 2021

Refactoring/optimization2 (#114) #222

Open

1 task

Takym mentioned this pull request Jun 11, 2021

DateTimeUtility のインスタンスを外部から簡単に書き変える事がでてしまう #223

Closed

keiji removed the request for review from cocoa-dev June 14, 2021 01:11

Merge branch 'develop' into refactoring/optimization

895ddfa

keiji self-requested a review June 14, 2021 03:38

cocoa-dev reviewed Jul 15, 2021

View reviewed changes

Merge branch 'develop' into refactoring/optimization

bab156e

cocoa-dev approved these changes Jul 15, 2021

View reviewed changes

keiji approved these changes Jul 15, 2021

View reviewed changes

keiji merged commit 6a563f3 into cocoa-mhlw:develop Jul 15, 2021

keiji mentioned this pull request Jul 29, 2021

Add feature migrations mechanism #165

Merged

This was referenced Sep 8, 2021

Release 1.2.6 #376

Merged

Release 1.2.6 #377

Closed

Release 1.2.6 #378

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactoring/optimization (#114) #151

Refactoring/optimization (#114) #151

Takym commented May 4, 2021

keiji left a comment

Takym commented May 4, 2021 •

edited

Loading

keiji left a comment

keiji commented May 5, 2021

keiji left a comment

fumiya-kume May 14, 2021

Takym May 14, 2021 •

edited

Loading

keiji May 15, 2021

Takym May 15, 2021

Takym commented Jun 7, 2021

Takym commented Jun 7, 2021 •

edited

Loading

keiji left a comment

keiji commented Jun 11, 2021

Takym commented Jun 11, 2021

Takym commented Jun 11, 2021 •

edited

Loading

keiji commented Jun 14, 2021

Takym commented Jun 14, 2021

cocoa-dev Jul 15, 2021

Takym Jul 15, 2021

cocoa-dev Jul 15, 2021

keiji Jul 15, 2021

Takym Jul 15, 2021

cocoa-dev Jul 15, 2021

cocoa-dev commented Jul 15, 2021

Takym commented Jul 15, 2021

Refactoring/optimization (#114) #151

Refactoring/optimization (#114) #151

Conversation

Takym commented May 4, 2021

Issue 番号 / Issue ID

目的 / Purpose

破壊的変更をもたらしますか / Does this introduce a breaking change?

Pull Request の種類 / Pull Request type

検証方法 / How to test

コードの入手 / Get the code

コードの検証 / Test the code

確認事項 / What to check

その他 / Other information

keiji left a comment

Choose a reason for hiding this comment

Takym commented May 4, 2021 • edited Loading

keiji left a comment

Choose a reason for hiding this comment

keiji commented May 5, 2021

keiji left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Takym May 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Takym commented Jun 7, 2021

Takym commented Jun 7, 2021 • edited Loading

keiji left a comment

Choose a reason for hiding this comment

keiji commented Jun 11, 2021

Takym commented Jun 11, 2021

Takym commented Jun 11, 2021 • edited Loading

keiji commented Jun 14, 2021

Takym commented Jun 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

参考

Choose a reason for hiding this comment

cocoa-dev commented Jul 15, 2021

Takym commented Jul 15, 2021

Takym commented May 4, 2021 •

edited

Loading

Takym May 14, 2021 •

edited

Loading

Takym commented Jun 7, 2021 •

edited

Loading

Takym commented Jun 11, 2021 •

edited

Loading