Detekt - Resolve/Suppress All Baseline Warnings - Long methods warnings_ HomePageSettingsDialog, AuthorUseCase, ClicksUseCase, ReferrersUseCase #17446
Conversation
…HomepageSettingsDialog from baseline.xml
|
👋🏾 @ParaskP7 |
There was a problem hiding this comment.
👋 @Hoossayn !
Thank you so much for your contribution to WPAndroid with this PR! 🥇
I have reviewed and tested this PR as per the instructions, along with triggering CI on it via a draft PR, which I just pushed on the main repo, everything works as expected, good job! 🌟
However, and apologies for making it once more a bit difficult for you to merge, but I think you rushed a bit with this PR, let me explain: ✍️
- Warning (
⚠️ ): You included two unrelated changes, one for theSite Settingsscreens and itsHome Pagesettings, which you explicitly included test instruction and I verified that everything works as expect, but also for theStatsscreen and itsReferrers,ClicksandAuthorsstat related cards, which you also didn't included test instructions for.
FYI: This means that my confidence in you testing this change is lower than expected and I would now rather had this split into 2 PRs than just one, one for the Site Settings screen with the Hope Page settings change, and one for the Stats screen with the Stats Cards change.
No matter, I tested it on your behalf and everything seems to be working as expected. But, please, do update the testing instructions. 🙏
- Warnings (
⚠️ ): For the use case related changes you choose to do a refactor that I would recommend against, that is to pass theitemslist as a parameter to your extracted methods (seeitems: MutableList<BlockListItem>). Doing so is dangerous (see pass by reference vs. pass by value in Java), but also makes the code harder to reason about as multiple functions are adding to this list of items.
Instead, I would urge you to consider this alternative instead, the below is for ClicksUseCase only, but applies to all 3 use cases:
override fun buildUiModel(domainModel: ClicksModel, uiState: SelectedClicksGroup): List<BlockListItem> {
val isBlockMode = useCaseMode == BLOCK
return if (domainModel.groups.isEmpty()) {
buildUiModelForNoGroup(isBlockMode)
} else {
buildUiModelForGroups(domainModel, uiState, isBlockMode)
}
}
private fun buildUiModelForNoGroup(
isBlockMode: Boolean
): List<BlockListItem> {
val items = mutableListOf<BlockListItem>()
if (isBlockMode) items.add(Title(R.string.stats_clicks))
items.add(Empty(R.string.stats_no_data_for_period))
return items
}
private fun buildUiModelForGroups(
domainModel: ClicksModel,
uiState: SelectedClicksGroup,
isBlockMode: Boolean
): List<BlockListItem> {
val items = mutableListOf<BlockListItem>()
if (isBlockMode) items.add(Title(R.string.stats_clicks))
val header = Header(R.string.stats_clicks_link_label, R.string.stats_clicks_label)
items.add(header)
domainModel.groups.forEachIndexed { index, group ->
...
}
if (useCaseMode == BLOCK && domainModel.hasMore) {
...
}
return items
}
Is the diff clear to you, and more so, how an alternative like that is a better fit for such a refactor? Let me know if not and I can further elaborate to help you understand. 🤔
-
Suggestion (💡): Consider removing the extra two,
ComplexMethod:HomepageSettingsDialogandComplexMethod:ReferrersUseCasefrom thebaseline.xmlas well. They can be removed as well. 🎉 -
Minor (🔍): With this ad6bb23 change you also removed the SAM constructor. I would recommend you don't mix such refactoring into one commit, exactly like you did with this 1cbf132 commit followed. You could have included both removals of SAM constructors in this 1cbf132 second commit of yours. PS: This tip is for next time.
Let me know about all that above and we would then progress accordingly. 💯
Generated by 🚫 dangerJS |
it didn't skip my mind to test or include the test steps. i wasn't sure if i should include test for it. because up until this time, i've made changes directly on a fragment/activity. going forward i will ask for clarification before you review instead of going mute
Thanks for your suggestion and also i haven't updated the commits in this doc. kindly let me know if i should or go ahead and split this PR further |
|
👋 @Hoossayn ! FYI: You could use the
I see now. 👍 As a general rule, whatever code is being touched, no matter if unit tests are included, it is better to manual test the change as well. For sure, it should be included in the PR description so that the reviewers know were to focus on during their testing. Please don't feel that you need to ask for clarification next time, you could safely assume that we always manual test any of our changes, especially since our codebase in not fully covered with UI tests, but only some E2E tests. Let me know if you still have doubts about that. 🙏
This is awesome that you did your own research, thank you for that, much appreciated! 🙇
Let me take a look at that and I'll get back to you on it. 👍
Cool, let me take a look first and I'll let you know my recommendation with how to proceed forward with this PR. 💯 |
There was a problem hiding this comment.
👋 @Hoossayn !
I did another pass on your latest changes and I recommend you take a closer look at my included comments below, especially the warning (
Then, and because this PR is already getting a bit out of hand, with 2 separate changes in terms of testing the business logic, maybe it is indeed better for you to redo it, a bit more carefully this time, and in isolation to each other, that is:
- The 1st PR should only focus on
HomepageSettingsDialog - The 2nd PR should only focus on these 3
Statsrelated use cases.
FYI: In addition to the above, and fact that the use case related unit tests didn't fail on your changes, especially on the wrongly introduced changes for the ReferrersUseCase, I would highly recommend for you to take this opportunity and add unit tests, on each use case, to try to achieve as high of a coverage as possible first before going forward with any method extraction, that is, main code change. This is so that to verify that your change ends up resulting in the expected outcome.
Wdyt? 🤔
| @@ -98,74 +98,98 @@ class AuthorsUseCase constructor( | |||
| } | |||
|
|
|||
| override fun buildUiModel(domainModel: AuthorsModel, uiState: SelectedAuthor): List<BlockListItem> { | |||
| val isBlockMode = useCaseMode == BLOCK | |||
| return if(domainModel.authors.isEmpty()){ | |||
There was a problem hiding this comment.
Warnings (if/else statement as it missing empty spaces between if and else.
PS: The same apply to the other 2 use cases.
| } | ||
| } | ||
| } | ||
| if (useCaseMode == BLOCK && domainModel.hasMore) { |
There was a problem hiding this comment.
Suggestion (💡): Now that the isBlockMode is extracted and used as a parameter to this function, you could reuse that and replace useCaseMode == BLOCK with it.
PS: Same apply for the ClicksUseCase use case.
| Link( | ||
| text = R.string.stats_insights_view_more, | ||
| navigateAction = create(statsGranularity, this::onViewMoreClicked) | ||
|
|
There was a problem hiding this comment.
Minor (🔍): Consider removing the extra empty line here.
| @@ -111,106 +111,151 @@ class ReferrersUseCase( | |||
| } | |||
|
|
|||
| override fun buildUiModel(domainModel: ReferrersModel, uiState: SelectedGroup): List<BlockListItem> { | |||
| val isViewAllMode = useCaseMode != VIEW_ALL | |||
There was a problem hiding this comment.
Warning (isNotViewAllMode so as for it to not becoming misleading going forward.
PS: The same renaming should be done on the buildUiModelForNoGroup(...) and buildUiModelForGroups(...) levels and everywhere within this class that this naming is used.
| isViewAllMode: Boolean | ||
| ): List<BlockListItem> { | ||
| val items = mutableListOf<BlockListItem>() | ||
| if (isViewAllMode) items.add(Title(R.string.stats_clicks)) |
There was a problem hiding this comment.
Warning (R.string.stats_clicks is most probably a copy-paste mistake from ClicksUseCase and should be corrected to R.string.stats_referrers.
| val items = mutableListOf<BlockListItem>() | ||
| if (isViewAllMode) items.add(Title(R.string.stats_clicks)) | ||
| val header = Header(R.string.stats_referrer_label, R.string.stats_referrer_views_label) | ||
| buildPieChart(domainModel) |
There was a problem hiding this comment.
Blocker (🚫): You will notice that by doing this change you are effectively ignoring adding the returned items to the main lists of such items. As such you should think about how to best progress with this change and why this is blocking. Let me know and I can further help you with that.
FYI: I would recommend the following alternative:
- First change the
buildPieChart(...)to this simpler version of it as you don't need to return a list of items anyway:
private fun buildPieChart(
domainModel: ReferrersModel
): PieChartItem? {
return if (BuildConfig.IS_JETPACK_APP && useCaseMode == BLOCK_DETAIL) {
buildPieChartItem(domainModel)
} else {
null
}
}
- Then check for this item's availability and only add it to the list when it is not null, see below:
buildPieChart(domainModel)?.let { items.add(it) }
PS: The same apply to the showReferrer(...) and showMore(...) functions that you extracted. All 3 of them are actually ignoring the returned list of items.
There was a problem hiding this comment.
@ParaskP7 did you mean showReferrer(...) and showmore(...) should also return PieChartItem
There was a problem hiding this comment.
👋 @Hoossayn !
Actually no, what I meant by that is that the same buildPieChart(...) problem applies to showReferrer(...) and showmore(...) extractions you did, but the solution to those are different, in a sense thy need to return a list or item, specific to their case, not necessary a PieChartItem item.
Does the above clear things for you? 🙏
| } | ||
| } | ||
|
|
||
| if (useCaseMode == VIEW_ALL && domainModel.hasMore) { |
There was a problem hiding this comment.
Blocker (🚫): This seems to be another mistake, where did you get this if (useCaseMode == VIEW_ALL && domainModel.hasMore) from? 🤔
FYI: I am only aware of the below logic, which is anyway already included within the extracted showMore(...) function:
val shouldShowViewMore = itemCount < domainModel.groups.size ||
(useCaseMode == BLOCK && domainModel.hasMore)
| ) | ||
| }) | ||
| items.add(Divider) | ||
| return items |
There was a problem hiding this comment.
Minor (🔍): Consider removing the extra space between the return and items.
@ParaskP7 can you kindly give hint of what to test for in these cases. also i might have to close the draft PR i already have, as i went ahead to make the code changes before writing the test |
|
👋 @Hoossayn !
Sure, so, each of these use-cases has this Does that make sense? 🤔 |
|
👋 @Hoossayn , I hope I find you well! FYI: I am going to close this PR as it hasn't been worked on for a while and as such it is now a bit outdated. PS: Feel free to open another PR at your convenience. |
Parent: #17010
This PR resolves/suppresses all complexity related LongMethod warnings for the HomePageSettingsDialog class, AuthorUseCase class, ClicksUseCase class, and ReferrersUseCase class (see docs here):
4x LongMethod (Resolve: ad6bb23 + 1cbf132 + f887ee0 + 4e0cee3 + ccbb810 + 0431cde + 55311df + 9432e48 )To test:
There is nothing much to test here.
Verifying that all the CI checks are successful should be enough (especially the detekt check).
However, if you really want to be thorough, you could smoke test the WordPress and/or Jetpack apps to verify that everything works as expected on every screen that relates to these changes. Specifically, you could follow the below steps:
To Test
StatsDaystab and confirm that the correct data forReferrers,Clicks, andAuthorsare displayingweeks,monthsandyeartabRegression Notes
Potential unintended areas of impact
can't think of any
What I did to test those areas of impact (or what existing automated tests I relied on)
See
To testsection above.What automated tests I added (or what prevented me from doing so)
N/A
PR submission checklist:
RELEASE-NOTES.txtif necessary.