New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Peek pop 対応 #4

Merged
merged 10 commits into from Dec 7, 2017

Conversation

Projects
None yet
2 participants
@hhyyg
Owner

hhyyg commented Dec 6, 2017

  • Peekで、詳細画面のプレビューを表示するように。そのままPopで詳細画面に遷移。
  • puick quick action で、そのアイテムを削除できるように
  • 他リファクタ

@hhyyg hhyyg requested a review from mono0926 Dec 6, 2017

fatalError("Unable to instantiate meal2")
}
return [meal1, meal2, meal3]

This comment has been minimized.

@mono0926

mono0926 Dec 6, 2017

Collaborator

Meal nameの[String]あたりを元に、mapとか使ってきれいに書けそう。

fatalError("Unable to instantiate meal2")
}
guard let meal3 = Meal(name: "Pasta with Meatballs", photo: photo3, rating: 3) else {
fatalError("Unable to instantiate meal2")

This comment has been minimized.

@mono0926

mono0926 Dec 6, 2017

Collaborator

Meal nameの[String]あたりを元に、mapとか使ってきれいに書けそう。

meal2ではなく3だけど、こういうミスも防げるはず。

if self.traitCollection.forceTouchCapability == UIForceTouchCapability.available {
registerForPreviewing(with: self, sourceView: view)
}
}
override func didReceiveMemoryWarning() {
super.didReceiveMemoryWarning()
// Dispose of any resources that can be recreated.
}

This comment has been minimized.

@mono0926

mono0926 Dec 6, 2017

Collaborator

不使用の雛形メソッドは基本的に消す。

}
meals += [meal1, meal2, meal3]
return DataContainer.meals.count

This comment has been minimized.

@mono0926

mono0926 Dec 6, 2017

Collaborator

各所で可変のDataContainer.mealsを参照しているとタイミングで不整合起こりそうで不安。
reload前にプロパティにセットしてそれを参照する方が堅牢になりそう。

This comment has been minimized.

@mono0926

mono0926 Dec 6, 2017

Collaborator

と思ったけど、今の全体の作り的にはこれでいいかなあ 🤔

This comment has been minimized.

@hhyyg

hhyyg Dec 7, 2017

Owner

今回のサンプルでは見逃したい

extension MealTableViewController: MealViewControllerDelegate {
func mealViewController(_ viewController: MealViewController, mealDeleteDidTap meal: Meal) {
let i = DataContainer.meals.index { $0.id == meal.id }!

This comment has been minimized.

@mono0926

mono0926 Dec 6, 2017

Collaborator

容易にミスってクラッシュしそうだから、!使わずにguardしてassertしつつreturnくらいが妥当かな(開発時はassert引っかかって、リリース時はクラッシュせず動かないけどデータの整合性が直ったら動くようになる感じ)

This comment has been minimized.

@mono0926

mono0926 Dec 6, 2017

Collaborator

動的なデータとか絡む場合は、基本的に!系の操作しない方が良い。
初期化処理とかの開発時に絶対気づけそうなところは!で良いけど。

This comment has been minimized.

@hhyyg

hhyyg Dec 7, 2017

Owner

なるほどです

override var previewActionItems: [UIPreviewActionItem] {
let deleteAction = UIPreviewAction(title: "Delete", style: .destructive) { (action, previewViewController ) in
self.delegate!.mealViewController(self, mealDeleteDidTap: self.meal!)

This comment has been minimized.

@mono0926

mono0926 Dec 6, 2017

Collaborator

この!も、リリース時にこのパスで落としたいという明確な意思がない限り避けたい。

This comment has been minimized.

@hhyyg

hhyyg Dec 7, 2017

Owner

直しました。!を多用してしまう気がする

(name: "Pasta with Meatballs", photoName: "meal3", rating: 3)
]
let sampleMeals: [Meal] = elements.map { (name, photoName, rating) in

This comment has been minimized.

@mono0926

mono0926 Dec 7, 2017

Collaborator

enumerated()挟むと、mapの引数にindexが渡るから、"photoName(i+1)"みたいに画像文字列組み立てられる。

@@ -151,7 +150,10 @@ extension MealTableViewController: UIViewControllerPreviewingDelegate {
extension MealTableViewController: MealViewControllerDelegate {
func mealViewController(_ viewController: MealViewController, mealDeleteDidTap meal: Meal) {
let i = DataContainer.meals.index { $0.id == meal.id }!
guard let i = (DataContainer.meals.index { $0.id == meal.id }) else {
logger.debug("no found meal:\(meal.id) \(meal.name)")

This comment has been minimized.

@mono0926

mono0926 Dec 7, 2017

Collaborator

こういう予期しない異常系はerror系のログ吐くのが普通。

This comment has been minimized.

@mono0926

mono0926 Dec 7, 2017

Collaborator

あるいは、エラー内容書くのが面倒だったり自明という理由でassertionFailure()で済ませることもよくある。
何れにしても、debugログだとミスした時に気づくのが遅れるから、握りつぶしに近い。

This comment has been minimized.

@mono0926

mono0926 Dec 7, 2017

Collaborator

ここはプログラムミスしなければ到達しないはずなので、assertionFailureかlogger.fatal、あるいはその両方が適切に思う。

そこそこ頻度があるエラー(APIから返るエラーレスポンスとか?)は、infoかerrorレベルのログにしてる。

@mono0926

This comment has been minimized.

Collaborator

mono0926 commented Dec 7, 2017

面倒くさそうなコンフリクト発生している( ´・‿・`)

@mono0926

mono0926 requested changes Dec 7, 2017 edited

[このコメント無視して]

@mono0926

あれ、ここに書いたレビューが一覧に出てこないような。

@mono0926

This comment has been minimized.

Collaborator

mono0926 commented Dec 7, 2017

61fe16c#r155422018 のレビューがここの一覧に出てこない 🤔

@mono0926

This comment has been minimized.

Collaborator

mono0926 commented Dec 7, 2017

そういえばGitHubのコメントの挙動がおかしかったのが直ったかも。

hhyyg added some commits Dec 7, 2017

Merge branch 'master' into peek-pop
# Conflicts:
#	Miso.TrackingMealApp/TrackingMealApp.xcodeproj/project.pbxproj
#	Miso.TrackingMealApp/TrackingMealApp/MealViewController.swift

@hhyyg hhyyg merged commit 6f76976 into master Dec 7, 2017

@mono0926 mono0926 deleted the peek-pop branch Dec 7, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment