Office Hours — Today, November 3

← Thursday, November 1

Nov 3
3:50 PM
Mark M.
has entered the room
Mark M.
turned on guest access
4:00 PM
Aaron
has entered the room
Mark M.
hello, Aaron!
how can I help you today?
Aaron
hey, hey. I am ready for that mini-code review if it works out today :)
Mark M.
OK
Aaron
here's what I came up with in terms of priorities
View paste (2 more lines) 
Ok, so conceptually, what would be most helpful is higher-level feedback, such as about the app's architecture, interactions between components, violations of SOLID principles and other best practices, and so on. I am less concerned about the internal implementation of methods, etc., I'm sure they need plenty of work but it feels like a second priority. Overall I'm open to what you come up with though, as a big reason I am asking you to do this, is to call out my unknown unknowns. So I don't want to be too specific.

Structurally, I think these classes are the priority.

1. ViewModel and Repository
2. MainActivity
3. PostFragment & subclasses
4. PostsFragment & subclasses

One particular overarching question is where I should abstract more. For example most of my components are fairly tightly coupled and I am not making very prodigious use of interfaces, but then again, the purpose of the program is inherently limited and unlikely to change or grow significantly in the future. Similar kind of question on the method level. 

Let me know if any of that doesn't make sense, but that's the best I could come up with in terms of what I am hoping to get out of this. Thank you in advance.
...
Mark M.
OK
4:05 PM
Mark M.
particularly in a chat structure, I'm going to have to do this pretty much "stream of consciousness", as I read things
Aaron
my general game plan is to make changes based on your feedback, publish the first version to Google Play and put it in front of a small group of people who will be interested while I integrate RxJava and convert some classes to Kotlin and write some unit tests. FYI
yep, sounds good
Mark M.
so while I can take the classes at least somewhat in the order that you list them, my comments will likely be all over the map (high level, low level, etc.)
Aaron
yep
Mark M.
I'll try to budget time at the end to circle back and try to address some of your architecture-y concerns
Aaron
that would be awesome
Mark M.
so, first nit: NoSurfRepository.java has an empty case FETCH_SUBSCRIBED_POSTS_SYNC that you could remove or add a // TODO comment to indicate what your plans are
your onFailure() callbacks just seem to log to LogCat -- how does the user find out if something doesn't work?
4:10 PM
Mark M.
question: for your methods whose names end in "Sync" -- what does the "Sync" mean?
Aaron
asynchronous network calls
Mark M.
then... shouldn't it be "Async" instead of "Sync"?
Aaron
that was a convention mentioned in a course I took, I thought the same thing. guess it's not common, I'll change it
Mark M.
having a convention to indicate an asynchronous call is reasonable... but then the convention should indicate an asynchronous call
Aaron
agreed
Mark M.
IMHO, "Sync" implies a synchronous call, or possibly data synchronizatino
er, synchronization
but on the whole, I don't see method names using that sort of suffix all that often
Aaron
ok, good to know
oh yeah, one other thing I'll mention in terms of my game plan, is that instead of starting another app after this or trying to add new features, I'm going to spend a lot of time reading other people's code from big open source projects. think I can learn a lot that way
Mark M.
warning: just because it's open source does not mean that it either good or all that readable
code smell: don't use an AsyncTask when a Thread will do
4:15 PM
Mark M.
in particular, if your AsyncTask does not override onPostExecute(), because you have nothing that you need to do on the main application thread, do not use AsyncTask
Aaron
OK
Mark M.
probably use a ThreadPoolExecutor over a plain Thread, but either of those over an AsyncTask where one is not needed
Aaron
makes sense, as I understand AsyncTask is there to help post results to the UI thread
Mark M.
right, and you're not really using it for that
AsyncTask also has a bad reputation nowadays, and so using it is "not a good look", as they say
in your viewmodel, while I used "stitched together" as a phrase, I suspect that using "stitched" in a field name is going to cause confusion
Aaron
because it has another common meaning, or because it has no common meaning?
Mark M.
because it's mostly used in sewing :-)
Aaron
I called it zip at first but I felt like that would be confusing since it's not really that
LOL
ok
4:20 PM
Mark M.
"merged" or "combined" might be better choices
4:20 PM
Mark M.
BTW, overall: load up on code comments
Aaron
OK
Mark M.
your objective is to impress code reviewers more than end users, if I understand your situation correctly
reviewers coming in cold to a code base appreciate comments
Aaron
both, but yes, and will do
Mark M.
you have some good tactical ones, but JavaDoc comments would be a nice addition
Aaron
roger that
Mark M.
question: in your viewmodel, why do you have a separate initApp() method, instead of initializing in the constructor?
IOW, what is the trigger for initApp() being called, if not just when the viewmodel is instantiated?
Aaron
that was one of the first methods I wrote, and I think at the time it was important that it happen at some specific point when I was calling it from the activity's onCreate, but I don't think it matters anymore, can probably roll it into the constructor
good call, I'll check it
4:25 PM
Mark M.
question: if logUserOut() is called on the viewmodel, is the viewmodel still usable? or will other viewmodel methods start to fail until something is done to log the user in again?
Aaron
mostly it is, only the methods with "Subscribed" in the name rely on the user being logged in. all the others can work with an anonymous token
and those methods will do nothing if the user is logged out
Mark M.
at minimum, that's worth a comment
Aaron
OK
it would be more obvious to someone familiar with Reddit, but you are right, I should not assume that anyone looking at the code is familiar.
*everyone
4:30 PM
Mark M.
I would review how you are using SingleLiveEvent, as I don't think you're using it correctl
er, correctly
in particular, I don't think that the way it is written that you're supposed to do your "consume" step
personally, I prefer the "Use an Event wrapper" approach from https://medium.com/androiddevelopers/livedata-w...
Aaron
OK yes I think you are right, and I will investigate that
Mark M.
another area that could really use comments is anywhere you find yourself having to work around oddities in somebody else's stuff
for example, if you find yourself naming a parameter twiceEncodedSelfTextHtml, you probably need some comments
Aaron
OK agree
4:35 PM
Mark M.
your repository is exposing Retrofit data transfer objects (DTOs) as its model data -- as I think we discussed in a previous chat, ideally your repository exposes model objects that are designed for your UI consumption, hiding the DTOs
Aaron
oh I see. so you are saying, the "stitch" method and all that should be in the repository instead of the vm?
Mark M.
yes
that way, if Reddit changes details of their API, or you switch to something other than Retrofit, or you start caching data, the UI doesn't care
hide all the icky stuff in the repository
the UI is plenty icky already just due to Android concerns :-)
Aaron
I see, I had put it in the vm on the premise that one role of the vm is to prepare data for the UI, so that seemed like the logical thing to do
I guess I have the wrong definition of "prepare" in this case
Mark M.
you can think of it as being three tiers:
on the lowest level, you have the data that you get back from Retrofit and Room
those have limitations based on Retrofit, Gson, Room, etc. and may not be ideal for use in the UI layer
in the middle level, you have model objects that clean up those Retrofit/Room objects into something that is more natural
possibly, at the high level, you have objects that represent the subset of the models that you need, with transformations that might be needed in a specific UI element
4:40 PM
Mark M.
(that may or may not be needed depending on the complexity of the app)
so, for example, let's take the classic problem: dates
your Web service will represent dates on way, and your Room entities might represent dates some other way
so the lower-level objects (DTOs) need to know about that
the "true" model objects in the middle layer would use something more natural: java.util.Date, Joda-Time classes, etc.
however, at the high-level, this one TextView might want to show that date in a particular user format (e.g., YYYY-MM-DD), so you have code that converts the Date into the string representation that you want to show
in some cases, you can avoid having a third tier of objects depending on how you populate your UI (e.g., data binding expressions and binding adapters)
the details of that will vary a lot based on how you want to populate your UI
Aaron
I see, makes sense
Mark M.
View paste 
my main concern is getting stuff like this out of your viewmodel:

    private String getCommentAuthor(List<Listing> input, int i) {
        return input
                .get(1)
                .getData()
                .getChildren()
                .get(i)
                .getData()
                .getAuthor();
    }
the fact that Reddit's API has that nasty set of nested stuff really shouldn't be the viewmodel's concern
Aaron
View paste 
yup, OK, I get it, it's basically centers around what you said here -> "
that way, if Reddit changes details of their API, or you switch to something other than Retrofit, or you start caching data, the UI doesn't care"
Mark M.
right
4:45 PM
Aaron
I'll fix it
Mark M.
on the other hand, stuff like your formatCommentDetails() is reasonable, because thing like "we want a dot here" isn't a model thing, but rather is a UI thing
Aaron
OK
Mark M.
your decodeHtml() seems to duplicate code from just a few lines above it -- getCommentBodyHtml() probably should call decodeHtml()
(and, sometime next year when you move to AndroidX, you can use HtmlCompat and eliminate that version check)
Aaron
already switched it over to AndroidX, and clearly I am not taking advantage of it, because I didn't know it afforded a way to eliminate version checks
Mark M.
it's more that they *finally* released an HtmlCompat class
(only about six years after they should have)
Aaron
I see
Mark M.
most of the ...Compat classes in the support library/AndroidX are there to deal with version incompatibiltiies
er, incompatibilities
HtmlCompat, in particular, handles the changes they made to Html in N, so you can use HtmlCompat and it will "do the right thing" for you
4:50 PM
Aaron
nice, I'll look into these classes
Mark M.
going back to your opening pasted comments, "One particular overarching question is where I should abstract more" -- if you literally mean abstract classes, I don't know that you necessarily need a ton
Aaron
I didn't really realize that AndroidX had anything new, I thought for now it was mostly just a reorganization, I'll read up on that
Mark M.
however, going back to my tiered models stuff, I would consider the middle tier to be an "abstraction" -- an idealized in-memory representation of the data that you need
Aaron
OK
Mark M.
well, most of the ...Compat classes have been around for a while, and did just get repackaged
HtmlCompat happens to be new
https://commonsware.com/blog/2018/05/29/at-last... for a few notes from me on HtmlCompat
regarding MainActivity, I think the pattern going forward is to avoid all those listeners
your activity can have its own viewmodel, which your fragments can access (in addition to their own "local" viewmodel)
that activity viewmodel can have LiveData representing events that are being raised from the fragments that the activity should react to
Aaron
OK
aside from the listeners,
4:55 PM
Aaron
does it make sense that I made MainActivity the "nexus" of launching different fragments?
Mark M.
if you mean as opposed to fragments launching fragments, yes
Aaron
rather than having them launched from various other places, it seemed helpful to consolidate it all somewhere, so I picked there
Mark M.
that's perfectly reasonable
Aaron
OK
Mark M.
and, depending on what UI you might want to do for larger screens, that approach might be essential
Aaron
tablet support is on my list, eventually, but it seems lower priority than learning RxJava
Mark M.
in your case, your NoSurfViewModel is already shared, so you can add LiveData to it to reflect events that are in the fragments' UI but have effects that transcend the individual fragment, such as those necessitating a FragmentTransaction
for your purposes, RxJava is more important than is tablet support, I'd say
Aaron
yep, I think RxJava and unit/integration testing are the highest priorities, learning some Kotlin second, everything else third
Mark M.
we're just about out of time -- any questions?
Aaron
roughly...
not really, I'll need to research some of this stuff further to implement it, but everything you said makes sense
Mark M.
that's reasonable
Aaron
thanks a lot, this was very useful
Mark M.
happy to help!
5:00 PM
Aaron
a few random questions but I'll save them for next time
have a good weekend!
Mark M.
OK
you too!
Aaron
toodles
Aaron
has left the room
Mark M.
turned off guest access

← Thursday, November 1

 

Office Hours

People in this transcript

  • Aaron
  • Mark Murphy