Skip to content
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

Refactored ImageGalleryPanGestureDelegate #132

Merged
merged 1 commit into from
Apr 15, 2016
Merged

Conversation

aashishdhawan
Copy link
Contributor

Refactors code a little bit. The original implementation had 2 issues.

  1. ImageGalleryPanGestureDelegate had following functions
- func presentViewController(controller: UIAlertController)
-  func dismissViewController(controller: UIAlertController)
-  func permissionGranted()
-  func hideViews()

These should not fall under ImageGalleryPanGestureDelegate.

  1. It is not responsibility of ImageGalleryView to call PHPhotoLibrary.requestAuthorization...violates MVC in my point of view.

@RamonGilabert
Copy link
Contributor

Hey @aashishdhawan. We had this sort of discussion yesterday, and I think it's nice if you can give your 5 cents. There are many ways where you can see that the pattern of MVC is sort of broken, in Hyper we are tending towards and MVVC model now, but the problem is still in the VC part in my opinion, we are creating massive controllers with huge views that have no logic, which is nice, until you see huge controllers.

With that being said I see 4 options that I presented yesterday to the team.

  1. Having all the views in the controller.
  2. Having one view that has all the views but has some logic and protocols.
  3. Having one view but you handle it in the controller like view.textField.
  4. A combination of some of the cases above.

When I talk about some logic I talk about logic in the view, meaning, the view controls it's subviews, animations, gestures, etc. And it only notifies the controller when there's something he should do, meaning, doing some request, calling a bigger delegate, controller another controller, present something, etc. So we have basically a view with all the view related logic and a controller with all the controller logic. This would be point 2, which is the one that I favor the most.

What do you think about this?

I'll include @hyperoslo/malibu here.

@aashishdhawan
Copy link
Contributor Author

Well....in my point of view the idea is to separate responsibilities and achieve decoupling which is also the gist of MVC and MVVC.

If i look at your suggestions my position is close to point 4 which is A combination of some of the cases above.

Now coming to point 2 which is Having one view that has all the views but has some logic and protocols.. I would prefer to avoid having one view which contains all views as subviews but at the same time i want to keep views in viewController to be minimum so that ViewController does not become fat.

I usually apply a grouping technique. For Example in this project we can wrap BottomContainerView and ImageGalleryView in one parent view like BottomImageGalleryView.
Also move PanGesture functions from ImagePickerController into this view. We can also move functions collapseGalleryView expandGalleryView into this one.

class BottomImageGalleryView {
  let bottomContainerView: BottomContainerView?
  let imageGalleryView: ImageGalleryView?

//apply pan gesture functionality here  
  func panGestureDidChange(translation: CGPoint) {
    //code 
  }

  func panGestureDidStart() {
    //code
  }

func collapseGalleryView() {

}

func expandGalleryView() {

}
.......so on
}

In a similar fashion we can refactor CameraView and moving related functionality into it

Now with this approach we do not have one main single view in ViewController but we have significantly reduced number of views and the code which should not be there in ViewController.

And here i spend my 5 cents. I hope i am not being stupid.

@RamonGilabert
Copy link
Contributor

When I talk about having a single view, I'm talking about a view with subviews too, though, that means, having a view called ImagePickerView, which has concerns of: BottomView, CameraView and TopView. That ImagePickerView is responsible for all the logic that the subviews have, so animations and stuff like that, whereas the controller controls those views. That's what I meant. (I don't remember the code that much in ImagePicker though, tbh).

@aashishdhawan
Copy link
Contributor Author

Gotcha. @RamonGilabert

@aashishdhawan
Copy link
Contributor Author

Merging this one for now. Will come back to above suggestion when time.

@aashishdhawan aashishdhawan merged commit 1f19b78 into master Apr 15, 2016
@aashishdhawan aashishdhawan deleted the refactor/better branch April 15, 2016 16:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants