Refactoring Simplenote

November 2, 2021

Simplenote is an Open Source note taking application published by Automattic. Source for the Android client is available on github.

AnalyticsTrackerNosara.java

Here’s the original source file

	package com.automattic.simplenote.analytics;

	import android.content.Context;
	import android.content.SharedPreferences;
	import android.text.TextUtils;

	import androidx.preference.PreferenceManager;

	import com.automattic.android.tracks.TracksClient;

	import org.json.JSONObject;

	import java.util.Map;
	import java.util.UUID;

	public class AnalyticsTrackerNosara implements AnalyticsTracker.Tracker {

	private static final String TRACKS_ANON_ID = "nosara_tracks_anon_id";
	private static final String EVENTS_PREFIX = "spandroid_";

	private String mUserName = null;
	private String mAnonID = null; // do not access this variable directly. Use methods.

	private TracksClient mNosaraClient;
	private Context mContext;

	public AnalyticsTrackerNosara(Context context) {
	if (null == context) {
	mNosaraClient = null;
	return;
	}

	mContext = context;
	mNosaraClient = TracksClient.getClient(context);
	}

	private void clearAnonID() {
	mAnonID = null;
	SharedPreferences preferences = PreferenceManager.getDefaultSharedPreferences(mContext);
	if (preferences.contains(TRACKS_ANON_ID)) {
	final SharedPreferences.Editor editor = preferences.edit();
	editor.remove(TRACKS_ANON_ID);
	editor.apply();
	}
	}

	private String getAnonID() {
	if (mAnonID == null) {
	SharedPreferences preferences = PreferenceManager.getDefaultSharedPreferences(mContext);
	mAnonID = preferences.getString(TRACKS_ANON_ID, null);
	}
	return mAnonID;
	}

	private String generateNewAnonID() {
	String uuid = UUID.randomUUID().toString();
	SharedPreferences preferences = PreferenceManager.getDefaultSharedPreferences(mContext);
	final SharedPreferences.Editor editor = preferences.edit();
	editor.putString(TRACKS_ANON_ID, uuid);
	editor.apply();

	mAnonID = uuid;
	return uuid;
	}

	@Override
	public void track(AnalyticsTracker.Stat stat, String category, String label) {
	track(stat, category, label, null);
	}

	@Override
	public void track(AnalyticsTracker.Stat stat, String category, String label, Map<String, ?> properties) {
	if (mNosaraClient == null) {
	return;
	}

	String eventName = stat.name().toLowerCase();

	final String user;
	final TracksClient.NosaraUserType userType;
	if (mUserName != null) {
	user = mUserName;
	userType = TracksClient.NosaraUserType.SIMPLENOTE;
	} else {
	// This is just a security checks since the anonID is already available here.
	// refresh metadata is called on login/logout/startup and it loads/generates the anonId when necessary.
	if (getAnonID() == null) {
	user = generateNewAnonID();
	} else {
	user = getAnonID();
	}
	userType = TracksClient.NosaraUserType.ANON;
	}

	if (properties != null) {
	JSONObject propertiesJson = new JSONObject(properties);
	mNosaraClient.track(EVENTS_PREFIX + eventName, propertiesJson, user, userType);
	} else {
	mNosaraClient.track(EVENTS_PREFIX + eventName, user, userType);
	}
	}

	@Override
	public void refreshMetadata(String username) {
	if (mNosaraClient == null) {
	return;
	}

	if (!TextUtils.isEmpty(username)) {
	mUserName = username;
	if (getAnonID() != null) {
	mNosaraClient.trackAliasUser(mUserName, getAnonID(), TracksClient.NosaraUserType.SIMPLENOTE);
	clearAnonID();
	}
	} else {
	mUserName = null;
	if (getAnonID() == null) {
	generateNewAnonID();
	}
	}
	}

	@Override
	public void flush() {
	if (mNosaraClient == null) {
	return;
	}

	mNosaraClient.flush();
	}
	}

view raw AnalyticsTrackerNosara.java hosted with ❤ by GitHub

I’ll approach this as if I plan to make some feature update; meaning, refactor first before starting the update.

A couple of things stand out:

A cluster of private XAnonID() methods
A longish track() method

I’ll focus on the track() method first.

	@Override
	public void track(AnalyticsTracker.Stat stat, String category, String label, Map<String, ?> properties) {
	if (mNosaraClient == null) {
	return;
	}

	String eventName = stat.name().toLowerCase();

	final String user;
	final TracksClient.NosaraUserType userType;
	if (mUserName != null) {
	user = mUserName;
	userType = TracksClient.NosaraUserType.SIMPLENOTE;
	} else {
	// This is just a security checks since the anonID is already available here.
	// refresh metadata is called on login/logout/startup and it loads/generates the anonId when necessary.
	if (getAnonID() == null) {
	user = generateNewAnonID();
	} else {
	user = getAnonID();
	}
	userType = TracksClient.NosaraUserType.ANON;
	}

	if (properties != null) {
	JSONObject propertiesJson = new JSONObject(properties);
	mNosaraClient.track(EVENTS_PREFIX + eventName, propertiesJson, user, userType);
	} else {
	mNosaraClient.track(EVENTS_PREFIX + eventName, user, userType);
	}
	}

view raw AnalyticsTrackerNosara.java hosted with ❤ by GitHub

There are three sections in this method, each separated by some whitespace.

In the top section, the initial null client check is fine. The client is initialized in the constructor. I’m not sure why that would fail, but I don’t want to change any behavior here, so it stays.

The bottom section calls the tracking client with an event name, some attributes and optional properties. I want to extract the if-else embedded there.

TrackableUser

That leaves the middle section and I’ll start there. This section assigns user and userType attributes for the analytics event. The nested if-else blurs the fact that it is just making those assignments. Making it more readable for the next developer is an important goal of refactoring, so I’ll do that.

There are three conditions bound up in that middle assignment section.

When a user has a username (from an instance variable)
When an unnamed user has an anonymous id
When a user is unnamed and does not have an anonymous id

I chose to replace the bulky if conditions with a representative interface that I named TrackableUser. The analytics code tracks users by name if it has one or anonymously by uuid if no name is present. It’s going to take a few steps before this function is more readable, but I only want to make small deltas that don’t change the existing behavior; starting with the user name.

	interface TrackableUser {
	Boolean isNamed();
	}

view raw AnalyticsTrackerNosara.java hosted with ❤ by GitHub

I implemented two classes of the interface that represent named and unnamed users and provided a factory function to create them.

	class NamedUser implements TrackableUser {
	@Override
	public Boolean isNamed() {
	return true;
	}
	}

	class UnknownUser implements TrackableUser {
	@Override
	public Boolean isNamed() {
	return false;
	}
	}

view raw AnalyticsTrackerNosara.java hosted with ❤ by GitHub

	private TrackableUser createTrackableUser(String userName) {
	if (userName != null) {
	return new NamedUser();
	}
	return new UnknownUser();
	}

view raw AnalyticsTrackerNosara.java hosted with ❤ by GitHub

Next, replace the explicit username condition check with the new interface.

	@Override
	public void track(AnalyticsTracker.Stat stat, String category, String label, Map<String, ?> properties) {
	if (mNosaraClient == null) {
	return;
	}

	String eventName = stat.name().toLowerCase();

	final String user;
	final TracksClient.NosaraUserType userType;
	final TrackableUser trackable = createTrackableUser(mUserName);

	if (trackable.isNamed()) {
	user = mUserName;
	userType = TracksClient.NosaraUserType.SIMPLENOTE;
	} else {
	// This is just a security checks since the anonID is already available here.
	// refresh metadata is called on login/logout/startup and it loads/generates the anonId when necessary.
	if (getAnonID() == null) {
	user = generateNewAnonID();
	} else {
	user = getAnonID();
	}
	userType = TracksClient.NosaraUserType.ANON;
	}

	if (properties != null) {
	JSONObject propertiesJson = new JSONObject(properties);
	mNosaraClient.track(EVENTS_PREFIX + eventName, propertiesJson, user, userType);
	} else {
	mNosaraClient.track(EVENTS_PREFIX + eventName, user, userType);
	}
	}

view raw AnalyticsTrackerNosara.java hosted with ❤ by GitHub

It doesn’t look like much yet, but we haven’t changed any behavior. The TrackableUser interface gives us a place to push code, eventually simplifying the function.

AnonymousId

To continue along this path, it looks like things will get jammed up with those XAnonID() functions very soon. So, I decide to take a quick diversion to the concept of anonymous ID. It’s a concept that wants to escape from the existing implementation. One way to tell is by noticing all of the XAnonID suffixes.

clearAnonID()
getAnonID()
generateNewAnonId()

	private void clearAnonID() {
	mAnonID = null;
	SharedPreferences preferences = PreferenceManager.getDefaultSharedPreferences(mContext);
	if (preferences.contains(TRACKS_ANON_ID)) {
	final SharedPreferences.Editor editor = preferences.edit();
	editor.remove(TRACKS_ANON_ID);
	editor.apply();
	}
	}

	private String getAnonID() {
	if (mAnonID == null) {
	SharedPreferences preferences = PreferenceManager.getDefaultSharedPreferences(mContext);
	mAnonID = preferences.getString(TRACKS_ANON_ID, null);
	}
	return mAnonID;
	}

	private String generateNewAnonID() {
	String uuid = UUID.randomUUID().toString();
	SharedPreferences preferences = PreferenceManager.getDefaultSharedPreferences(mContext);
	final SharedPreferences.Editor editor = preferences.edit();
	editor.putString(TRACKS_ANON_ID, uuid);
	editor.apply();

	mAnonID = uuid;
	return uuid;
	}

view raw AnalyticsTrackerNosara.java hosted with ❤ by GitHub

Luckily, these functions aren’t spread out too far. In a bigger file, those fluffs of functions can get disorganized quickly. Even though this is a pretty small implementation, let’s give AnonymousId the conceptual status that seems to be forming. It’s going to need to be extracted to make the track() function more readable.

Preferring composition to inheritance doesn’t mean that classes and objects are code smells. A class is a good organizing structure for similar functions.

	class AnonymousId {
	private String anonymousId = null;

	public String create(SharedPreferences preferences) {
	anonymousId = UUID.randomUUID().toString();
	final SharedPreferences.Editor editor = preferences.edit();
	editor.putString(AnalyticsTrackerNosara.TRACKS_ANON_ID, anonymousId);
	editor.apply();
	return anonymousId;
	}

	public String getId(SharedPreferences preferences) {
	if (anonymousId == null) {
	anonymousId = preferences.getString(AnalyticsTrackerNosara.TRACKS_ANON_ID, null);
	}
	return anonymousId;

	}
	public void clear(SharedPreferences preferences) {
	anonymousId = null;
	if (preferences.contains(AnalyticsTrackerNosara.TRACKS_ANON_ID)) {
	final SharedPreferences.Editor editor = preferences.edit();
	editor.remove(AnalyticsTrackerNosara.TRACKS_ANON_ID);
	editor.apply();
	}
	}
	}

view raw AnalyticsTrackerNosara.java hosted with ❤ by GitHub

Now back to our trackable user, there are actually three conditions to distinguish, so finish those out.

	interface TrackableUser {
	Boolean isNamed();
	Boolean isUnknown();
	}

	class NamedUser implements TrackableUser {
	@Override
	public Boolean isNamed() {
	return true;
	}

	@Override
	public Boolean isUnknown() {
	return false;
	}
	}

	class AnonymousUser implements TrackableUser {
	@Override
	public Boolean isNamed() {
	return false;
	}

	@Override
	public Boolean isUnknown() {
	return false;
	}
	}

	class UnknownUser implements TrackableUser {
	@Override
	public Boolean isNamed() {
	return false;
	}

	@Override
	public Boolean isUnknown() {
	return true;
	}
	}

view raw AnalyticsTrackerNosara.java hosted with ❤ by GitHub

The track function still looks quite bulky. But I’m not worried, the magic is about to happen. A few small refactor patterns are about to collapse these multiple if-elses.

	@Override
	public void track(AnalyticsTracker.Stat stat, String category, String label, Map<String, ?> properties) {
	if (mNosaraClient == null) {
	return;
	}

	String eventName = stat.name().toLowerCase();

	final String user;
	final TracksClient.NosaraUserType userType;
	final TrackableUser trackable = createTrackableUser(mUserName, anonymousId.getId(PreferenceManager.getDefaultSharedPreferences(mContext)));

	if (trackable.isNamed()) {
	user = mUserName;
	userType = TracksClient.NosaraUserType.SIMPLENOTE;
	} else {
	// This is just a security checks since the anonID is already available here.
	// refresh metadata is called on login/logout/startup and it loads/generates the anonId when necessary.
	if (trackable.isUnknown()) {
	user = anonymousId.create(PreferenceManager.getDefaultSharedPreferences(mContext));
	} else {
	user = anonymousId.getId(PreferenceManager.getDefaultSharedPreferences(mContext));
	}
	userType = TracksClient.NosaraUserType.ANON;
	}

	if (properties != null) {
	JSONObject propertiesJson = new JSONObject(properties);
	mNosaraClient.track(EVENTS_PREFIX + eventName, propertiesJson, user, userType);
	} else {
	mNosaraClient.track(EVENTS_PREFIX + eventName, user, userType);
	}
	}

view raw AnalyticsTrackerNosara.java hosted with ❤ by GitHub

Adding those new instances of SharedPreferences around added some unnecessary heft to the AnonymousId method calls, so let’s clean that up by moving the SharedPreferences in the constructor. This also removes the need for the mContext instance variable in the Tracker class.

	class AnonymousId {
	private String anonymousId = null;
	private final SharedPreferences preferences;

	AnonymousId(SharedPreferences preferences) {
	this.preferences = preferences;
	}

	public String create() {
	anonymousId = UUID.randomUUID().toString();
	final SharedPreferences.Editor editor = preferences.edit();
	editor.putString(AnalyticsTrackerNosara.TRACKS_ANON_ID, anonymousId);
	editor.apply();
	return anonymousId;
	}

	public String getId() {
	if (anonymousId == null) {
	anonymousId = preferences.getString(AnalyticsTrackerNosara.TRACKS_ANON_ID, null);
	}
	return anonymousId;

	}
	public void clear() {
	anonymousId = null;
	if (preferences.contains(AnalyticsTrackerNosara.TRACKS_ANON_ID)) {
	final SharedPreferences.Editor editor = preferences.edit();
	editor.remove(AnalyticsTrackerNosara.TRACKS_ANON_ID);
	editor.apply();
	}
	}
	}

view raw AnalyticsTrackerNosara.java hosted with ❤ by GitHub

Now, we can start pushing code into the Trackable User in order to consolidate concepts and get similar code closer together.

	interface TrackableUser {
	String user();
	TracksClient.NosaraUserType userType();
	}

	class NamedUser implements TrackableUser {
	String userName;
	NamedUser(String userName) {
	this.userName = userName;
	}

	@Override
	public String user() {
	return userName;
	}

	@Override
	public TracksClient.NosaraUserType userType() {
	return TracksClient.NosaraUserType.SIMPLENOTE;
	}
	}

	class AnonymousUser implements TrackableUser {
	AnonymousId id;
	AnonymousUser(AnonymousId id) {
	this.id = id;
	}

	@Override
	public String user() {
	return id.getId();
	}

	@Override
	public TracksClient.NosaraUserType userType() {
	return TracksClient.NosaraUserType.ANON;
	}
	}

	class UnknownUser implements TrackableUser {
	AnonymousId id;
	UnknownUser(AnonymousId id) {
	this.id = id;
	}

	@Override
	public String user() {
	return id.create();
	}

	@Override
	public TracksClient.NosaraUserType userType() {
	return TracksClient.NosaraUserType.ANON;
	}
	}

view raw AnalyticsTrackerNosara.java hosted with ❤ by GitHub

This makes the track() method start to look more purposeful.

	@Override
	public void track(AnalyticsTracker.Stat stat, String category, String label, Map<String, ?> properties) {
	if (mNosaraClient == null) {
	return;
	}

	String eventName = stat.name().toLowerCase();

	final TrackableUser trackable = createTrackableUser(mUserName, mAnonymousId);
	final String user = trackable.user();
	final TracksClient.NosaraUserType userType = trackable.userType();

	if (properties != null) {
	JSONObject propertiesJson = new JSONObject(properties);
	mNosaraClient.track(EVENTS_PREFIX + eventName, propertiesJson, user, userType);
	} else {
	mNosaraClient.track(EVENTS_PREFIX + eventName, user, userType);
	}
	}

view raw AnalyticsTrackerNosara.java hosted with ❤ by GitHub

Event tracking

Now to deal with that bottom section of the track() method. Extracting the event tracking and inlining the user and userType assignment really clarifies responsibilities here, IMO.

	@Override
	public void track(AnalyticsTracker.Stat stat, String category, String label, Map<String, ?> properties) {
	if (mNosaraClient == null) {
	return;
	}

	final TrackableUser trackable = createTrackableUser(mUserName, mAnonymousId);

	trackEvent(
	properties,
	stat.name().toLowerCase(),
	trackable.user(),
	trackable.userType()
	);
	}

	private void trackEvent(Map<String, ?> properties, String eventName, String user, TracksClient.NosaraUserType userType) {
	if (properties != null) {
	JSONObject propertiesJson = new JSONObject(properties);
	mNosaraClient.track(EVENTS_PREFIX + eventName, propertiesJson, user, userType);
	} else {
	mNosaraClient.track(EVENTS_PREFIX + eventName, user, userType);
	}
	}

view raw AnalyticsTrackerNosara.java hosted with ❤ by GitHub

After some final refactoring, here’s the updated file.

	package com.automattic.simplenote.analytics;

	import android.content.Context;
	import android.content.SharedPreferences;
	import android.text.TextUtils;

	import com.automattic.android.tracks.TracksClient;

	import org.json.JSONObject;

	import java.util.Map;
	import java.util.UUID;

	import androidx.preference.PreferenceManager;

	public class AnalyticsTrackerNosara implements AnalyticsTracker.Tracker {

	private static final String EVENTS_PREFIX = "spandroid_";

	private AnonymousId mAnonymousId;
	private String mUserName;
	final private TracksClient mNosaraClient;

	public AnalyticsTrackerNosara(Context context) {
	if (null == context) {
	mNosaraClient = null;
	return;
	}

	mAnonymousId = new AnonymousId(PreferenceManager.getDefaultSharedPreferences(context));
	mNosaraClient = TracksClient.getClient(context);
	}

	@Override
	public void track(AnalyticsTracker.Stat stat, String category, String label) {
	track(stat, category, label, null);
	}

	@Override
	public void track(AnalyticsTracker.Stat stat, String category, String label, Map<String, ?> properties) {
	if (mNosaraClient == null) {
	return;
	}

	final TrackableUser trackable = trackableUser(mUserName, mAnonymousId);
	trackEvent(
	properties,
	stat.name().toLowerCase(),
	trackable.user(),
	trackable.userType()
	);
	}

	private void trackEvent(Map<String, ?> properties, String eventName, String user, TracksClient.NosaraUserType userType) {
	if (properties != null) {
	JSONObject propertiesJson = new JSONObject(properties);
	mNosaraClient.track(EVENTS_PREFIX + eventName, propertiesJson, user, userType);
	} else {
	mNosaraClient.track(EVENTS_PREFIX + eventName, user, userType);
	}
	}

	private TrackableUser trackableUser(String userName, AnonymousId anonymousId) {
	if (userName != null) {
	return new NamedUser(userName, anonymousId);
	}
	if (anonymousId.getId() != null) {
	return new AnonymousUser(anonymousId);
	}
	return new UnknownUser(anonymousId);
	}

	@Override
	public void refreshMetadata(String username) {
	if (mNosaraClient == null) {
	return;
	}

	mUserName = (!TextUtils.isEmpty(username)) ? username : null;
	final TrackableUser trackable = trackableUser(mUserName, mAnonymousId);
	trackable.trackAliasUser(mNosaraClient);
	trackable.createAnonymousIdIfUserUnnamed();
	}

	@Override
	public void flush() {
	if (mNosaraClient != null) {
	mNosaraClient.flush();
	}
	}
	}

	interface TrackableUser {
	String user();
	TracksClient.NosaraUserType userType();
	void trackAliasUser(TracksClient mNosaraClient);
	void createAnonymousIdIfUserUnnamed();
	}

	class NamedUser implements TrackableUser {

	final private String userName;
	final private AnonymousId id;

	NamedUser(String userName, AnonymousId id) {
	this.userName = userName;
	this.id = id;
	}

	@Override
	public String user() {
	return userName;
	}

	@Override
	public TracksClient.NosaraUserType userType() {
	return TracksClient.NosaraUserType.SIMPLENOTE;
	}

	@Override
	public void trackAliasUser(TracksClient mNosaraClient) {
	if (id.getId() != null) {
	mNosaraClient.trackAliasUser(userName, id.getId(), TracksClient.NosaraUserType.SIMPLENOTE);
	id.clear();
	}
	}

	@Override
	public void createAnonymousIdIfUserUnnamed() {
	// no-op, this user is named
	}
	}

	class AnonymousUser implements TrackableUser {

	private final AnonymousId id;

	AnonymousUser(AnonymousId id) {
	this.id = id;
	}

	@Override
	public String user() {
	return id.getId();
	}

	@Override
	public TracksClient.NosaraUserType userType() {
	return TracksClient.NosaraUserType.ANON;
	}

	@Override
	public void trackAliasUser(TracksClient mNosaraClient) {
	// no-op, this user is unnamed
	}

	@Override
	public void createAnonymousIdIfUserUnnamed() {
	id.createIfMissing();
	}
	}

	class UnknownUser implements TrackableUser {

	private final AnonymousId id;
	UnknownUser(AnonymousId id) {
	this.id = id;
	}

	@Override
	public String user() {
	return id.create();
	}

	@Override
	public TracksClient.NosaraUserType userType() {
	return TracksClient.NosaraUserType.ANON;
	}

	@Override
	public void trackAliasUser(TracksClient mNosaraClient) {
	// no-op, this user is unnamed
	}

	@Override
	public void createAnonymousIdIfUserUnnamed() {
	id.createIfMissing();
	}
	}

	class AnonymousId {
	private static final String TRACKS_ANON_ID = "nosara_tracks_anon_id";
	private String uuid = null;

	private final SharedPreferences preferences;

	AnonymousId(SharedPreferences preferences) {
	this.preferences = preferences;
	}

	public String create() {
	uuid = UUID.randomUUID().toString();
	preferences.edit()
	.putString(TRACKS_ANON_ID, uuid)
	.apply();
	return uuid;
	}

	public void createIfMissing() {
	if (getId() == null) {
	create();
	}
	}

	public String getId() {
	if (uuid == null) {
	uuid = preferences.getString(TRACKS_ANON_ID, null);
	}
	return uuid;

	}
	public void clear() {
	uuid = null;
	if (preferences.contains(TRACKS_ANON_ID)) {
	preferences.edit()
	.remove(TRACKS_ANON_ID)
	.apply();
	}
	}
	}

view raw AnalyticsTrackerNosara.java hosted with ❤ by GitHub

Summary and takeaways

We clarified two concepts that seemed to want recognition. Both TrackableUser and AnonymousId seem to be workable concrete implementations that could have their own set of unit tests. That’s probably what I would do next before starting the (pretend) feature change that led to this refactor.

If I was new to this project, refactoring might be an excellent way to learn how the code works. Also important to remember is that you don’t have to push your first refactor. If you treat it like an onboarding kata, then it’s a safe way to explore the code and extract understanding.

Watch for var and function affixes to identify clumps of data (or functions) that can be grouped together.
Inside many long methods are concepts that deserve more explicit definition. Refactoring provides clarity; making it easier for the next developer to read and grok the intended behavior.
Watch for whitespace (or comments) in long methods. Often, those are seams between sections having (too) many responsibilities. These are helpful signposts indicating where to separate the large function into smaller, single-purpose ones. Many developers instinctively add those whitespace dividers because they “feel” right. Often what they are feeling is actually a code smell when a long function has too many responsibilities.