Alice Yuan Android Engineer at Pinterest [email protected] @Names_Alice Poor Programming Patterns and How to Avoid Them 

Pinterest Lite Demo App github.com/AliceYuan/DroidConDemo 

github.com/AliceYuan/DroidConDemo 

github.com/AliceYuan/DroidConDemo 

Problem #1 

#1. Adding new features difficult Simple UI - but a pain to add features previous design MyProfileFragment PinnerFragment 

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 

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 

MyProfileFragment BaseProfileFragment - abstract class PinnerFragment PinnerFragmentA PinnerFragmentB MyProfileFragmentA MyProfileFragmentB #1. Inheritance Nesting What if we multiplied the fragments? 

MyProfileFragment BaseProfileFragment - abstract class PinnerFragment PinnerFragmentA PinnerFragmentB MyProfileFragmentA MyProfileFragmentB #1. Inheritance Nesting What if we multiplied the fragments? Inheritance Hell 

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 

Previous architecture PinnerFragment MyProfileFragment BaseProfileFragment - abstract class Override loadUser() Override updateView(User) Override loadUser() Override updateView(User) updateView(User) #1. Inheritance Nesting 

New architecture PinnerFragment MyProfileFragment #1. Inheritance Nesting AvatarView updateView(...) 

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 

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 

Problem #2 

#2. Eventbus causing bugs Follow button is causing bugs MyProfileFragment PinnerFragment 

Libraries: Eventbus, Otto, Tinybus #2. Eventbus causing bugs At the implementation level, it is a global event queue. 

#2. Eventbus causing bugs Libraries: Eventbus, Otto, Tinybus At the implementation level, it is a global event queue. 

Notifying changes - why is it breaking? PinnerFragment #2. Eventbus causing bugs EventBus.getDefault() .post(new FollowEvent( newFollowingCount )); Publisher 

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 

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 

- 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 

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 

Solution: Observer/ Listener Pattern public class MyProfileFragment implements FollowListener { @Override public void onFollowCountChanged(int count) { setFollowingCount(count); } } Subscriber #2. Eventbus causing bugs 

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); } } 

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 

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 

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 

- 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 

Problem #3 

Why do I even need to send events? #3. Why send events MyProfileFragment PinnerFragment 

Caching models on every screen #3. Poor data consistency MyProfileFragment PinnerFragment PDKUser _myUser; int _followingCount PDKUser _curUser; 

No content

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 

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 

Solution: User Repository #3. Poor data consistency interface RepositoryListener { void onSuccess(M model); void onError(Exception e); } Observer pattern 

#3. Poor data consistency public class UserRepository { private PDKUser _myUser; //... public void loadMyUser(@NonNull final RepositoryListener 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 

#3. Poor data consistency public class MyProfileFragment extends Fragment { private AvatarView _avatarView; //... private void loadMyUser() { UserRepository.get().loadMyUser(new RepositoryListener() { @Override public void onSuccess(PDKUser user) { _avatarView.updateView(user.getFirstName() + " " + user.getLastName(), user.getImageUrl(), user.getBio()); } }); } Solution: User Repository Then check network 

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 

Final issue, Problem #4 

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 

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 

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 

@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 

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 

Separate concerns - MVP example Contract Presenter (Business Logic) Fragment View Interface Models (Repository) #4. Writing unit tests is hard 

interface Presenter { 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 

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 

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 

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 

public interface UserDataSource { void loadMyUser(@NonNull final RepositoryListener listener); void loadMyUserNumFollowing(@NonNull final RepositoryListener listener); } Define a Contract for Repository #4. Writing unit tests is hard 

public interface UserDataSource { void loadMyUser(@NonNull final RepositoryListener listener); void loadMyUserNumFollowing(@NonNull final RepositoryListener listener); } Implement defined interface #4. Writing unit tests is hard public class UserRepository implements UserDataSource { 

Use interface with the presenter #4. Writing unit tests is hard public class MyProfilePresenter implements Presenter { 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() { @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 

@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 

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 

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 

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 

We made it, that’s all! 

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 

The solution to avoiding poor patterns is awareness 

Thanks! You can find me at: [email protected] @Names_Alice Any questions?