atsuki-uenishi / movice

1 stars 1 forks source link

Update Podfile #1

Closed atsuki-uenishi closed 2 years ago

inushima commented 2 years ago

https://github.com/atsuki-uenishi/movice/blob/main/movice/Controller/GetImageByUrlController.swift

Controllerフォルダには基本ViewControllerを格納します。 GetImageByUrlControllerはViewControllerではないので、ユーティリティクラスを格納する別のフォルダ(Libなど)を作成して格納する方がいいですね。

クラス名も修正した方がいいです。 例

class ImageUtil
inushima commented 2 years ago

https://github.com/atsuki-uenishi/movice/blob/main/movice/Controller/GetImageByUrlController.swift#L29

UIImageに対する操作なので、UIImageに対するExtensionとして実装できないか、検討してみてください。

inushima commented 2 years ago

https://github.com/atsuki-uenishi/movice/blob/main/movice/Controller/DetailViewController.swift#L55

viewDidLoadのようなライフサイクルメソッドは、iOS側から呼び出されることを前提としています。 プログラムから呼び出しは、意図しない動作を引き起こすことがあり、NGです。

追加・更新のタイミングで、画面を再描画することを意図してると思いますが、一旦再描画に必要な処理のみメソッドに切り出し、そのメソッドを呼び出すようにしてみてください。

(rxSwiftが使えるようになると、イベントに応じて画面の必要な箇所だけを再描画するといった処理が書きやすくなります。)

inushima commented 2 years ago

https://github.com/atsuki-uenishi/movice/blob/main/movice/Controller/DetailViewController.swift#L36

ifを使わず、コンパクトに書けそうです。

            alreadyAdd = !watchlistMovie?.isEmpty
inushima commented 2 years ago

https://github.com/atsuki-uenishi/movice/blob/main/movice/Controller/WatchListViewController.swift#L63

処理の事前条件は、なるべくguardにまとめておくことが多いです。

 guard let cell = tableView.dequeueReusableCell(withIdentifier: "watchlistCell", for: indexPath), let safeWatchlist = watchlist else { return UITableViewCell() }
inushima commented 2 years ago

以下に移動 https://github.com/atsuki-uenishi/movice/pull/7

inushima commented 2 years ago

@atsuki-uenishi こちらはクローズお願いします。