search

fenimore / DemocracyDroid

Android application for watching Democracy Now! The war and peace report
https://play.google.com/store/apps/details?id=com.workingagenda.democracydroid
GNU General Public License v3.0
24 stars 2 forks source link

Code Refactoring. #44

Closed DerrickRocha closed 6 years ago

DerrickRocha commented 6 years ago

Cleaned up architecture. Added dependency injection.

fenimore commented 6 years ago

Hey I"ve been super busy, but I'll take a look soon. Looks like there are some conflicts/

DerrickRocha commented 6 years ago

Hey Fen. I did some more Democracy Droid refactoring and switched back to the old mvc pattern. We are doing a new project at work using mvc and it's MUCH cleaner and easier to read. We were not doing too much unit testing involving UI, mostly just the business logic, so we decided to ditch the mvp pattern. I also did a little smoke testing with democracy droid and fixed this bug I created by forgetting to download the audio feed. (Good thing you didnt merge the last pull request lol). This was crashing the app when the user would try to download audio. I'll do some smoke testing on various different devices today and make sure I didn't break anything else. Im also seeing some issues with the app thats in production now involving the opening of external apps. For example, when i click the menu item to open the web browser, the calander app will open. Ill try to get that all knocked out today, and ill get back to you asap. Thanks.

On Jan 23, 2018 6:18 AM, "Fenimore" notifications@github.com wrote:

Hey I"ve been super busy, but I'll take a look soon. Looks like there are some conflicts/

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/fenimore/DemocracyDroid/pull/44#issuecomment-359787526, or mute the thread https://github.com/notifications/unsubscribe-auth/AJBF_dg72SiGpMUYRVjXDnDAnvFvYWvVks5tNdw1gaJpZM4Rd5fL .

fenimore commented 6 years ago

Hi Derrik! I'm sorry I never got around to looking into this, I've been super busy this winter, and recently I was on a vacation, far from a computer :smile: -- it sounds lik eyou're making good progress here! And I really will try to compile and do some smoke tests myself :wink:

It would be nice to have unit tests so that way we don't have to manually go through everything to check... there are a quite a few edge cases actually

Part of the reason I still haven't gotten to it is because the changes are quite large, and some of them (the kotlin parts) I don't understand yet. Maybe you could put some high level comments on some of the changes to help guide me through them?

Thanks again for refactoring and generally cleaning up the code :sweat_smile:

fenimore commented 6 years ago

also don't hesitate to comment here and nudge me with a reminder if it falls behind in my list of TODOS :smile:

fenimore commented 6 years ago

I was having trouble understanding the changes you made, on the one hand because I don't know Kotlin, but also because the structure of the app changes dramatically with this PR. I am not opposed to using kotlin, and making these changes, but I cannot merge like this.

Instead is it possible to split up the PR into chewable chunks, like 200 lines at a time, so I can try running it and see what changes etc?

Also, this is a good opportunity to add Unit Tests. With unit tests big changes would be easier/safer to make. I'm looking into adding unit tests, but it's hard and I'm not sure what to do actually :man_shrugging:

fenimore commented 6 years ago

@DerrickRocha

DerrickRocha commented 6 years ago

I'm not sure I'll have the capacity to do that. No worries. All the changes I made are still in the repository that I forked. You can always cherry pick parts of the code from there if you need to. When it comes to testing on the android, I usually just test the networking layer. For example, we could create mock response bodies and make sure the results from a 200 are in the right format for a test. We would also write tests to make sure our network fails right if we have a 400 or 500 error. I also like to set up mock data for all the endpoints that get called, and make sure the parameters from the responses are correct. We would more than likely have to refactor our networking code in a way that's testable, so it might not be worth it. The last app that I did this with had lots of post, get, delete calls to our back end along with custom error handling, so it was a pretty good idea to have unit testing for networking on the clients. It might be a little over kill for this app.

One thing we may want to work on that would benefit the users of the app would be the exoplayer playback. I noticed a bug if I'm streaming video and you get a phone call. When you get a call and a video is playing, the video pauses which is expected. The problem lies once you answer the call. The video player will start playing once you answer the call. This wouldn't be a problem if I wanted to share the daily broadcast with everyone that called me when watching democracy now(Maybe this is a good thing). You may want to create a bug ticket for it if you can reproduce it. I know I might be pretty busy for the next month or two, so I'm not sure I could look at it any time soon.

Thanks

On Sun, May 27, 2018, 8:23 AM Fenimore notifications@github.com wrote:

@DerrickRocha https://github.com/DerrickRocha

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/fenimore/DemocracyDroid/pull/44#issuecomment-392334670, or mute the thread https://github.com/notifications/unsubscribe-auth/AJBF_TMWhPqBQEEMnjhIROmwZhqAV1zrks5t2rb-gaJpZM4Rd5fL .

fenimore commented 6 years ago

I didn't know about this! What version of Android do u have?