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

Empty cards are no longer displayed #48

Closed
wants to merge 5 commits into from
Closed

Empty cards are no longer displayed #48

wants to merge 5 commits into from

Conversation

Ninjaman494
Copy link

Previously, cards with no items in them still go displayed in About Activity. I fixed this by adding checks to see if a card has an item to display before adding it to About Activity.

The method first checks if each card as an item to display, if does then the card is added to the build.
@mikemee
Copy link
Collaborator

mikemee commented Jan 12, 2018

@Ninjaman494 Thanks so much! It looks like there are some PMD errors. Do you mind fixing those so the build passes please?

@Ninjaman494
Copy link
Author

I looked at the errors and I'm not sure what's wrong. Two are about useless parentheses at these two lines:

if((config.buildType != null && !TextUtils.isEmpty(config.appPublisher) && !TextUtils.isEmpty(config.packageName))
                || (!TextUtils.isEmpty(config.companyHtmlPath) && !TextUtils.isEmpty(config.aboutLabelTitle))){

But I think removing the parentheses would change the if statement's meaning.

The other is Possible God class at line 1, which is just the package declaration.
package com.eggheadgames.aboutbox.activity;

I might be missing something though, I don't have much experience with pmd reports, or builds tests at all for that matter.

@mikemee
Copy link
Collaborator

mikemee commented Jan 14, 2018

@Ninjaman494 Thanks for taking a look. I've never seen PMD get parenthesis mixed up. Perhaps it's worthwhile to add an intermediate variable there to simplify the test.

Hmm, "Possible God Class" usually means we've crossed some (arbitrary) line to do with the number of functions, nesting etc.

@Ninjaman494
Copy link
Author

Fixed the parenthesis errors. I'm not sure how to fix the God Class one. I could consolidate the if statements in one place, since they're pretty much identical to the ones that happen in the private build methods, but I think that would just make the code more complex. Any suggestions?

@Ninjaman494 Ninjaman494 changed the title Empty cards are no longer dispalyed Empty cards are no longer displayed Jan 14, 2018
@mikemee
Copy link
Collaborator

mikemee commented Jan 14, 2018

A great improvement! For God Class, generally I just "play with the code" to see what might remove the error. Usually it's a sign that the entire class is too big or too complex and that it should be refactored into two classes. But since it wasn't an error before, and is now, chances are there's something simpler than that possible.

Feel free to see if you can improve it. If I get a chance I will try too, but probably not until late this week.

Thanks!

Each card is now goes through an isEmpty check before being added to the activity.
Removed redundant logic checks in getMaterialAboutList
@mikemee
Copy link
Collaborator

mikemee commented May 15, 2020

Fixed in #54. Closing.

@Ninjaman494 thanks for the inspiration. Without this PR I doubt this would have happened. Sorry it took so long to complete.

@mikemee mikemee closed this May 15, 2020
@Ninjaman494
Copy link
Author

No problem! It's my fault for dropping the ball on this. Life kinda got in the way and I completely forgot about this PR. I'm happy someone else was able pick it up and finish it.

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.

None yet

2 participants