Is using Section as HashMap key a good design?

[yccheok]

I have been using version 1.2.0 for several years.

I tries to start a new project with version 3.1.0

I notice there are some significant design change especially

https://github.com/luizgrp/SectionedRecyclerViewAdapter/blob/master/library/src/main/java/io/github/luizgrp/sectionedrecyclerviewadapter/SectionedRecyclerViewAdapter.java#L36

My question is, is using Section abstract class as HashMap key a good design? What if, the child of Section, override hashCode and equals method?

Thanks.

[luizgrp]

Good point 👍

We could change it to use the generated string for the section as key, but then this method getAdapterForSection(Section) would have to loop through the sections to find the tag before returning the SectionAdapter.

Or we could just not store any SectionAdapter and have them created on the go by the getAdapterForSection methods. In this case we would also have loop through the sections to check if the Section passed as parameter is valid. So not much benefit in relation to the previous option, as it will also have to instantiate a class on top of looping through the sections.

Both options above will and an extra loop through the sections, so I feel like we could compromise and keep the current solution.

What do you think? Do you have any other ideas?

[yccheok]

Hem...

I just feel that, having a class instance, for every Section, to perform a set of "helper function operation", doesn't look very good to me. It just smell redundant, and not efficient.

Usually, for "helper functions", I prefer to design them as public static set of functions.

But, such design would put burden on caller. We would need to have

public static int getPositionInSection(SectionedRecyclerViewAdapter sectionedAdapter, Section section, final int position)

instead

public int getPositionInSection(final int position)

My understanding is that, you wish to move out all "helper functions" implementation out from SectionedRecyclerViewAdapter class ?

Do you want to consider using default function feature of interface?

Something which looks like

interface SectionPositionIdentifier {
    SectionedRecyclerViewAdapter getSectionedRecyclerViewAdapter();

    // All helper functions implementation will go into SectionPositionIdentifier's default functions.
    default int getPositionInSection(final int position) {
        SectionedRecyclerViewAdapter sectionedRecyclerViewAdapter = getSectionedRecyclerViewAdapter();
        // Perform some computation based on sectionedRecyclerViewAdapter.
        return 0;
    }
}

public class SectionedRecyclerViewAdapter implements SectionPositionIdentifier {

    @Override
    public SectionedRecyclerViewAdapter getSectionedRecyclerViewAdapter() {
        return this;
    }        
}

[luizgrp]

I agree with you. They are helper methods and I considered to build them as static methods, but then decided to make it convenient instead.

My understanding is that, you wish to move out all "helper functions" implementation out from SectionedRecyclerViewAdapter class ?

Reasoning is described in this pull request https://github.com/luizgrp/SectionedRecyclerViewAdapter/pull/153

Do you want to consider using default function feature of interface?

That would address items 1 and 3 of the "Pros" described in that pull request, but not items 2 and 4. Also, the idea was to distinguish between an adapter to manipulate sections and another to manipulate items from a specific section.

Let me know your thoughts..

[yccheok]

You are right. The proposed default interface method doesn't address item 2 and 4.

In such case, your current HashMap might be the best available option.

But, I feel maybe we can have the following improvement in the following line?

https://github.com/luizgrp/SectionedRecyclerViewAdapter/blob/master/library/src/main/java/io/github/luizgrp/sectionedrecyclerviewadapter/SectionedRecyclerViewAdapter.java#L224

Check the returned value of

sectionAdapters.put(section, new SectionAdapter(this, section));

It should return null. If not, throw runtime exception as this might imply the caller overrides equals and hashCode methods. Such action will cause 2 different instance of Sections returning same value in equals and hashCode methods.

This is kind of defensive way, to have early detection on problematic implementation.

Thanks.

[luizgrp]

sounds good 👍 let's do it