Add a separate class to manipulate items of a specific Section

[luizgrp]

Use Case Extract all the methods to manipulate items of a single Section into a separate class. This way, SectionedRecyclerViewAdapter would only have methods to add/remove a section, any other methods to manipulate items of a specific Section would be placed in this new class.

Pros:

• reduce the amount of methods in the adapter;
• allow method names to be shorter as they won't need the "InSection", "FromSection" in them;
• make unit tests easier to be built and less verbose;
• avoid overloaded methods: one for section object as parameter, another for section tag as parameter;

Cons: It will make it more verbose: sectionedAdapter.getSectionHandler(section).notifyAllItemsChanged() Whereas the current implementation for the above is: sectionedAdapter.notifyAllItemsChangedInSection(section)

Design

• Add a new class to manipulate a single Section - calling it SectionHandler for now but would like to give it a better name;
• Add an interface SectionNotifier with all the notify methods definitions;
• Implement SectionNotifier in SectionHandler;

How to use Developers would have to call sectionedAdapter.getSectionHandler(sectionTag) or sectionedAdapter.getSectionHandler(section) to access the handler and then call its methods, e.g.sectionedAdapter.getSectionHandler(section).notifyAllItemsChanged().

[luizgrp]

Hi @yccheok, I would like to hear your thoughts about this proposal

[yccheok]

Thanks @luizgrp for asking about my thoughts..

I have few concerns.

1. Previous call sectionedAdapter.notifyAllItemsChangedInSection(section), doesn't have any object instantiation operation involved. With sectionedAdapter.getSectionHandler(section).notifyAllItemsChanged(), a new SectionHandler object instantiation happens every-time. As far as I know, object instantiation is relatively expensive. Possible have performance impact on certain use cases?

To eliminate possible risk, we might consider to provide a utility class/ helper class. Something (Better class naming required) likes

Utils {
    public static void notifyAllItemsChanged(SectionedRecyclerViewAdapter sectionedRecyclerViewAdapter, Section section) {
        sectionedRecyclerViewAdapter.callSuperNotifyItemRangeChanged(getPositionInAdapter(section, 0), section.getContentItemsTotal());
    }
}

2. Why we need an interface SectionNotifier. Is there any value we can gain, by introducing SectionNotifier?

3. Instead of removing methods like notifyAllItemsChangedInSection, should we mark them as @Deprecated?

4. This seems to be huge changes. Do we have enough unit tests coverage?

If you ask my preference as end user point of view, I prefer use style sectionedAdapter.notifyAllItemsChangedInSection(section) over sectionedAdapter.getSectionHandler(section).notifyAllItemsChanged() or over Utils.notifyAllItemsChanged(sectionedAdapter, section). As, the usage is more straightforward.

But, if I stood in your shoes, I can understand why you would like to have such change - Separation of concern.

Thank you. Please kindly let me know, if you have other thoughts.

[codecov-io]

Codecov Report

Merging #153 into 3.0.0 will increase coverage by 0.58%. The diff coverage is 99.09%.

@@            Coverage Diff             @@
##            3.0.0     #153      +/-   ##
==========================================
+ Coverage   88.39%   88.98%   +0.58%     
==========================================
  Files           4        7       +3     
  Lines         543      581      +38     
  Branches       78       78              
==========================================
+ Hits          480      517      +37     
+ Misses         44       39       -5     
- Partials       19       25       +6

Impacted Files	Coverage Δ
.../luizgrp/sectionedrecyclerviewadapter/Section.java	78.4% <100%> (-9.64%)	:arrow_down:
...r/compat/SectionedRecyclerViewAdapterV2Compat.java	100% <100%> (ø)
...onedrecyclerviewadapter/utils/EmptyViewHolder.java	100% <100%> (ø)
...yclerviewadapter/SectionedRecyclerViewAdapter.java	88.34% <100%> (-2.3%)	:arrow_down:
...p/sectionedrecyclerviewadapter/SectionAdapter.java	98.26% <98.26%> (ø)
...sectionedrecyclerviewadapter/StatelessSection.java	52.17% <0%> (-8.7%)	:arrow_down:
... and 1 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 0e0b1cf...b453ea0. Read the comment docs.

[luizgrp]

Hi @yccheok ,

Thanks for your reply. Please see below my comments

1. This is a valid concern. We could also create instances of SectionAdapter (previously called SectionHandler) every time a Section is added to the SectionedRecyclerViewAdapter and keep them in a list. This way when getSectionAdapter is called we could just retrieve it from the list. I would avoid premature optimisation and leave as it is for now until we realise it's impacting performance.

2. I also added the interface SectionPositionIdentifier. The only value is for us maintainers, really. I wanted to break down the SectionedRecyclerViewAdapter because it is too big. After analysing all the custom methods, I figure out we could split them into two groups: "Notifiers" and "PositionIdentifiers". Maybe one advantage is that if SectionAdapter starts to become too big, we could move the implementations to those interfaces to separate classes and use them from SectionAdapter, keeping the class small.

3. We could do that, but then SectionedRecyclerViewAdapter will still be too big.

4. Yes, as you can see on the comment above by @codecov-io , these changes reduced the test coverage by 0.88%. But this is still work in progress, it's missing unit tests for the new getSectionAdapter methods.

I appreciate your comment regarding your preference. In order to facilitate the migration to the new version, I would suggest that we add a SectionedRecyclerViewAdapterCompat that will keep the same calls as the previous version. So if you still want to use the old style you can. But the Compat class will be marked as deprecated and will not support new methods introduced in the main class.

What do you think?

[luizgrp]

Hi @yccheok ,

If you ask my preference as end user point of view, I prefer use style sectionedAdapter.notifyAllItemsChangedInSection(section) over sectionedAdapter.getSectionHandler(section).notifyAllItemsChanged() or over Utils.notifyAllItemsChanged(sectionedAdapter, section). As, the usage is more straightforward.

Class SectionedRecyclerViewAdapterV2Compat was added in order to make the migration easier so you can still call sectionedAdapter.notifyAllItemsChangedInSection(section) if your adapter extend SectionedRecyclerViewAdapterV2Compat instead of SectionedRecyclerViewAdapter.

You can try these changes before the final release adding jitpack to your root build.gradle:

    allprojects {
        repositories {
            ...
            maven { url 'https://jitpack.io' }
        }
    }

And adding the dependency:

    dependencies {
            implementation 'io.github.luizgrp:sectionedrecyclerviewadapter:improvements-SNAPSHOT'
    }

I will work in another PR on avoiding the creation of SectionAdapter every time getAdapterForSection is called.

Merging this one for now.