hello, Aaron!

how can I help you today?

hey Mark!

I don't have any specific questions today. If you are up for it, I would really like it if you could just poke around in my project and see if any problems pop out. I'm interested in feedback at any level of analysis, but I'd say my main concern is identifying any major pieces that are being used incorrectly

OK, got a link?

put another way, anything that looks off that you might expect an interviewer to grill me about; better prepare for it

since this project has a few dozen classes and stuff, is there anything in particular you would like me to look at first?

not really, to be honest, in past "code reviews" I've called out specific things, but this time I guess I just want to invert that, and let you look at whatever you think might be interesting to look at

I can provide some guidance if needed, but at this point I've addressed most of the things that I know needed work, so I need more open-ended feedback

I guess just pretend that I sent this to you as part of a job application

since I haven't hired anyone in ~20 years, that's a bit far-fetched, but I get what you're saying

it's cool that you added the license headers! I'm uncertain if "2018-present" will hold up if it came to some sort of legal throwdown, but that's fairly unlikely anyway

OK, and lol at legal throwdown

I copied the headers from the RxJava repo, so if it's good enough for that, it's good enough for me

you have setupStrictMode(); commented out in BaseActivity -- I recommend either using it (e.g., based on a BuildConfig flag) or axing it entirely

BuildConfig is a good idea.

// region comments are a cool trick, but personally I find them annoying for smaller files -- I recommend using them only if they will materially help navigation within the source (so, for example, BaseActivity doesn't really need them IMHO)

OK true

https://github.com/ajh3/NoSurfForReddit/blob/ma... -- you have "activity" in the error message, but this is the fragment one (copy/paste error, I'm guessing)

yep

in NoSurfApplication, you have initLeakCanary() commented out -- so, same thing as with setupStrictMode() (use it or nuke it)

I think LeakCanary can actually be left in release builds, IIRC they somehow replace all of the classes/methods with empty stubs, I'll double check that

there are two separate dependencies, one you use with debugImplmentation and the other with releaseImplementation

left *enabled

that allows you to keep your LeakCanary configuration code alone, and it just no-ops on a release build

ah yeah, that's right

yeah I'll make sure that's set up right.

https://github.com/ajh3/NoSurfForReddit/blob/ma... -- code comments are for "future you" and team members, and I would expect all of them to know the things that you commented on

code comments are great, but at the same time, you don't want to come across as "talking down" to the reader of those comments

yeah, I understand, those were actually there for my own benefit but I get your point, I'll pull them out and keep that information in my private notes

there are likely more comments like that, I'll fix them

another possibility for that role is to add "note to self:" or something as a prefix

true

in that same file, deleteAllClickedPostIds() is marked with @SuppressWarnings("unused") -- is that really unused?

I have had some problems with Lint reporting stuff as unused that is actually used. As far as I can tell it's bugs in Lint. I added the annotations just to make it stop, not sure if that's wise though

hmmm... OK, that might be worth an explanatory comment, if the problem still exists

it's possible I'm just not understanding something about how Lint works though, i'm somewhat, but not super sure about it being bugs

ok

similarly, on ClickedPostIdRoomDatabase, you have @SuppressWarnings("ALL") -- that's kinda scary, so if you're sure you want that, a comment justifying the decision might be a good idea

OK

I'll try deleting all the annotations and re-running the inspections and take another look

things may have changed anyway since I added them, and in any case, I'll comment them this time

another commented out bit: addCallback(clickedPostIdRoomDatabaseCallback) in your RoomDatabase.Builder setup

part of the reason why I mention this commented-out code is that in a big app, unused code is just stuff that gets in the way of things like refactoring, and so people might be sensitive to having unused code lying around a small project like this

OK, that piece in particular I left there because I thought I might want to use it, but I get it, I'll either comment it or remove it

sounds like removing it is preferred

yeah, up until you want to start showing the code to others, having stuff around like that is perfectly reasonable

roger that, yeah I guess adding comments isn't going to help get it out of the way.

and keeping that stuff on a private git branch is also a possibilty

also in your RoomDatabase.Builder setup, since you're still on schema version 1, fallbackToDestructiveMigration() shouldn't be needed

I need to spend a few days learning more git, I actually think I'm going to get roasted about that in interviews, as my commit history is just linear commits to master with nothing else, not so ideal

OK

eh, for a small single-developer project like this, not using an elaborate branching strategy is reasonable

being honest about your level of git skills is appropriate, but you don't have to show *everything* off in this repo

ah, well, good lol

you seem to go back and forth between using constants for strings or using them inline, such as the "clicked_post_id_database" parameter in your RoomDatabase.Builder

ah, that's an oversight, I meant to extract them all out, will fix it

you can do what you want, but I recommend an identifiable pattern and one that you're willing to defend

yes, good catch

"always defined as constants" or "always defined as constants if referred to 2+ times" are good identifiable patterns

I like the former

https://github.com/ajh3/NoSurfForReddit/blob/ma... -- you have an interface to isolate the implementation details, but then you leak an implementation detail in the form of SharedPreferences.OnSharedPreferenceChangeListener

*technically*, you can use that listener for things that are not SharedPreferences, but that'd be weird

ok, so you're saying I should make some kind of custom listener class that just wraps SharedPreferences.OnSharedPreferenceChangeListener?

and use that instead?

ideally, yes

OK

got it

makese sense

actually, it's more that SettingsStore would have its own listener interface (e.g., SettingsStore.Listener)

your clients would implement that interface instead of OnSharedPreferenceChangeListener

ah, yes, that would align with SharedPreferences having its listener, sounds good

and listeners are usually interfaces (or abstract classes)

SettingsStore has no setters -- when do these values get changed?

tbh there are a lot of places in this app where I could abstract more, like my repository just exposes retrofit calls, it doesn't try to wrap that in any API of its own, but i'm just accepting that since I am never going to replace retrofit, and that's what I'll tell interviewers if they ask

OK

er, I'm not sure why those variables are even in the interface there

I should probably have setters and getters in the interface and NOT the variables

I think, at least

I'm not sure what you're referring to precisely -- I just don't see where your preferences get updated

there is a preference fragment

ah

a settings screen

yeah

in that case, I would recommend just getting rid of SettingsStore

you're kinda locked into SharedPreferences as the data storage vehicle, so the abstraction isn't buying you anything

keep PreferenceSettingsStore, and have clients use it, just without the interface

yeah, I had included it more just to show an example of abstraction, but that might be counterproductive if there's not a reason for doing so

and what I was referring to regarding the variables, it's my understanding that you only really include variables in an interface if you want to have constants

um, you lost me there, but, that's OK

lol ok, I'll brush up on that

looking at RateLimitInterceptor, you have another couple of strings not as constants

also, just to confirm: the Reddit rate-limit is per-client (i.e, per-user), not per-app, right?

correct

that's rather nice of them

a lot of these rate limits are per-app, which makes client-side use troublesome if you get popular

I had to actually dig to figure that out, I had the same question and it wasn't clear in their docs

but yes

also in RateLimitInterceptor, you should be able to avoid the division, and hence the division-by-zero scenario

you have int requestRatePerSecondDuringThisPeriod = xRateLimitUsed / (600 - xRateLimitReset);

and you have if (requestRatePerSecondDuringThisPeriod >= 1)

which means that you are comparing xRateLimitUsed / (600 - xRateLimitReset) >= 1

which is equivalent to xRateLimitUsed >= 600 - xRateLimitReset (multiplying both sides by 600 - xRateLimitReset)

math is hard

I'll fix it

if you can avoid the division in your algorithm, it's probably a good thing

agree

having a Thread.sleep() in the interceptor is kinda icky, though I agree that it is a convenient solution

I don't have tests for that class and I only noticed the division by zero when it happened to occur

thanks so much for all of this, for the last few minutes of the chat can you tell me what you think about this:

pretty open ended and subjective but nevertheless

I'm going to address all of this, and some feedback I've gotten from other people, but I'm mostly switching over to job seeking mode, although I want to keep working on code or I'm going to start forgetting stuff, so my plan is

to start working through one of Google's sample apps, either the I/O app or Sunflower

I think that would be a really good next step, as I should have enough baseline knowledge now to make sense of those

seem like a reasonable plan to you? like I said I know it's subjective

¯\_(ツ)_/¯

lol yeah

fair enough

I mean, I tend to be more tactical when reviewing other apps

but that's me, and I know lots of people like to sit down and tear through an app like those to see stuff

yeah, my feeling is just that it would 1) be good to get practice absorbing other people's code, and 2) these Jetpack demo apps use lots of the same stuff I used, except probably more correctly, so I can learn from that

that's all reasonable

anyway yeah, that's what I'm going to do, mainly just wanted to see if that generated any particularly storng response from you, but sounds like it's reasonable, exactly

cool

alright, that's awesome, thanks so much for your help today

you're welcome!

I'll probably still pop in with some questions in the next few weeks, but not nearly as heavily as I have been

enjoy the rest of the weekend, talk later

that sounds fine

see ya!