From 476b3af20661d9e6d52e2046abff34e1534c889d Mon Sep 17 00:00:00 2001 From: Sam Bosley Date: Fri, 27 Apr 2012 14:36:54 -0700 Subject: [PATCH] Refactor and cleanup based on Tim's suggestions --- .../todoroo/astrid/dao/ABTestEventDao.java | 19 +++++++---------- .../com/todoroo/astrid/data/ABTestEvent.java | 21 +++++++------------ .../ABTestEventReportingService.java | 7 ++++--- .../service/abtesting/ABTestInvoker.java | 6 ++---- .../astrid/service/abtesting/ABTests.java | 16 ++++---------- .../astrid/service/ABTestingServiceTest.java | 6 +++--- 6 files changed, 28 insertions(+), 47 deletions(-) diff --git a/astrid/src/com/todoroo/astrid/dao/ABTestEventDao.java b/astrid/src/com/todoroo/astrid/dao/ABTestEventDao.java index 7eeaaff31..c070f9b41 100644 --- a/astrid/src/com/todoroo/astrid/dao/ABTestEventDao.java +++ b/astrid/src/com/todoroo/astrid/dao/ABTestEventDao.java @@ -36,7 +36,7 @@ public class ABTestEventDao extends DatabaseDao { event.setValue(ABTestEvent.TEST_VARIANT, testVariant); event.setValue(ABTestEvent.NEW_USER, newUser ? 1 : 0); event.setValue(ABTestEvent.ACTIVATED_USER, activeUser ? 1 : 0); - event.setValue(ABTestEvent.TIME_INTERVAL, ABTestEvent.TIME_INTERVAL_0); + event.setValue(ABTestEvent.TIME_INTERVAL, 0); event.setValue(ABTestEvent.DATE_RECORDED, DateUtilities.now()); createNew(event); @@ -90,25 +90,22 @@ public class ABTestEventDao extends DatabaseDao { */ public void createRelativeDateEvents() { TodorooCursor allEvents = query(Query.select(ABTestEvent.TEST_NAME, ABTestEvent.DATE_RECORDED) - .where(ABTestEvent.TIME_INTERVAL.eq(ABTestEvent.TIME_INTERVAL_0))); + .where(ABTestEvent.TIME_INTERVAL.eq(0))); try { long now = DateUtilities.now(); + ABTestEvent event = new ABTestEvent(); for (allEvents.moveToFirst(); !allEvents.isAfterLast(); allEvents.moveToNext()) { - ABTestEvent event = new ABTestEvent(allEvents); + event.readFromCursor(allEvents); long baseTime = event.getValue(ABTestEvent.DATE_RECORDED); long timeSinceBase = now - baseTime; String testName = event.getValue(ABTestEvent.TEST_NAME); int timeInterval = -1; - if (timeSinceBase > 3 * DateUtilities.ONE_WEEK) - timeInterval = ABTestEvent.TIME_INTERVAL_21; - else if (timeSinceBase > 2 * DateUtilities.ONE_WEEK) - timeInterval = ABTestEvent.TIME_INTERVAL_14; - else if (timeSinceBase > DateUtilities.ONE_WEEK) - timeInterval = ABTestEvent.TIME_INTERVAL_7; - else if (timeSinceBase > 3 * DateUtilities.ONE_DAY) - timeInterval = ABTestEvent.TIME_INTERVAL_3; + long days = timeSinceBase / DateUtilities.ONE_DAY; + for(int i = 0; i < ABTestEvent.TIME_INTERVALS.length; i++) + if(days >= ABTestEvent.TIME_INTERVALS[i]) + timeInterval = ABTestEvent.TIME_INTERVALS[i]; if (timeInterval > 0) createTestEventWithTimeInterval(testName, timeInterval); diff --git a/astrid/src/com/todoroo/astrid/data/ABTestEvent.java b/astrid/src/com/todoroo/astrid/data/ABTestEvent.java index 6cd0c6d05..145fcc511 100644 --- a/astrid/src/com/todoroo/astrid/data/ABTestEvent.java +++ b/astrid/src/com/todoroo/astrid/data/ABTestEvent.java @@ -20,19 +20,7 @@ import com.todoroo.astrid.api.AstridApiConstants; @SuppressWarnings("nls") public class ABTestEvent extends AbstractModel { - public static final int TIME_INTERVAL_0 = 0; - public static final int TIME_INTERVAL_3 = 3; - public static final int TIME_INTERVAL_7 = 7; - public static final int TIME_INTERVAL_14 = 14; - public static final int TIME_INTERVAL_21 = 21; - - public static final int[] TIME_INTERVALS = { - TIME_INTERVAL_0, - TIME_INTERVAL_3, - TIME_INTERVAL_7, - TIME_INTERVAL_14, - TIME_INTERVAL_21 - }; + public static final int[] TIME_INTERVALS = { 0, 3, 7, 14, 21 }; // --- table and uri @@ -80,7 +68,8 @@ public class ABTestEvent extends AbstractModel { /** * Which time interval event this data point corresponds to. - * Should be one of the time interval constants defined above. + * Should be one of the time interval constants defined int the + * above array. */ public static final IntegerProperty TIME_INTERVAL = new IntegerProperty( TABLE, "timeInterval"); // one of the constants defined above @@ -121,6 +110,10 @@ public class ABTestEvent extends AbstractModel { readPropertiesFromCursor(cursor); } + public void readFromCursor(TodorooCursor cursor) { + super.readPropertiesFromCursor(cursor); + } + @Override public long getId() { return getIdHelper(ID); diff --git a/astrid/src/com/todoroo/astrid/service/abtesting/ABTestEventReportingService.java b/astrid/src/com/todoroo/astrid/service/abtesting/ABTestEventReportingService.java index 426ff19eb..bf4b91508 100644 --- a/astrid/src/com/todoroo/astrid/service/abtesting/ABTestEventReportingService.java +++ b/astrid/src/com/todoroo/astrid/service/abtesting/ABTestEventReportingService.java @@ -74,8 +74,9 @@ public final class ABTestEventReportingService { try { JSONArray payload = jsonArrayFromABTestEvents(unreported); abTestInvoker.post(payload); + ABTestEvent model = new ABTestEvent(); for (unreported.moveToFirst(); !unreported.isAfterLast(); unreported.moveToNext()) { - ABTestEvent model = new ABTestEvent(unreported); + model.readFromCursor(unreported); model.setValue(ABTestEvent.REPORTED, 1); abTestEventDao.saveExisting(model); } @@ -114,9 +115,9 @@ public final class ABTestEventReportingService { String lastTestKey = ""; JSONObject testAcc = null; + ABTestEvent model = new ABTestEvent(); for (events.moveToFirst(); !events.isAfterLast(); events.moveToNext()) { - ABTestEvent model = new ABTestEvent(events); - + model.readFromCursor(events); if (!model.getValue(ABTestEvent.TEST_NAME).equals(lastTestKey)) { if (testAcc != null) result.put(testAcc); diff --git a/astrid/src/com/todoroo/astrid/service/abtesting/ABTestInvoker.java b/astrid/src/com/todoroo/astrid/service/abtesting/ABTestInvoker.java index b7a048e52..2334dd5a6 100644 --- a/astrid/src/com/todoroo/astrid/service/abtesting/ABTestInvoker.java +++ b/astrid/src/com/todoroo/astrid/service/abtesting/ABTestInvoker.java @@ -48,8 +48,6 @@ public class ABTestInvoker { public JSONObject post(JSONArray payload) throws IOException { try { HttpEntity postData = createPostData(payload); - if (postData == null) - throw new IOException("Unsupported URL encoding"); String response = restClient.post(URL, postData); JSONObject object = new JSONObject(response); if (object.getString("status").equals("error")) { @@ -69,7 +67,7 @@ public class ABTestInvoker { * @param payload * @return */ - private HttpEntity createPostData(JSONArray payload) { + private HttpEntity createPostData(JSONArray payload) throws IOException { List params = new ArrayList(); params.add(new BasicNameValuePair("apikey", API_KEY)); params.add(new BasicNameValuePair("payload", payload.toString())); @@ -92,7 +90,7 @@ public class ABTestInvoker { try { return new UrlEncodedFormEntity(params, HTTP.UTF_8); } catch (UnsupportedEncodingException e) { - return null; + throw new IOException("Unsupported URL encoding"); } } diff --git a/astrid/src/com/todoroo/astrid/service/abtesting/ABTests.java b/astrid/src/com/todoroo/astrid/service/abtesting/ABTests.java index 8d2648721..66b797819 100644 --- a/astrid/src/com/todoroo/astrid/service/abtesting/ABTests.java +++ b/astrid/src/com/todoroo/astrid/service/abtesting/ABTests.java @@ -116,21 +116,13 @@ public class ABTests { private void initialize() { // Set up //Calls to addTest go here - addTest(AB_TEST_SWIPE_ENABLED_KEY, AB_TEST_SWIPE_ENABLED_PROBS_NEW_USER, - AB_TEST_SWIPE_ENABLED_PROBS_EXISTING_USER, AB_TEST_SWIPE_ENABLED_DESC); - addTest(AB_TEST_CONTACTS_PICKER_ENABLED, AB_TEST_CONTACTS_ENABLED_PROBS_NEW_USER, - AB_TEST_CONTACTS_ENABLED_PROBS_EXISTING_USER, AB_TEST_CONTACTS_ENABLED_DESC); + addTest(AB_TEST_SWIPE_ENABLED_KEY, new int[] { 1, 1 }, + new int[] { 1, 1 }, new String[] { "swipe-lists-disabled", "swipe-lists-enabled" }); //$NON-NLS-1$//$NON-NLS-2$ + addTest(AB_TEST_CONTACTS_PICKER_ENABLED, new int[] { 1, 1 }, + new int[] { 1, 1 }, new String[] { "contacts-disabled", "contacts-enabled" }); //$NON-NLS-1$//$NON-NLS-2$ } public static final String AB_TEST_SWIPE_ENABLED_KEY = "swipeEnabled"; //$NON-NLS-1$ - private static final int[] AB_TEST_SWIPE_ENABLED_PROBS_NEW_USER = { 1, 1 }; - private static final int[] AB_TEST_SWIPE_ENABLED_PROBS_EXISTING_USER = { 1, 1 }; - private static final String[] AB_TEST_SWIPE_ENABLED_DESC = { "swipe-lists-disabled", "swipe-lists-enabled" }; //$NON-NLS-1$//$NON-NLS-2$ - public static final String AB_TEST_CONTACTS_PICKER_ENABLED = "contactsEnabled"; //$NON-NLS-1$ - private static final int[] AB_TEST_CONTACTS_ENABLED_PROBS_NEW_USER = { 1, 1 }; - private static final int[] AB_TEST_CONTACTS_ENABLED_PROBS_EXISTING_USER = { 1, 1 }; - private static final String[] AB_TEST_CONTACTS_ENABLED_DESC = { "contacts-disabled", "contacts-enabled" }; //$NON-NLS-1$//$NON-NLS-2$ - } diff --git a/tests/src/com/todoroo/astrid/service/ABTestingServiceTest.java b/tests/src/com/todoroo/astrid/service/ABTestingServiceTest.java index b7f194969..c1802b08f 100644 --- a/tests/src/com/todoroo/astrid/service/ABTestingServiceTest.java +++ b/tests/src/com/todoroo/astrid/service/ABTestingServiceTest.java @@ -27,7 +27,7 @@ public class ABTestingServiceTest extends DatabaseTestCase { } private void testInitialEvents(boolean newUser, boolean activatedUser) { - testInterval(newUser, activatedUser, ABTestEvent.TIME_INTERVAL_0); + testInterval(newUser, activatedUser, 0); } public void testIntervalEventWithShortIntervalNewUser() { @@ -47,11 +47,11 @@ public class ABTestingServiceTest extends DatabaseTestCase { } private void testIntervalEventWithShortInterval(boolean newUser, boolean activatedUser) { - testInterval(newUser, activatedUser, ABTestEvent.TIME_INTERVAL_3); + testInterval(newUser, activatedUser, 3); } private void testIntervalEventWithLongInterval(boolean newUser, boolean activatedUser) { - testInterval(newUser, activatedUser, ABTestEvent.TIME_INTERVAL_14); + testInterval(newUser, activatedUser, 14); } private void testInterval(boolean newUser, boolean activatedUser, int testInterval) {