5
\$\begingroup\$

This Activity has got a RecyclerView that is inflated with an ArrayList that is ca. 400 Elements.

This is what an Element of the ArrayList looks like:

chords.add(new Accordo(new int[]{
            R.drawable.do_maggiore, R.drawable.do_maggiore_alt1, R.drawable.do_maggiore_alt2,
            R.drawable.do_maggiore_alt3, R.drawable.do_maggiore_alt4, R.drawable.do_maggiore_alt5,
            R.drawable.do_maggiore_alt6}, R.string.do_maggiore, "Do, Mi, Sol", R.raw.do_maggiore));

So it has got some images, two Strings and a Sound file.

This whole List is inflated into the RecyclerView at Activity's launch. This causes the Activity to have some lag when I scroll fast through the List.

How can I improve the performance of this class?

code:

public class ChordsListActivity extends AppCompatActivity implements PopupMenu.OnMenuItemClickListener {

ArrayList<Accordo> chords;
ImageButton cerca;
RecyclerView recyclerView;
InterstitialAd chordsListAd;
boolean isPremium;
static final String PREFERENCES = "prefs";

@Override
protected void onCreate(Bundle savedInstanceState) {
    super.onCreate(savedInstanceState);
    setContentView(R.layout.activity_chords_list);

    SharedPreferences settings = getSharedPreferences(PREFERENCES,0);
    isPremium = settings.getBoolean("status", false);


    /** gets the list */
    chords = new ArrayList<>();
    ChordsList list = new ChordsList();
    list.createList();
    chords = list.getList();

    /** inflates the list into the recyclerview */
    recyclerView = (RecyclerView) findViewById(R.id.chords_recycler);
    recyclerView.setLayoutManager(new LinearLayoutManager(ChordsListActivity.this, LinearLayoutManager.VERTICAL, false));
    recyclerView.setHasFixedSize(true);

    final ChordsListAdapter adapter = new ChordsListAdapter (this, chords);
    recyclerView.setAdapter(adapter);


    /** manages ads */
    if (!isPremium) {
        MobileAds.initialize(getApplicationContext(), "ca-app-pub-6723047396589178/2654753246");
        AdView listBanner = (AdView) findViewById(R.id.chords_list_banner);
        AdRequest adRequest = new AdRequest.Builder().build();
        listBanner.loadAd(adRequest);

        /** loads full-screen add */
        chordsListAd = new InterstitialAd(this);
        chordsListAd.setAdUnitId("ca-app-pub-6723047396589178/7447672046");
        requestNewInterstitial();


        chordsListAd.setAdListener(new AdListener() {
            @Override
            public void onAdClosed() {
                requestNewInterstitial();
            }
        });}


    /** manages what happens when a recyclerview item is clicked */
    ItemClickSupport.addTo(recyclerView).setOnItemClickListener(new ItemClickSupport.OnItemClickListener(){
        @Override
        public void onItemClicked(RecyclerView recyclerView, int position, View v) {
            if (!isPremium) {
                if (chordsListAd.isLoaded())
                    chordsListAd.show();
            }

            Intent intent = new Intent(ChordsListActivity.this, ChordActivity.class);

            Bundle bundle = new Bundle();
            bundle.putParcelable("selected", chords.get(position));
            intent.putExtras(bundle);
            startActivity(intent);
        }
    });



    /** reads show_dialog preferences and if false opens the PopUpInfoActivity */
    SharedPreferences sharedPref = PreferenceManager.getDefaultSharedPreferences(this);
    Boolean showDialog = sharedPref.getBoolean("Show_dialog", true);

    Intent legendaIntent = new Intent(ChordsListActivity.this, PopUpInfoActivity.class);

    if(showDialog)
        startActivity(legendaIntent);

    /** manages what happens when search button is pressed */
    cerca = (ImageButton) findViewById(R.id.search);

    cerca.setOnClickListener(new View.OnClickListener() {
        @Override
        public void onClick(View v) {
            Intent intent = new Intent(ChordsListActivity.this, SearchActivity.class);

            Bundle bundle = new Bundle();
            bundle.putParcelableArrayList("chords", chords);
            intent.putExtras(bundle);
            startActivity(intent);
        }
    });}

/** queries server for new interstitial add  */
private void requestNewInterstitial() {
    AdRequest request = new AdRequest.Builder().build();

    chordsListAd.loadAd(request);
}

/** manages what happens in the menu */

public void showMenu(View v) {
    PopupMenu menu = new PopupMenu(this, v);
    menu.setOnMenuItemClickListener(this);
    menu.inflate(R.menu.menu);
    menu.show();
}

@Override
public boolean onMenuItemClick(MenuItem item) {
    switch (item.getItemId()) {
        case R.id.informazioni_menu:
            Intent intent = new Intent(ChordsListActivity.this, Info.class);
            startActivity(intent);
            return true;

        case R.id.legenda_menu:
            Intent legendaIntent = new Intent(ChordsListActivity.this, PopUpInfoActivity.class);
            startActivity(legendaIntent);
            return true;

        case R.id.premium_menu:
            Intent premiumIntent = new Intent(ChordsListActivity.this, Premium.class);
            startActivity(premiumIntent);
            return true;

        case R.id.problema_menu:
            Intent email = new Intent(Intent.ACTION_SENDTO, Uri.fromParts(
                    "mailto", "dancam.dev@gmail.com", null));
            email.putExtra(Intent.EXTRA_SUBJECT, "Chords Problem");
            email.putExtra(Intent.EXTRA_TEXT, true);
            startActivity(Intent.createChooser(email, getResources().getString(R.string.choose_client)));
            return true;

        case R.id.invita:
            Intent shareIntent = new Intent(android.content.Intent.ACTION_SEND);
            shareIntent.setType("text/plain");
            shareIntent.putExtra(Intent.EXTRA_SUBJECT, getResources().getString(R.string.invito_oggetto));
            shareIntent.putExtra(Intent.EXTRA_TEXT, getResources().getString(R.string.invita_messaggio) +
                    " https://play.google.com/store/apps/details?id=" + getApplicationContext().getPackageName());
            startActivity(Intent.createChooser(shareIntent, getResources().getString(R.string.condividi_con)));


        default:
            return false;
    }
}

}

Here is my ChordsListAdapter class

public class ChordsListAdapter extends RecyclerView.Adapter<ChordsListAdapter.MyViewHolder> {
private ArrayList<Accordo> chordsList;
Context c;



public class MyViewHolder extends RecyclerView.ViewHolder {
    public TextView nome; //note
    public ImageView immagine;

    public MyViewHolder(View view) {
        super(view);
        nome = (TextView) view.findViewById(R.id.nome);
        //note = (TextView) view.findViewById(R.id.note);
        immagine = (ImageView) view.findViewById(R.id.immagine_accordo);
    }
}

public ChordsListAdapter(Context c, ArrayList<Accordo> chordsList) {
    this.chordsList = chordsList;
    this.c = c;
}

@Override
public MyViewHolder onCreateViewHolder(ViewGroup parent, int viewType) {
    View itemView = LayoutInflater.from(parent.getContext()).inflate(R.layout.row, parent, false);

    return new MyViewHolder(itemView);
}

@Override
public void onBindViewHolder(MyViewHolder holder, int position) {
    Accordo chord = chordsList.get(position);
    holder.nome.setText(chord.getName());
    //holder.note.setText(chord.getNote());
    holder.immagine.setImageResource(chord.getImage()[0]);
}

@Override
public int getItemCount() {
    return chordsList.size();
}
}

Here is my model Accordo.class (Accordo means Chord)

public class Accordo implements Parcelable {
int[] image;
int name;
String note;
int sound;

public Accordo(int[] image, int name, String note, int sound) {
    this.image = image;
    this.name = name;
    this.note = note;
    this.sound = sound;
}

public String getNote() {
    return note;
}

public void setNote(String note) {
    this.note = note;
}

public int[] getImage() {
    return image;
}

public void setImage(int[] image) {
    this.image = image;
}

public int getName() {
    return name;
}

public void setName(int name) {
    this.name = name;
}

public int getSound() { return sound; }

public void setSound(int sound) { this.sound = sound; }

@Override
public int describeContents() {
    return 0;
}

@Override
public void writeToParcel(Parcel dest, int flags) {
    dest.writeIntArray(this.image);
    dest.writeInt(this.name);
    dest.writeString(this.note);
    dest.writeInt(this.sound);
}

protected Accordo(Parcel in) {
    this.image = in.createIntArray();
    this.name = in.readInt();
    this.note = in.readString();
    this.sound = in.readInt();
}

public static final Creator<Accordo> CREATOR = new Creator<Accordo>() {
    @Override
    public Accordo createFromParcel(Parcel source) {
        return new Accordo(source);
    }

    @Override
    public Accordo[] newArray(int size) {
        return new Accordo[size];
    }
};
}

Here is my chords_list_activity.xml

<?xml version="1.0" encoding="utf-8"?>
<RelativeLayout xmlns:android="http://schemas.android.com/apk/res/android"
xmlns:tools="http://schemas.android.com/tools"
android:layout_width="match_parent"
android:layout_height="match_parent"
xmlns:ads="http://schemas.android.com/apk/res-auto"
android:background="@drawable/background"
android:paddingBottom="@dimen/activity_vertical_margin"
android:paddingLeft="@dimen/activity_horizontal_margin"
android:paddingRight="@dimen/activity_horizontal_margin"
android:paddingTop="@dimen/activity_vertical_margin"
tools:context=".ChordsList.ChordsListActivity">

<RelativeLayout
    android:id="@+id/layout_top"
    android:layout_width="match_parent"
    android:layout_height="wrap_content">

    <ImageView
        android:id="@+id/logo"
        android:layout_width="match_parent"
        android:layout_height="100dp"
        android:scaleType="fitStart"
        android:src="@drawable/icon_verde_acqua_tras"/>

    <ImageButton
        android:id="@+id/menu"
        android:background="@color/trans"
        android:layout_width="50dp"
        android:layout_height="50dp"
        android:src="@drawable/tasto_menu"
        android:scaleType="fitCenter"
        android:layout_alignParentEnd="true"
        android:layout_marginTop="20dp"
        android:onClick="showMenu"/>

    <ImageButton
        android:id="@+id/search"
        android:background="@color/trans"
        android:layout_width="50dp"
        android:layout_height="50dp"
        android:src="@drawable/tasto_ricerca"
        android:scaleType="fitCenter"
        android:layout_toStartOf="@id/menu"
        android:layout_marginTop="20dp" />

</RelativeLayout>

<android.support.v7.widget.RecyclerView
    android:layout_width="wrap_content"
    android:layout_height="wrap_content"
    android:id="@+id/chords_recycler"
    android:layout_below="@+id/layout_top"
    android:layout_alignParentStart="true"
    android:layout_marginTop="25dp" />

<com.google.android.gms.ads.AdView
    android:id="@+id/chords_list_banner"
    android:layout_width="wrap_content"
    android:layout_height="wrap_content"
    android:layout_centerHorizontal="true"
    android:layout_alignParentBottom="true"
    ads:adSize="BANNER"
    ads:adUnitId='@string/chords_list_banner'>
</com.google.android.gms.ads.AdView>
</RelativeLayout>

Please provide suggestions by which I can increase the performance of my App

Thanks a lot in advance!!

\$\endgroup\$
  • 2
    \$\begingroup\$ As we all want to make our code more efficient or improve it in one way or another, try to write a title that summarizes what your code does, not what you want to get out of a review. Please see How to get the best value out of Code Review - Asking Questions for guidance on writing good question titles. \$\endgroup\$ – Mast Sep 18 '16 at 18:23
  • \$\begingroup\$ I have updated my code with comments that should let you understand what every chunk of code does, thanks for the suggestions @Mast \$\endgroup\$ – Daniele Sep 18 '16 at 19:56
  • \$\begingroup\$ I have rolled back the last edit. Please see what you may and may not do after receiving answers. \$\endgroup\$ – Heslacher Sep 21 '16 at 6:43
  • \$\begingroup\$ Can you kindly post your ChordsListAdapter code \$\endgroup\$ – Debu Sep 22 '16 at 8:01
  • \$\begingroup\$ @Debu please have a look at the edited question. I have added the adapter \$\endgroup\$ – Daniele Sep 22 '16 at 8:05
1
+50
\$\begingroup\$

Nitpicks:

Getting these out of the way first:

  • Class bodies are usually indented by one level. I don't see this in the code here :/
  • Using default visibilities is not common. It's considered good practice to hide as much implementation detail as possible, meaning all your fields should be private
  • It's common that constants are put in the first possible place in a class
  • It's recommended to program against interfaces. chords should be declared as a List<Accordo> accordingly (pun somewhat intended)

Less nitpicky formatting stuff:

I had trouble parsing the end of the if (!isPremium) block (as well as the onCreate method). I strongly recommend to put closing curly braces onto a separate line. It's also usually recommended to always place braces on single-instruction if-statements. That helps when extending the code and also serves to help parse the code faster.

Additionally I personally find it strange to have javadoc-style comments inside a method.... on a single line. Just use standard single line comments by beginning the line with //

Finally performance:

GvinciNovelsINC already scratched the surface of the problem in their answer. What you're looking at right now is the problem of slow something that you don't directly need right now. If you could just give this work to something and retrieve it somewhen later ... Unfortunately I'm nut usually developing Android, so I don't exactly know what would be common, but you should look into Future and related classes.

Questioning your premises:

But I'm not quite sure we're solving the right problem here... Unfortunately I am not dead sure on what problem exactly you're solving here, but if you only have chords that you want to play, that's a problem that has been solved over and over in music theory.

At some point you may want to rethink your approach. From my (admittedly basic) understanding of how chords work, you always have a "base" tone, then a third above that (small third for minor, large third for major chords) and finish it off with a fifth.
After that you can (but don't have to) add bells and whistles, like a seventh or a ninth and sometimes (very seldomly) you replace some parts with others (eg. dropping the third for a second).

This means we could try to capture this process into Accordo (which should be named Chord as Phrancis points out in his answer). I'm not even quite sure how I'd do that, but I'm rather sure it should be significantly faster than preloading the chords.

\$\endgroup\$
  • \$\begingroup\$ Thank you I will follow your suggestions and improve my code \$\endgroup\$ – Daniele Sep 22 '16 at 7:36
8
\$\begingroup\$

I don't know much about Android so I will comment on a more general concern with the code.


There is something I find makes your code difficult to read for me: I don't speak Italian. In fact, most programmers don't speak Italian...

comic

Source: CommmitStrip: The story of a coder who doesn't speak English

Java, as most programming languages, is written in English, and so you can be assured that every Java programmer also speaks English to a reasonable extent. By writing your code (names, etc.) in English you make your code maintainable by any Java programmer, without them needing an Italian-English dictionary to help them follow along.

  • R.drawable.do_maggiore would be better as R.drawable.do_major.

  • ImageButton cerca could have a few meanings.. do you mean search or run? After reading further down cerca = (ImageButton) findViewById(R.id.search); it's obviously search, but I had to scroll down the code to make sure.

  • The switch inside your onMenuItemClick also has quite a few Italian names, along with inconsistent naming e.g.: your information menu's Intent is called intent while the other Intent are legendaIntent, premiumIntent, email, shareIntent.

\$\endgroup\$
  • 3
    \$\begingroup\$ You are absolutely right, Thank you for your answer. Beeing a beginner I used to not giving much importance to variable names or the language they are in. But you definitely have a point. I will keep it in mind in my future projects. Thanks for the helpful advice \$\endgroup\$ – Daniele Sep 20 '16 at 12:39

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.