From 4d5407a5cc8cc91a130867d6dcc185a62349908f Mon Sep 17 00:00:00 2001 From: Alinson Xavier Date: Fri, 16 Sep 2016 13:17:52 -0400 Subject: [PATCH] Fix bug on compute methods that prevented them from recomputing old values --- .../common/views/ScoreChartTest.java | 2 +- .../sqlite/SQLiteCheckmarkListTest.java | 10 ++ .../models/sqlite/SQLiteScoreListTest.java | 15 +- .../habits/show/views/ScoreCard.java | 2 +- .../isoron/uhabits/models/CheckmarkList.java | 49 +++++- .../org/isoron/uhabits/models/ScoreList.java | 140 +++++++++++------- .../models/memory/MemoryCheckmarkList.java | 22 ++- .../models/memory/MemoryScoreList.java | 43 +++--- .../models/sqlite/SQLiteCheckmarkList.java | 31 +++- .../models/sqlite/SQLiteScoreList.java | 86 ++++++----- .../isoron/uhabits/widgets/ScoreWidget.java | 2 +- .../isoron/uhabits/models/ScoreListTest.java | 9 +- 12 files changed, 255 insertions(+), 156 deletions(-) diff --git a/app/src/androidTest/java/org/isoron/uhabits/activities/common/views/ScoreChartTest.java b/app/src/androidTest/java/org/isoron/uhabits/activities/common/views/ScoreChartTest.java index fa0f7f023..d747eb77a 100644 --- a/app/src/androidTest/java/org/isoron/uhabits/activities/common/views/ScoreChartTest.java +++ b/app/src/androidTest/java/org/isoron/uhabits/activities/common/views/ScoreChartTest.java @@ -49,7 +49,7 @@ public class ScoreChartTest extends BaseViewTest habit = fixtures.createLongHabit(); view = new ScoreChart(targetContext); - view.setScores(habit.getScores().getAll()); + view.setScores(habit.getScores().toList()); view.setColor(ColorUtils.getColor(targetContext, habit.getColor())); view.setBucketSize(7); measureView(view, dpToPixels(300), dpToPixels(200)); diff --git a/app/src/androidTest/java/org/isoron/uhabits/models/sqlite/SQLiteCheckmarkListTest.java b/app/src/androidTest/java/org/isoron/uhabits/models/sqlite/SQLiteCheckmarkListTest.java index d66e3bc12..32e6df853 100644 --- a/app/src/androidTest/java/org/isoron/uhabits/models/sqlite/SQLiteCheckmarkListTest.java +++ b/app/src/androidTest/java/org/isoron/uhabits/models/sqlite/SQLiteCheckmarkListTest.java @@ -92,6 +92,16 @@ public class SQLiteCheckmarkListTest extends BaseAndroidTest assertThat(list.get(7).getTimestamp(), equalTo(today - 10 * day)); } + @Test + public void testGetByInterval_withLongInterval() + { + long from = today - 200 * day; + long to = today; + + List list = checkmarks.getByInterval(from, to); + assertThat(list.size(), equalTo(201)); + } + @Test public void testInvalidateNewerThan() { diff --git a/app/src/androidTest/java/org/isoron/uhabits/models/sqlite/SQLiteScoreListTest.java b/app/src/androidTest/java/org/isoron/uhabits/models/sqlite/SQLiteScoreListTest.java index 1e05d9ecc..37b3c06a7 100644 --- a/app/src/androidTest/java/org/isoron/uhabits/models/sqlite/SQLiteScoreListTest.java +++ b/app/src/androidTest/java/org/isoron/uhabits/models/sqlite/SQLiteScoreListTest.java @@ -33,8 +33,6 @@ import org.junit.runner.*; import java.util.*; -import static junit.framework.Assert.assertNotNull; -import static junit.framework.Assert.assertNull; import static org.hamcrest.MatcherAssert.*; import static org.hamcrest.Matchers.*; @@ -66,7 +64,7 @@ public class SQLiteScoreListTest extends BaseAndroidTest @Test public void testGetAll() { - List list = scores.getAll(); + List list = scores.toList(); assertThat(list.size(), equalTo(121)); assertThat(list.get(0).getTimestamp(), equalTo(today)); assertThat(list.get(10).getTimestamp(), equalTo(today - 10 * day)); @@ -103,17 +101,6 @@ public class SQLiteScoreListTest extends BaseAndroidTest assertThat(records.get(0).timestamp, equalTo(today)); } - @Test - public void testGetByTimestamp() - { - Score s = scores.getByTimestamp(today); - assertNotNull(s); - assertThat(s.getTimestamp(), equalTo(today)); - - s = scores.getByTimestamp(today - 200 * day); - assertNull(s); - } - private List getAllRecords() { return new Select() diff --git a/app/src/main/java/org/isoron/uhabits/activities/habits/show/views/ScoreCard.java b/app/src/main/java/org/isoron/uhabits/activities/habits/show/views/ScoreCard.java index dde6f0533..f228c00dd 100644 --- a/app/src/main/java/org/isoron/uhabits/activities/habits/show/views/ScoreCard.java +++ b/app/src/main/java/org/isoron/uhabits/activities/habits/show/views/ScoreCard.java @@ -146,7 +146,7 @@ public class ScoreCard extends HabitCard List scores; ScoreList scoreList = getHabit().getScores(); - if (bucketSize == 1) scores = scoreList.getAll(); + if (bucketSize == 1) scores = scoreList.toList(); else scores = scoreList.groupBy(getTruncateField(bucketSize)); chart.setScores(scores); diff --git a/app/src/main/java/org/isoron/uhabits/models/CheckmarkList.java b/app/src/main/java/org/isoron/uhabits/models/CheckmarkList.java index 318bddc24..2b23199e7 100644 --- a/app/src/main/java/org/isoron/uhabits/models/CheckmarkList.java +++ b/app/src/main/java/org/isoron/uhabits/models/CheckmarkList.java @@ -176,19 +176,54 @@ public abstract class CheckmarkList * specified interval of time. Days that already have a corresponding * checkmark are skipped. * + * This method assumes the list of computed checkmarks has no holes. That + * is, if there is a checkmark computed at time t1 and another at time t2, + * then every checkmark between t1 and t2 is also computed. + * * @param from timestamp for the beginning of the interval * @param to timestamp for the end of the interval */ - protected final synchronized void compute(long from, final long to) + protected final synchronized void compute(long from, long to) { final long day = DateUtils.millisecondsInOneDay; - Checkmark newestCheckmark = getNewestComputed(); - if (newestCheckmark != null) - from = newestCheckmark.getTimestamp() + day; + Checkmark newest = getNewestComputed(); + Checkmark oldest = getOldestComputed(); + + if (newest == null) + { + forceRecompute(from, to); + } + else + { + forceRecompute(from, oldest.getTimestamp() - day); + forceRecompute(newest.getTimestamp() + day, to); + } + } + + /** + * Returns oldest checkmark that has already been computed. + * + * @return oldest checkmark already computed + */ + protected abstract Checkmark getOldestComputed(); + /** + * Computes and stores one checkmark for each day that falls inside the + * specified interval of time. + * + * This method does not check if the checkmarks have already been + * computed or not. If they have, then duplicate checkmarks will + * be stored, which is a bad thing. + * + * @param from timestamp for the beginning of the interval + * @param to timestamp for the end of the interval + */ + private synchronized void forceRecompute(long from, long to) + { if (from > to) return; + final long day = DateUtils.millisecondsInOneDay; Frequency freq = habit.getFrequency(); long fromExtended = from - (long) (freq.getDenominator()) * day; @@ -231,8 +266,8 @@ public abstract class CheckmarkList /** * Computes and stores one checkmark for each day, since the first - * repetition until today. Days that already have a corresponding checkmark - * are skipped. + * repetition of the habit until today. Days that already have a + * corresponding checkmark are skipped. */ protected final void computeAll() { @@ -245,8 +280,6 @@ public abstract class CheckmarkList /** * Returns newest checkmark that has already been computed. - *

- * Ignores any checkmark that has timestamp in the future. * * @return newest checkmark already computed */ diff --git a/app/src/main/java/org/isoron/uhabits/models/ScoreList.java b/app/src/main/java/org/isoron/uhabits/models/ScoreList.java index a756bdc92..126058d7b 100644 --- a/app/src/main/java/org/isoron/uhabits/models/ScoreList.java +++ b/app/src/main/java/org/isoron/uhabits/models/ScoreList.java @@ -57,19 +57,6 @@ public abstract class ScoreList implements Iterable */ public abstract void add(List scores); - public abstract List getAll(); - - /** - * Returns the score that has the given timestamp. - *

- * If no such score exists, returns null. - * - * @param timestamp the timestamp to find. - * @return the score with given timestamp, or null if none exists. - */ - @Nullable - public abstract Score getByTimestamp(long timestamp); - public ModelObservable getObservable() { return observable; @@ -88,22 +75,23 @@ public abstract class ScoreList implements Iterable /** * Returns the value of the score for a given day. *

- * If there is no score at the given timestamp (for example, if the - * timestamp given happens before the first repetition of the habit) then - * returns zero. + * If the timestamp given happens before the first repetition of the habit + * then returns zero. * * @param timestamp the timestamp of a day * @return score value for that day */ public final int getValue(long timestamp) { - Score s = getByTimestamp(timestamp); - if (s != null) return s.getValue(); - return 0; + compute(timestamp, timestamp); + Score s = getComputedByTimestamp(timestamp); + if(s == null) throw new IllegalStateException(); + return s.getValue(); } public List groupBy(DateUtils.TruncateField field) { + computeAll(); HashMap> groups = getGroupedValues(field); List scores = groupsToAvgScores(groups); Collections.sort(scores, (s1, s2) -> s2.compareNewer(s1)); @@ -122,9 +110,20 @@ public abstract class ScoreList implements Iterable @Override public Iterator iterator() { - return getAll().iterator(); + return toList().iterator(); } + /** + * Returns a Java list of scores, containing one score for each day, from + * the first repetition of the habit until today. + *

+ * The scores are sorted by decreasing timestamp. The first score + * corresponds to today. + * + * @return list of scores + */ + public abstract List toList(); + public void writeCSV(Writer out) throws IOException { computeAll(); @@ -140,16 +139,15 @@ public abstract class ScoreList implements Iterable } /** - * Computes and saves the scores that are missing inside a given time - * interval. + * Computes and stores one score for each day inside the given interval. *

* Scores that have already been computed are skipped, therefore there is no * harm in calling this function more times, or with larger intervals, than * strictly needed. The endpoints of the interval are included. *

- * This function assumes that there are no gaps on the scores. That is, if - * the newest score has timestamp t, then every score with timestamp lower - * than t has already been computed. + * This method assumes the list of computed scores has no holes. That is, if + * there is a score computed at time t1 and another at time t2, then every + * score between t1 and t2 is also computed. * * @param from timestamp of the beginning of the interval * @param to timestamp of the end of the time interval @@ -157,34 +155,24 @@ public abstract class ScoreList implements Iterable protected synchronized void compute(long from, long to) { final long day = DateUtils.millisecondsInOneDay; - final double freq = habit.getFrequency().toDouble(); - - int newestValue = 0; - long newestTimestamp = 0; Score newest = getNewestComputed(); - if (newest != null) + Score oldest = getOldestComputed(); + + if (newest == null) { - newestValue = newest.getValue(); - newestTimestamp = newest.getTimestamp(); + Repetition oldestRep = habit.getRepetitions().getOldest(); + if (oldestRep != null) + from = Math.min(from, oldestRep.getTimestamp()); + forceRecompute(from, to, 0); } - - if (newestTimestamp > 0) from = newestTimestamp + day; - - final int checkmarkValues[] = habit.getCheckmarks().getValues(from, to); - final long beginning = from; - - int lastScore = newestValue; - List scores = new LinkedList<>(); - - for (int i = 0; i < checkmarkValues.length; i++) + else { - int value = checkmarkValues[checkmarkValues.length - i - 1]; - lastScore = Score.compute(freq, lastScore, value); - scores.add(new Score(beginning + day * i, lastScore)); + if (oldest == null) throw new IllegalStateException(); + forceRecompute(from, oldest.getTimestamp() - day, 0); + forceRecompute(newest.getTimestamp() + day, to, + newest.getValue()); } - - add(scores); } /** @@ -196,20 +184,66 @@ public abstract class ScoreList implements Iterable Repetition oldestRep = habit.getRepetitions().getOldest(); if (oldestRep == null) return; - long toTimestamp = DateUtils.getStartOfToday(); - compute(oldestRep.getTimestamp(), toTimestamp); + long today = DateUtils.getStartOfToday(); + compute(oldestRep.getTimestamp(), today); } /** - * Returns the most recent score that has already been computed. - *

- * If no score has been computed yet, returns null. + * Returns the score that has the given timestamp, if it has already been + * computed. If that score has not been computed yet, returns null. * - * @return the newest score computed, or null if none exist + * @param timestamp the timestamp of the score + * @return the score with given timestamp, or null not yet computed. + */ + @Nullable + protected abstract Score getComputedByTimestamp(long timestamp); + + /** + * Returns the most recent score that has already been computed. If no score + * has been computed yet, returns null. */ @Nullable protected abstract Score getNewestComputed(); + /** + * Returns oldest score already computed. If no score has been computed yet, + * returns null. + */ + @Nullable + protected abstract Score getOldestComputed(); + + /** + * Computes and stores one score for each day inside the given interval. + *

+ * This function does not check if the scores have already been computed. If + * they have, then it stores duplicate scores, which is a bad thing. + * + * @param from timestamp of the beginning of the interval + * @param to timestamp of the end of the interval + * @param previousValue value of the score on the day immediately before the + * interval begins + */ + private void forceRecompute(long from, long to, int previousValue) + { + if(from > to) return; + + final long day = DateUtils.millisecondsInOneDay; + + final double freq = habit.getFrequency().toDouble(); + final int checkmarkValues[] = habit.getCheckmarks().getValues(from, to); + + List scores = new LinkedList<>(); + + for (int i = 0; i < checkmarkValues.length; i++) + { + int value = checkmarkValues[checkmarkValues.length - i - 1]; + previousValue = Score.compute(freq, previousValue, value); + scores.add(new Score(from + day * i, previousValue)); + } + + add(scores); + } + @NonNull private HashMap> getGroupedValues(DateUtils.TruncateField field) { diff --git a/app/src/main/java/org/isoron/uhabits/models/memory/MemoryCheckmarkList.java b/app/src/main/java/org/isoron/uhabits/models/memory/MemoryCheckmarkList.java index d8d45fb2a..5b02be26a 100644 --- a/app/src/main/java/org/isoron/uhabits/models/memory/MemoryCheckmarkList.java +++ b/app/src/main/java/org/isoron/uhabits/models/memory/MemoryCheckmarkList.java @@ -72,21 +72,17 @@ public class MemoryCheckmarkList extends CheckmarkList } @Override - protected Checkmark getNewestComputed() + protected Checkmark getOldestComputed() { - long newestTimestamp = 0; - Checkmark newestCheck = null; + if(list.isEmpty()) return null; + return list.getLast(); + } - for (Checkmark c : list) - { - if (c.getTimestamp() > newestTimestamp) - { - newestCheck = c; - newestTimestamp = c.getTimestamp(); - } - } - - return newestCheck; + @Override + protected Checkmark getNewestComputed() + { + if(list.isEmpty()) return null; + return list.getFirst(); } } diff --git a/app/src/main/java/org/isoron/uhabits/models/memory/MemoryScoreList.java b/app/src/main/java/org/isoron/uhabits/models/memory/MemoryScoreList.java index 2705d407d..0fafe0e8d 100644 --- a/app/src/main/java/org/isoron/uhabits/models/memory/MemoryScoreList.java +++ b/app/src/main/java/org/isoron/uhabits/models/memory/MemoryScoreList.java @@ -27,7 +27,7 @@ import java.util.*; public class MemoryScoreList extends ScoreList { - List list; + LinkedList list; public MemoryScoreList(Habit habit) { @@ -35,6 +35,24 @@ public class MemoryScoreList extends ScoreList list = new LinkedList<>(); } + @Override + public void add(List scores) + { + list.addAll(scores); + Collections.sort(list, + (s1, s2) -> Long.signum(s2.getTimestamp() - s1.getTimestamp())); + } + + @Nullable + @Override + public Score getComputedByTimestamp(long timestamp) + { + for (Score s : list) + if (s.getTimestamp() == timestamp) return s; + + return null; + } + @Override public void invalidateNewerThan(long timestamp) { @@ -49,7 +67,7 @@ public class MemoryScoreList extends ScoreList @Override @NonNull - public List getAll() + public List toList() { computeAll(); return new LinkedList<>(list); @@ -57,28 +75,17 @@ public class MemoryScoreList extends ScoreList @Nullable @Override - public Score getByTimestamp(long timestamp) - { - computeAll(); - for (Score s : list) - if (s.getTimestamp() == timestamp) return s; - - return null; - } - - @Override - public void add(List scores) + protected Score getNewestComputed() { - list.addAll(scores); - Collections.sort(list, - (s1, s2) -> Long.signum(s2.getTimestamp() - s1.getTimestamp())); + if (list.isEmpty()) return null; + return list.getFirst(); } @Nullable @Override - protected Score getNewestComputed() + protected Score getOldestComputed() { if (list.isEmpty()) return null; - return list.get(0); + return list.getLast(); } } diff --git a/app/src/main/java/org/isoron/uhabits/models/sqlite/SQLiteCheckmarkList.java b/app/src/main/java/org/isoron/uhabits/models/sqlite/SQLiteCheckmarkList.java index f33a3b277..ea42a846b 100644 --- a/app/src/main/java/org/isoron/uhabits/models/sqlite/SQLiteCheckmarkList.java +++ b/app/src/main/java/org/isoron/uhabits/models/sqlite/SQLiteCheckmarkList.java @@ -84,7 +84,7 @@ public class SQLiteCheckmarkList extends CheckmarkList public List getByInterval(long fromTimestamp, long toTimestamp) { check(habit.getId()); - computeAll(); + compute(fromTimestamp, toTimestamp); String query = "select habit, timestamp, value " + "from checkmarks " + @@ -119,7 +119,6 @@ public class SQLiteCheckmarkList extends CheckmarkList protected Checkmark getNewestComputed() { check(habit.getId()); - String query = "select habit, timestamp, value " + "from checkmarks " + "where habit = ? " + @@ -127,24 +126,42 @@ public class SQLiteCheckmarkList extends CheckmarkList "limit 1"; String params[] = { Long.toString(habit.getId()) }; + return getSingleCheckmarkFromQuery(query, params); + } - CheckmarkRecord record = sqlite.querySingle(query, params); - if (record == null) return null; - record.habit = habitRecord; - return record.toCheckmark(); + @Override + protected Checkmark getOldestComputed() + { + check(habit.getId()); + String query = "select habit, timestamp, value " + + "from checkmarks " + + "where habit = ? " + + "order by timestamp asc " + + "limit 1"; + + String params[] = { Long.toString(habit.getId()) }; + return getSingleCheckmarkFromQuery(query, params); } @Contract("null -> fail") private void check(Long id) { if (id == null) throw new RuntimeException("habit is not saved"); - if (habitRecord != null) return; habitRecord = HabitRecord.get(id); if (habitRecord == null) throw new RuntimeException("habit not found"); } + @Nullable + private Checkmark getSingleCheckmarkFromQuery(String query, String params[]) + { + CheckmarkRecord record = sqlite.querySingle(query, params); + if (record == null) return null; + record.habit = habitRecord; + return record.toCheckmark(); + } + @NonNull private List toCheckmarks(@NonNull List records) { diff --git a/app/src/main/java/org/isoron/uhabits/models/sqlite/SQLiteScoreList.java b/app/src/main/java/org/isoron/uhabits/models/sqlite/SQLiteScoreList.java index ecafd2cec..5ef6d54e8 100644 --- a/app/src/main/java/org/isoron/uhabits/models/sqlite/SQLiteScoreList.java +++ b/app/src/main/java/org/isoron/uhabits/models/sqlite/SQLiteScoreList.java @@ -84,46 +84,20 @@ public class SQLiteScoreList extends ScoreList } } - @Override - @NonNull - public List getAll() - { - check(habit.getId()); - computeAll(); - - String query = "select habit, timestamp, score from Score " + - "where habit = ? order by timestamp desc"; - - String params[] = {Long.toString(habit.getId())}; - - List records = sqlite.query(query, params); - for (ScoreRecord record : records) record.habit = habitRecord; - - List scores = new LinkedList<>(); - for (ScoreRecord rec : records) - scores.add(rec.toScore()); - - return scores; - } - @Override @Nullable - public Score getByTimestamp(long timestamp) + public Score getComputedByTimestamp(long timestamp) { check(habit.getId()); - computeAll(); String query = "select habit, timestamp, score from Score " + "where habit = ? and timestamp = ? " + "order by timestamp desc"; String params[] = - {Long.toString(habit.getId()), Long.toString(timestamp)}; + { Long.toString(habit.getId()), Long.toString(timestamp) }; - ScoreRecord record = sqlite.querySingle(query, params); - if (record == null) return null; - record.habit = habitRecord; - return record.toScore(); + return getScoreFromQuery(query, params); } @Override @@ -138,6 +112,28 @@ public class SQLiteScoreList extends ScoreList getObservable().notifyListeners(); } + @Override + @NonNull + public List toList() + { + check(habit.getId()); + computeAll(); + + String query = "select habit, timestamp, score from Score " + + "where habit = ? order by timestamp desc"; + + String params[] = { Long.toString(habit.getId()) }; + + List records = sqlite.query(query, params); + for (ScoreRecord record : records) record.habit = habitRecord; + + List scores = new LinkedList<>(); + for (ScoreRecord rec : records) + scores.add(rec.toScore()); + + return scores; + } + @Nullable @Override protected Score getNewestComputed() @@ -147,22 +143,38 @@ public class SQLiteScoreList extends ScoreList "where habit = ? order by timestamp desc " + "limit 1"; - String params[] = {Long.toString(habit.getId())}; + String params[] = { Long.toString(habit.getId()) }; + return getScoreFromQuery(query, params); + } - ScoreRecord record = sqlite.querySingle(query, params); - if (record == null) return null; - record.habit = habitRecord; - return record.toScore(); + @Nullable + @Override + protected Score getOldestComputed() + { + check(habit.getId()); + String query = "select habit, timestamp, score from Score " + + "where habit = ? order by timestamp asc " + + "limit 1"; + + String params[] = { Long.toString(habit.getId()) }; + return getScoreFromQuery(query, params); } @Contract("null -> fail") private void check(Long id) { if (id == null) throw new RuntimeException("habit is not saved"); - - if(habitRecord != null) return; - + if (habitRecord != null) return; habitRecord = HabitRecord.get(id); if (habitRecord == null) throw new RuntimeException("habit not found"); } + + @Nullable + private Score getScoreFromQuery(String query, String[] params) + { + ScoreRecord record = sqlite.querySingle(query, params); + if (record == null) return null; + record.habit = habitRecord; + return record.toScore(); + } } diff --git a/app/src/main/java/org/isoron/uhabits/widgets/ScoreWidget.java b/app/src/main/java/org/isoron/uhabits/widgets/ScoreWidget.java index e515be2af..7ff8cf1fe 100644 --- a/app/src/main/java/org/isoron/uhabits/widgets/ScoreWidget.java +++ b/app/src/main/java/org/isoron/uhabits/widgets/ScoreWidget.java @@ -69,7 +69,7 @@ public class ScoreWidget extends BaseWidget List scores; ScoreList scoreList = habit.getScores(); - if (size == 1) scores = scoreList.getAll(); + if (size == 1) scores = scoreList.toList(); else scores = scoreList.groupBy(ScoreCard.getTruncateField(size)); int color = ColorUtils.getColor(getContext(), habit.getColor()); diff --git a/app/src/test/java/org/isoron/uhabits/models/ScoreListTest.java b/app/src/test/java/org/isoron/uhabits/models/ScoreListTest.java index b5e64a840..329bc207c 100644 --- a/app/src/test/java/org/isoron/uhabits/models/ScoreListTest.java +++ b/app/src/test/java/org/isoron/uhabits/models/ScoreListTest.java @@ -110,14 +110,17 @@ public class ScoreListTest extends BaseUnitTest 3699107, 2846927, 1948077, - 1000000 + 1000000, + 0, + 0, + 0 }; + ScoreList scores = habit.getScores(); long current = DateUtils.getStartOfToday(); for (int expectedValue : expectedValues) { - assertThat(habit.getScores().getValue(current), - equalTo(expectedValue)); + assertThat(scores.getValue(current), equalTo(expectedValue)); current -= DateUtils.millisecondsInOneDay; } }