Droidcon UK: Poor programming patterns and how to avoid them.

Every engineer has great intentions when diving into a codebase. You want to make the code beautiful, easy to read, and modular. However as time passes and quick bugs fixes are merged, somehow your android codebase turned into a mess and it takes forever to figure out exactly where a certain component exists.

Alice will share with you common poor patterns within android that many developers make and how to avoid or refactor them. All these mistakes previously existed within the Pinterest codebase!

See video: https://skillsmatter.com/skillscasts/10795-common-poor-coding-patterns-and-how-to-avoid-them

Transcript

1. Alice Yuan Android Engineer at Pinterest me@aliceyuan.ca @Names_Alice Poor Programming Patterns and How to Avoid Them
2. Pinterest Lite Demo App github.com/AliceYuan/DroidConDemo
3. github.com/AliceYuan/DroidConDemo
4. github.com/AliceYuan/DroidConDemo
5. Problem #1
6. #1. Adding new features difficult Simple UI - but a pain to add features previous design MyProfileFragment PinnerFragment
7. Simple UI - but a pain to add features PinnerFragment MyProfileFragment BaseProfileFragment - abstract class Override loadUser() Override updateView(User) Override loadUser() Override updateView(PDKUser) updateView(PDKUser) #1. Inheritance Nesting
8. abstract class BaseProfileFragment extends Fragment { @Nullable protected RoundedImageView _profileIv; @Nullable protected TextView _nameTv; @Nullable protected TextView _bioTv; @Nullable protected TextView _followersTv; @Nullable protected LinearLayout _layout; ... protected void updateView(@NonNull final PDKUser user) { _nameTv.setText(user.getFirstName() + " " + user.getLastName()); _bioTv.setText(user.getBio()); _followersTv.setText(getResources().getString(R.string.following, user.getFollowingCount()); What’s wrong? Let’s inspect the code BaseProfileFragment - base screen with profile user’s picture, name and desc protected members Base class has logic that may change #1. Inheritance Nesting
9. MyProfileFragment BaseProfileFragment - abstract class PinnerFragment PinnerFragmentA PinnerFragmentB MyProfileFragmentA MyProfileFragmentB #1. Inheritance Nesting What if we multiplied the fragments?
10. MyProfileFragment BaseProfileFragment - abstract class PinnerFragment PinnerFragmentA PinnerFragmentB MyProfileFragmentA MyProfileFragmentB #1. Inheritance Nesting What if we multiplied the fragments? Inheritance Hell
11. What is the relationship of the common UI? MyProfileFragment is an avatar view PinnerFragment is an avatar view OR MyProfileFragment has an avatar view ProfileFragment has an avatar view Is a? vs. Has a? Inheritance vs Composition #1. Inheritance Nesting
12. Previous architecture PinnerFragment MyProfileFragment BaseProfileFragment - abstract class Override loadUser() Override updateView(User) Override loadUser() Override updateView(User) updateView(User) #1. Inheritance Nesting
13. New architecture PinnerFragment MyProfileFragment #1. Inheritance Nesting AvatarView updateView(...)
14. Composition using AvatarView class AvatarView extends LinearLayout { @Nullable private RoundedImageView _profileIv; @Nullable private TextView _nameTv; @Nullable private TextView _bioTv; @Nullable private TextView _followersTv; public void updateView(String name, String imageUrl, String bioText) { _nameTv.setText(name); _bioTv.setText(bioText); Glide.with(getContext()) .load(imageUrl) .into(_profileIv); } } #1. Inheritance Nesting AvatarView private members Pass through custom params
15. Inheritance is intentional - always declare your class final initially Use inheritance when the is relationship makes sense - Eg. a vehicle has tires, a truck is a type of vehicle - Eg. the android Fragment: when we create a custom Fragment, the custom Fragment is an android fragment Be deliberate with inheritance - think composition first #1. Inheritance Nesting public final class MyProfileFragment
16. Problem #2
17. #2. Eventbus causing bugs Follow button is causing bugs MyProfileFragment PinnerFragment
18. Libraries: Eventbus, Otto, Tinybus #2. Eventbus causing bugs At the implementation level, it is a global event queue.
19. #2. Eventbus causing bugs Libraries: Eventbus, Otto, Tinybus At the implementation level, it is a global event queue.
20. Notifying changes - why is it breaking? PinnerFragment #2. Eventbus causing bugs EventBus.getDefault() .post(new FollowEvent( newFollowingCount )); Publisher
21. Notifying changes - why is it breaking? MyProfileFragment PinnerFragment EventBus.getDefault() .post(new FollowEvent( newFollowingCount )); onMessageEvent( FollowEvent event) { setFollowingCount( event.getNumFollowing(); } Subscriber Publisher #2. Eventbus causing bugs
22. Notifying changes - why is it breaking? MyProfileFragment PinnerFragment onMessageEvent( (FollowEvent) post(new FollowEvent(...)) post(new FollowEvent(...)) #2. Eventbus causing bugs EventBus.getDefault() .post(new FollowEvent( newFollowingCount )); onMessageEvent( FollowEvent event) { setFollowingCount( event.getNumFollowing(); } Subscriber Publisher
23. - There’s no enforced responsibility of ensuring something is listening - As we add more events it decreases reliability and maintainability of the code - A pain to write tests for - Use eventbus only when we do not care if the event is consumed or not eg. Logging Because it’s decoupled, Eventbus libraries have many pitfalls #2. Eventbus causing bugs
24. Solution: Observer/ Listener Pattern - Simple interface to enforce tight coupling with an observer and subscriber pattern #2. Eventbus causing bugs public interface FollowListener { void onFollowCountChanged(int count); } FollowListener.java
25. Solution: Observer/ Listener Pattern public class MyProfileFragment implements FollowListener { @Override public void onFollowCountChanged(int count) { setFollowingCount(count); } } Subscriber #2. Eventbus causing bugs
26. Solution: Observer/ Listener Pattern public class PinnerFragment { private FollowListener _followListener; public void registerListener(FollowListener followListener) { _followListener = followListener; } Publisher #2. Eventbus causing bugs new UserCountApiCallback() { @Override public void onSuccess(int count) { if (_followListener != null) { _followListener.onFollowCountChanged(count); } }
27. Event Driven patterns #2. Eventbus causing bugs Coupling Api Definition Eventbus libraries Loose Free - updating does not require api change Freely defined APIs allow us to have better code maintenance
28. Event Driven patterns #2. Eventbus causing bugs Coupling Api Definition Eventbus libraries Loose Free - updating does not require api change Observables using custom interface Tight Structured definition Freely defined APIs allow us to have better code maintenance
29. Event Driven patterns #2. Eventbus causing bugs Coupling Api Definition Eventbus libraries Loose Free - updating does not require api change Observables using custom interface Tight Structured definition RxJava library Observable streams Tight Free - updating does not require api change Freely defined APIs allow us to have better code maintenance
30. - Use event bus for places where loose coupling makes sense - When app does not depend on subscriber to consume events - Eg. Logging - Use an Observable/ Listener pattern otherwise Eventbus is often abused due to it’s simplicity #2. Eventbus causing bugs
31. Problem #3
32. Why do I even need to send events? #3. Why send events MyProfileFragment PinnerFragment
33. Caching models on every screen #3. Poor data consistency MyProfileFragment PinnerFragment PDKUser _myUser; int _followingCount PDKUser _curUser;
35. Poor data consistency public class MyProfileFragment extends MVPFragment implements MyProfileView { //cache values to avoid having to make network calls in the future private PDKUser _myUser; private int _followingCount; private void loadUser() { if (_myUser == null) { loadMyUserAPI(); } else { _avatarView.updateView(_myUser.getFirstName() + " " + _myUser.getLastName(), MyUserUtils.get().getLargeImageUrl(_myUser), _myUser.getBio()); updateFollowingCount(_followingCount); } } Caching models on every screen Null check to decide network vs cache #3. Poor data consistency
36. Solution: have a central area to handle all storing and retrieval of models Repository Fragment Models Repository Memory Cache Disk Cache Networking #3. Poor data consistency
37. Solution: User Repository #3. Poor data consistency interface RepositoryListener<M extends Object> { void onSuccess(M model); void onError(Exception e); } Observer pattern
38. #3. Poor data consistency public class UserRepository { private PDKUser _myUser; //... public void loadMyUser(@NonNull final RepositoryListener<PDKUser> listener) { if (_myUser != null) { listener.onSuccess(_myUser); return; } PDKClient.getInstance().getMe(USER_FIELDS, new PDKCallback() { @Override public void onSuccess(PDKResponse response) { _myUser = response.getUser(); listener.onSuccess(_myUser); } //... }); } Check cache first Then check network call Solution: User Repository
39. #3. Poor data consistency public class MyProfileFragment extends Fragment { private AvatarView _avatarView; //... private void loadMyUser() { UserRepository.get().loadMyUser(new RepositoryListener<PDKUser>() { @Override public void onSuccess(PDKUser user) { _avatarView.updateView(user.getFirstName() + " " + user.getLastName(), user.getImageUrl(), user.getBio()); } }); } Solution: User Repository Then check network
40. Build a central way to fetch and retrieve models #3. Poor data consistency - Stop storing instances of models in your fragments! - Ensure data consistency regardless of where we’re retrieving or storing our models - Store and fetch models in a central area
41. Final issue, Problem #4
42. Why is writing a unit test difficult? - We want to ensure that we are correctly setting the user profile display data - What makes writing this unit test so complex? #4. Writing unit tests is hard
43. What a typical fragment looks like Fragment UI Animation Networking Models Business Logic Caching Logging Android Services Unit test #4. Writing unit tests is hard
44. private void loadMyUser() { PDKClient.getInstance().getMe(USER_FIELDS, new PDKCallback() { @Override public void onSuccess(PDKResponse response) { ... PDKUser user = response.getUser(); _myAvatarView.setUser(user); } What’s wrong? Let’s inspect the code Mock out network callback Mock Android UI MyProfileFragment Mock translation of response to model #4. Writing unit tests is hard
45. @Test public void testLoadMyUserSuccess() throws Exception { verify(_myProfileView).updateAvatarView(FIRST_NAME + " " + LAST_NAME, IMAGE_URL, BIO); verify(_viewResources).getString(anyInt(), eq(_followingUsersList.size())); verify(_myProfileView).updateFollowingText(); } Let’s make this simpler, How should a unit test look like? #4. Writing unit tests is hard
46. Solution: Separate concerns through an interface - You’ve likely heard of the paradigms MVVM and MVP - Separate concerns between areas that do not need to know about each other. - communicate between classes without knowing the internals #4. Writing unit tests is hard
47. Separate concerns - MVP example Contract Presenter (Business Logic) Fragment View Interface Models (Repository) #4. Writing unit tests is hard
48. interface Presenter<V extends MVPView> { void attachView(@NonNull final V view); void detachView(); } interface MVPView { Presenter createPresenter(); Presenter getPresenter(); ViewResources getViewResources(); } Build an initial Framework #4. Writing unit tests is hard
49. public abstract class MVPFragment extends Fragment implements MVPView { Presenter _presenter; @Override public void onCreate(@Nullable Bundle savedInstanceState) { super.onCreate(savedInstanceState); _presenter = createPresenter(); } @Override public void onViewCreated(View view, @Nullable Bundle savedInstanceState) { _presenter.attachView(this); super.onViewCreated(view, savedInstanceState); } @Override abstract public Presenter createPresenter(); } * Build an Initial Framework Presenter is bound Presenter is created #4. Writing unit tests is hard
50. interface MyProfileView extends MVPView { void updateFollowingCount(int count); void updateAvatarView(String name, String imageUrl, String bioText); } Define a Contract for View #4. Writing unit tests is hard
51. Implement the interface public class MyProfileFragment extends MVPFragment implements MyProfileView { @Override public void updateAvatarView(String name, String imageUrl, String bioText) { _avatarView.updateView(name, imageUrl, bioText); } @Override public void updateFollowingCount(int count) { _avatarView.updateFollowingText(getResources().getString(R.string.my_user_following, count)); } } Implement defined contract #4. Writing unit tests is hard
52. public interface UserDataSource { void loadMyUser(@NonNull final RepositoryListener<PDKUser> listener); void loadMyUserNumFollowing(@NonNull final RepositoryListener<Integer> listener); } Define a Contract for Repository #4. Writing unit tests is hard
53. public interface UserDataSource { void loadMyUser(@NonNull final RepositoryListener<PDKUser> listener); void loadMyUserNumFollowing(@NonNull final RepositoryListener<Integer> listener); } Implement defined interface #4. Writing unit tests is hard public class UserRepository implements UserDataSource {
54. Use interface with the presenter #4. Writing unit tests is hard public class MyProfilePresenter implements Presenter<MyProfileView> { public MyProfilePresenter(@NonNull UserDataSource dataSource) { _dataSource = dataSource; } @Override public void attachView(@NonNull final MyProfileView view) { _view = view; loadUser(view); } void loadUser(final MyProfileView view) { _dataSource.loadMyUser(new RepositoryListener<PDKUser>() { @Override public void onSuccess(PDKUser user) { view.updateAvatarView(user.getFirstName() + " " + user.getLastName(), user.getImageUrl(), user.getBio()); } } }); No longer Android UI No longer network callback No longer referencing Repository
55. @Test public void testLoadMyUserSuccess() throws Exception { verify(_myProfileView).updateAvatarView(FIRST_NAME + " " + LAST_NAME, IMAGE_URL, BIO); verify(_viewResources).getString(anyInt(), eq(_followingUsersList.size())); verify(_myProfileView).updateFollowingText(null); // null because mock returns null } What does the unit test look like now? #4. Writing unit tests is hard
56. public static abstract class BaseMyProfileTest { @Mock MyProfileView _myProfileView; UserDataSource _mockDataSource; @Before public void setUp() throws Exception { _mockDataSource = getUserDataSource(); MyProfilePresenter myProfilePresenter = new MyProfilePresenter(_viewResources, _mockDataSource); myProfilePresenter.attachView(_myProfileView); } abstract UserDataSource getUserDataSource(); } *Some initial testing infra required #4. Writing unit tests is hard Mock MyProfileView interface
57. public static class TestUpdateMyProfileSuccess extends BaseMyProfileTest { @Override UserDataSource getUserDataSource() { return new MockUserDataSourceSuccess(_pdkUser, _followingUsersList); } } *Some initial testing infra required #4. Writing unit tests is hard Mock UserDataSource
58. Unit tests are easy to write when you separate the business logic - Choose a paradigm to follow which separates concerns - Use interfaces to abstract internals away - Improved testability and also understandability of the code - Can be used for many utilities #4. Writing unit tests is hard
59. We made it, that’s all!
60. 1. Be deliberate about inheritance, think about composition 2. Eventbus is a loosely coupled library - use tight coupling patterns such as RxJava or Observable callbacks instead 3. Create a central location to store and retrieve models to ensure data consistency 4. Separate the areas of concern to increase testability and maintenance Recap
61. The solution to avoiding poor patterns is awareness
62. Thanks! You can find me at: me@aliceyuan.ca @Names_Alice Any questions?