OneTimePassword を扱うための Model を追加 by ykws · Pull Request #20 · ykws/OneTimePasswordExample

ykws · 2021-11-14T19:15:53Z

対応する Issue が Linked Issues に設定されていることを確認してください

対応内容

OneTimePassword を扱うための Model を追加
設定に関してもこの Model を通して行い、管理する
パスワードの生成も Model を通して行う

未対応事項

設定画面の Picker の初期選択位置が正常に機能しないことがある
- その時は didSelectRowAtIndexPath が呼ばれていない
- settingsItem が選択したセルの位置と異なる

スクリーンショット

変更なし

ykws

変更内容についてコメントしました。

ykws · 2021-11-14T19:19:10Z

+
+@implementation OneTimePasswordSettings
+
+ (instancetype)sharedInstance {


シングルトンにするための処理です。

@ykws
今回はマルチスレッドな実装を行なっていないため、初期化処理に@synchronizedは必要ないのでしょうか？

@NMai-source とても良い指摘です。

結論としては必要ないです。

@synchronized の代わりに dispatch_once を利用しています。
iOS では、これによってシングルトンを実現できるようになっています。

dispatch_once

Objective-Cにおけるシングルトンのお話

Objective-CのSingleton、その歴史的経緯など - Singletonの実装（簡易版）

@ykws
承知致しました。
教えて頂きありがとうございます！
URLも参考になりました。

ykws · 2021-11-14T19:19:47Z

+    return sharedInstance;
+}
+
+- (NSString *)generateOneTimePassword {


ワンタイムパスワード生成はここに移動しました。

ykws · 2021-11-14T19:20:37Z

+}
+
+- (NSString *)digitsString {
+    return [NSString stringWithFormat:@"%d", (int)self.digits];


int のキャストは雑ですが、メニューでの設定値が限定されているので問題ないもの考えています。

ykws · 2021-11-14T19:21:29Z


 #import "SettingController.h"
 #import "SettingsTableViewCell.h"
-#import "ViewController.h"


Model/OneTimePasswordSettings を経由してデータの共有をしているため、 ViewController への依存をなくしました。

ykws · 2021-11-14T19:21:48Z

    AlgorithmItem,
    DigitsItem,
-    PeriodItme,
+    PeriodItem,


Typo があったので修正しました。

ykws · 2021-11-14T19:25:22Z

 #pragma mark - Table view delegate

 - (void)tableView:(UITableView *)tableView didSelectRowAtIndexPath:(NSIndexPath *)indexPath {
+    // FIXME: タップしても反応しないことがある


設定画面の任意のセルをタップしても didSelectRowAtIndexPath が呼ばれなくて Picker の初期表示位置が意図した通りにできないことがあります。

#17 Model を追加とは別で、 TableView+Picker の課題なので、別途 Issue を作成して対応します。

#22 で対応します

ykws · 2021-11-14T19:26:51Z

 @interface ViewController : UIViewController
-@property (nonatomic) NSString *algorithm;
-@property (nonatomic) NSString *digits;
-@property (nonatomic) NSString *period;


Model/OneTimePasswordSettings を利用してデータを共有するようになったので、 property の公開は不要になりました。

フォーマットも修正 #18

ykws · 2021-11-14T19:27:33Z

 @property (weak, nonatomic) IBOutlet UILabel *oneTimePasswordLabel;
 @property (weak, nonatomic) IBOutlet UAProgressView *oneTimePasswordProgressView;
 @property (nonatomic, assign) CGFloat localProgress;
-@property (nonatomic) float scheduledTimerWithTimeInterval;


Model/OneTimePasswordSettings を利用するようになったので、 import/property を整理しました。

フォーマットも修正 #18

ykws · 2021-11-14T19:28:16Z

-        if([_period isEqualToString:@"60"]) {
-            self.scheduledTimerWithTimeInterval = 0.06f;
-        }
-    }


ワンタイムパスワード生成に関する処理は Model/OneTimePasswordSettings に移動しました。

#16 も合わせて解消

ykws · 2021-11-14T19:29:50Z

+    [self.oneTimePasswordLabel setText:password];
+
+    NSTimeInterval timeInterval = self.settings.period / 1000;
+    [NSTimer scheduledTimerWithTimeInterval:timeInterval target:self selector:@selector(updateOneTimePasswordView:) userInfo:nil repeats:YES];


ワンタイムパスワードに関する処理が View からなくなったので、 View で扱う初期化処理は initOneTimePasswordView に関することとして一つにまとめました。

ykws · 2021-11-14T19:33:17Z

@NMai-source レビューをお願いできますでしょうか？
コードの書き方で疑問や質問などをコメントして欲しいです。
疑問や質問が解消できたら Approve してもらえればと思います。

ykws · 2021-11-14T19:41:02Z

+        sharedInstance.issuer = issuer;
+        sharedInstance.algorithm = [OTPToken defaultAlgorithm];
+        sharedInstance.digits = [OTPToken defaultDigits];
+        sharedInstance.period = [OTPToken defaultPeriod];


デフォルト値で初期化するようにしました #19

ykws · 2021-11-14T19:42:02Z


    _localProgress = newProgress;
    [self.oneTimePasswordProgressView setProgress:_localProgress animated:NO];
-


フォーマット修正 #18

ykws · 2021-11-15T00:44:50Z

+// 設定の文字列から保存できるようにする
+- (void)saveAlgorithmString:(NSString *)algorithmString;
+- (void)saveDigitsString:(NSString *)digitsString;
+- (void)savePeriodString:(NSString *)periodString;


save より store の方がしっくりくる

Suggested change

- (void)savePeriodString:(NSString *)periodString;

- (void)storePeriodString:(NSString *)periodString;

ykws · 2021-11-19T00:52:25Z

+ (instancetype)sharedInstance {
+    static OneTimePasswordSettings *sharedInstance = nil;
+    static dispatch_once_t onceToken;
+    dispatch_once(&onceToken, ^{


@NMai-source dispatch_once を利用しているのはこの部分です。

NMai-source

@ykws
お疲れ様です。
遅くなりましたが、一通り修正内容の確認が完了しました。

ykws · 2021-11-22T08:30:48Z

+    return token.password;
+}
+
+- (NSString *)algorithmString {


以下のコンバートの責務は Converter に切り離して、ここではそれを呼ぶだけにしても良さそう

algorithmString: OTPAlgorithm -> NSString

digitsString: NSUInteger -> NSString

periodString: NSTimeInterval -> NSString

saveAlgorithmString: NSString -> OTPAlgorithm

saveDigitsString: NSString -> NSUInteger

savePeriodString: NSString -> NSTimeInterval

#21 にて対応予定

ykws added 4 commits November 15, 2021 04:09

OneTimePassword を扱うための Model を追加

1005826

ViewController から Model/OneTimePasswordSettings を利用する

47bcecf

Fix typo

58ec868

SettingController から Model/OneTimePasswordSettings を利用する

691a7f0

ykws self-assigned this Nov 14, 2021

ykws linked an issue Nov 14, 2021 that may be closed by this pull request

Model を追加する #17

Closed

ykws requested a review from NMai-source November 14, 2021 19:17

ykws commented Nov 14, 2021

View reviewed changes

ykws linked an issue Nov 14, 2021 that may be closed by this pull request

設定を保存する #15

Closed

ykws mentioned this pull request Nov 14, 2021

設定を保存する #15

Closed

ykws linked an issue Nov 14, 2021 that may be closed by this pull request

property のアクセスに self を利用する #16

Closed

ykws mentioned this pull request Nov 14, 2021

property のアクセスに self を利用する #16

Closed

ykws linked an issue Nov 14, 2021 that may be closed by this pull request

フォーマット修正 #18

Closed

2 tasks

ykws mentioned this pull request Nov 14, 2021

フォーマット修正 #18

Closed

2 tasks

ykws linked an issue Nov 14, 2021 that may be closed by this pull request

初期値を設定する #19

Closed

ykws mentioned this pull request Nov 14, 2021

初期値を設定する #19

Closed

ykws commented Nov 14, 2021

View reviewed changes

ykws commented Nov 15, 2021

View reviewed changes

ykws commented Nov 19, 2021

View reviewed changes

NMai-source approved these changes Nov 22, 2021

View reviewed changes

ykws commented Nov 22, 2021

View reviewed changes

This was referenced Nov 22, 2021

OneTimePassword に関する変換処理を Converter に委譲する #21

Closed

設定画面 Picker の初期表示位置が違うことがある #22

Open

ykws merged commit df043be into main Nov 22, 2021

ykws deleted the feature/model branch November 22, 2021 08:44


		@implementation OneTimePasswordSettings

		+ (instancetype)sharedInstance {


		_localProgress = newProgress;
		[self.oneTimePasswordProgressView setProgress:_localProgress animated:NO];

	- (void)savePeriodString:(NSString *)periodString;
	- (void)storePeriodString:(NSString *)periodString;

Conversation

ykws commented Nov 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

対応内容

未対応事項

スクリーンショット

Uh oh!

ykws left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ykws Nov 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ykws Nov 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ykws Nov 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ykws commented Nov 14, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NMai-source left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ykws commented Nov 14, 2021 •

edited

Loading

ykws Nov 14, 2021 •

edited

Loading

ykws Nov 14, 2021 •

edited

Loading

ykws Nov 14, 2021 •

edited

Loading