Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add autocomplete fields to TransactionViewActivity #152

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jjdelvalle
Copy link
Contributor

Trying to meet the needs presented in #110

Not usually an Android dev, but figured I'd contribute anyway since this is a feature I really wanted myself.

@brarcher
Copy link
Owner

since this is a feature I really wanted myself.

Sometimes that the best reason for doing it. (:

I've been thinking about this today and ran some experiments. Let me share some some of my thoughts.

Before I wrote this application I was using a closed source application which did some of the same things. The basic idea was the same, budgets and transactions. One feature that app had which mine does not is autocomplete.

One part of autocomplete that bothered me is if i entered something spelled incorrectly once, it would always appear in the autocomplete list from there on out. It would always remind me of that time I typed it wrong. Though, in that case, I think if i were to edit the transaction in question it would correct itself. I think that app would gather the hints from the values in the database, rather than keeping a separate list.

Reviewing the proposed change, my understanding is that it keeps a set of Strings for all descriptions, notes, values, and accounts, which is parallel to the data in the database. In addition, the sets will grow unbounded; the more unique entries added the more options added to the autocomplete. One thing that the change does not account for is how to restore that list if one uses the backup/restore option for the app.

Here are the four areas which I think need to be addressed for this change to be accepted.

  1. Source the data from the database
  2. Limit the number of options for autocomplete
  3. Reconsider autocomplete on the value
  4. Tests

Following are details.

  1. Source data from the database
    The database is the source of truth for the data. If a user adds an entry it can be retrieved and corrected if there is a mistake. Further, there is a backup and restore feature for budgets and transactions. To this end, it would be better to source the autocomplete hints from the database. A few extra helper methods in DBHelper could be added which returns a collection of Strings. This data could be fed into the autocomplete. This has a few benefits:
    a. Limits duplicate data storage for the app
    b. data mistakes can be corrected

  2. Limit the number of options
    Looking through my database from using the app over the last several years I find I have 902 unique company names/descriptions, 84 unique accounts (at one point I was putting check numbers here), and 426 unique hints. That sounds like a lot to me, and some of them are typos. I tried your changes out with filling in the shared preferences with that many random strings of 15 characters. The idea was to see if there was a significant memory usage difference. Interestingly, I did not observe a marked increase in memory usage. Running the experiment again where I used 10x as many random strings I did see an increase of 3-4mb. 100x brings 14mb. It may be that some use cases will result in additional memory usage. To this end, what are your thoughts about limiting the number of entries used in the autocomplete? For example, how about using the last unique 50-100 entries? I'm hoping that there is an efficient database query which can be used to determine these strings. If not, maybe just use the last 100 transactions as a source.

  3. Autocomplete on value
    Would you use the autocomplete for the value field? I suspect over time it will just get cluttered with all sorts of combinations, and ultimately not be helpful.

  4. Tests
    All new features will need to have unit tests added. Currently all the unit tests use the Robolectric framework, so that tests can run without an Android emulator. There are examples of these unit tests, and most of the application is covered by at least one test. (Some stuff is not covered, like displaying dialogs). Please add tests for the new features. This will help demonstrate correctness and help prevent the feature from breaking in the future.

Besides these three areas, there are a few smaller nit-pick areas. One is {} usage. I like putting the { on the next line instead of inline with the if.

I was hoping to not wall-of-text you, so sorry about that. If you are still interested in contributing this feature and would like some advice on the details (you mentioned you are not normally an Android dev), let me know; I'll be glad to point you in the right direction.

@jjdelvalle
Copy link
Contributor Author

jjdelvalle commented Mar 19, 2018

Hey, I appreciate your thorough thoughts! I'm not married to the idea of using SharedPreferences for this, I simply asked an Android dev friend and he recommended I use that; in fact, using the DB was my first thought so I'm quite open to the idea.

Source the data from the database

Like I mentioned above, I agree with this and the benefits are pretty convincing, I hadn't considered typos at all to be completely honest.

Limit the number of options

Not sure about how to handle this one. The last ones used could be a good idea. I was also thinking querying for the most used ones instead of the last ones. What do you think of that? As for the number of entries, that's up to you.

Autocomplete on value

Agreed. Just realized I misread the original issue.

Tests

This I'm gonna have to check out the rest of the test to get an idea out of, I haven't worked with that test framework but I'll work on those tests.

Besides these three areas, there are a few smaller nit-pick areas. One is {} usage. I like putting the { on the next line instead of inline with the if.

Haha, you got it!

And yeah, I'd appreciate the input along the way and you bet I'm still interested, feedback like this on PRs is motivating so thanks!

@brarcher
Copy link
Owner

Limit the number of options

For the first pass we could try listing all options from the database. From there it can be trimmed down. Depending on the performance impact, we can see if querying the most recent or the most frequent is the better option. I'd think that having the most recently used may be valuable, otherwise it will take many many entries until something finally appears in the autocomplete list. Maybe there is a balance.

@brarcher brarcher mentioned this pull request Sep 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants