current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 4 down vote favorite

I am developing a custom grid view with different columns.

EDIT: Here View Possibilities Mockup

View Possibilities Mockup

Here is the full demo. You can directly use it, with Internet uses permission.

This works perfectly but I want to optimize it.

MainActivity.java

package com.example.rtrt;

import java.util.ArrayList;

import com.squareup.picasso.Picasso;

import android.os.Bundle;
import android.app.Activity;
import android.util.DisplayMetrics;
import android.view.LayoutInflater;
import android.view.Menu;
import android.view.View;
import android.widget.ImageView;
import android.widget.LinearLayout;

public class MainActivity extends Activity {

    int width = 0;
    int position = 0;
    ArrayList<String> arr_string = new ArrayList<String>();

    @Override
    protected void onCreate(Bundle savedInstanceState) {
        super.onCreate(savedInstanceState);
        setContentView(R.layout.activity_main);
        LinearLayout girdviewcustom = (LinearLayout) findViewById(R.id.girdviewcustom);
        DisplayMetrics displaymetrics = new DisplayMetrics();
        getWindowManager().getDefaultDisplay().getMetrics(displaymetrics);

        width = displaymetrics.widthPixels;

        arr_string
                .add("http://www.bluesodapromo.com/static/DesignCenter/Image/dc_images/clipart/nature/nature_1.png");
        arr_string
                .add("http://www.bluesodapromo.com/static/DesignCenter/Image/dc_images/clipart/nature/nature_1.png");
        arr_string
                .add("http://www.bluesodapromo.com/static/DesignCenter/Image/dc_images/clipart/nature/nature_1.png");
        arr_string
                .add("http://www.bluesodapromo.com/static/DesignCenter/Image/dc_images/clipart/nature/nature_1.png");
        arr_string
                .add("http://www.bluesodapromo.com/static/DesignCenter/Image/dc_images/clipart/nature/nature_1.png");
        arr_string
                .add("http://www.bluesodapromo.com/static/DesignCenter/Image/dc_images/clipart/nature/nature_1.png");
        arr_string
                .add("http://www.bluesodapromo.com/static/DesignCenter/Image/dc_images/clipart/nature/nature_1.png");
        arr_string
                .add("http://www.bluesodapromo.com/static/DesignCenter/Image/dc_images/clipart/nature/nature_1.png");
        arr_string
                .add("http://www.bluesodapromo.com/static/DesignCenter/Image/dc_images/clipart/nature/nature_1.png");
        arr_string
                .add("http://www.bluesodapromo.com/static/DesignCenter/Image/dc_images/clipart/nature/nature_1.png");
        arr_string
        .add("http://www.bluesodapromo.com/static/DesignCenter/Image/dc_images/clipart/nature/nature_1.png");

        int size = arr_string.size();

        if (size == 1)
            girdviewcustom.addView(getViewone());
        else if (size == 2)
            girdviewcustom.addView(getViewTwo());
        else if (size == 3)
            girdviewcustom.addView(getViewThree());
        else if (size == 4) {
            girdviewcustom.addView(getViewTwo());
            girdviewcustom.addView(getViewTwo());
        } else if (size == 5) {
            girdviewcustom.addView(getViewTwo());
            girdviewcustom.addView(getViewThree());
        } else if (size == 6) {
            girdviewcustom.addView(getViewThree());
            girdviewcustom.addView(getViewThree());
        } else if (size == 7) {
            girdviewcustom.addView(getViewTwo());
            girdviewcustom.addView(getViewTwo());
            girdviewcustom.addView(getViewThree());
        } else if (size == 8) {
            girdviewcustom.addView(getViewTwo());
            girdviewcustom.addView(getViewThree());
            girdviewcustom.addView(getViewThree());
        } else if (size == 9) {
            girdviewcustom.addView(getViewThree());
            girdviewcustom.addView(getViewThree());
            girdviewcustom.addView(getViewThree());
        } else {
            int mod = (size % 3);
            if (mod != 0)
                mod = 1;
            for (int i = 0; i < ((size / 3) + (mod)); i++) {

                girdviewcustom.addView(getViewThree());
            }
        }

    }

    @Override
    public boolean onCreateOptionsMenu(Menu menu) {
        getMenuInflater().inflate(R.menu.main, menu);
        return true;
    }

    public View getViewone() {
        LayoutInflater inflater = getLayoutInflater();
        View myView = inflater.inflate(R.layout.layout_one, null);
        ImageView a11 = (ImageView) myView.findViewById(R.id.a11);
        Picasso.with(MainActivity.this).load(arr_string.get(position))
                .resize(width, width).into(a11);
        position++;
        return myView;

    }

    public View getViewTwo() {
        LayoutInflater inflater = getLayoutInflater();
        View myView = inflater.inflate(R.layout.layout_two, null);
        ImageView a21 = (ImageView) myView.findViewById(R.id.a21);
        ImageView a22 = (ImageView) myView.findViewById(R.id.a22);
        Picasso.with(MainActivity.this).load(arr_string.get(position))
                .resize(width / 2, width / 2).into(a21);
        position++;
        Picasso.with(MainActivity.this).load(arr_string.get(position))
                .resize(width / 2, width / 2).into(a22);
        position++;
        return myView;

    }

    public View getViewThree() {
        LayoutInflater inflater = getLayoutInflater();
        View myView = inflater.inflate(R.layout.layout_three, null);
        ImageView a31 = (ImageView) myView.findViewById(R.id.a31);
        ImageView a32 = (ImageView) myView.findViewById(R.id.a32);
        ImageView a33 = (ImageView) myView.findViewById(R.id.a33);
        Picasso.with(MainActivity.this).load(arr_string.get(position))
                .resize(width / 3, width / 3).into(a31);
        position++;
        if (arr_string.size() > position) {
            Picasso.with(MainActivity.this).load(arr_string.get(position))
                    .resize(width / 3, width / 3).into(a32);
            position++;
            if (arr_string.size() > position) {
                Picasso.with(MainActivity.this).load(arr_string.get(position))
                        .resize(width / 3, width / 3).into(a33);
                position++;
            }
        }
        return myView;

    }

}

activity_main.xml

<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"
    tools:context=".MainActivity" >

    <LinearLayout
        android:id="@+id/girdviewcustom"
        android:layout_width="match_parent"
        android:layout_height="wrap_content"
        android:orientation="vertical" >
    </LinearLayout>

</RelativeLayout>

layout_one.xml

<?xml version="1.0" encoding="utf-8"?>
<LinearLayout xmlns:android="http://schemas.android.com/apk/res/android"
    android:layout_width="match_parent"
    android:layout_height="match_parent"
    android:orientation="vertical" >

    <LinearLayout
        android:layout_width="match_parent"
        android:layout_height="wrap_content"
        android:orientation="horizontal" >

        <ImageView
            android:id="@+id/a11"
            android:layout_width="match_parent"
            android:layout_height="wrap_content"
            android:layout_weight="1"
            android:scaleType="centerCrop" />
    </LinearLayout>

</LinearLayout>

layout_two.xml

<?xml version="1.0" encoding="utf-8"?>
<LinearLayout xmlns:android="http://schemas.android.com/apk/res/android"
    android:layout_width="match_parent"
    android:layout_height="match_parent"
    android:orientation="vertical" >

    <LinearLayout
        android:layout_width="match_parent"
        android:layout_height="wrap_content"
        android:orientation="horizontal" >

        <ImageView
            android:id="@+id/a21"
            android:layout_width="match_parent"
            android:layout_height="wrap_content"
            android:layout_weight="1"
            android:scaleType="centerCrop" />

        <ImageView
            android:id="@+id/a22"
            android:layout_width="match_parent"
            android:layout_height="wrap_content"
            android:layout_weight="1"
            android:scaleType="centerCrop" />
    </LinearLayout>

</LinearLayout>

layout_three.xml

<?xml version="1.0" encoding="utf-8"?>
<LinearLayout xmlns:android="http://schemas.android.com/apk/res/android"
    android:layout_width="match_parent"
    android:layout_height="match_parent"
    android:orientation="vertical" >

    <LinearLayout
        android:layout_width="match_parent"
        android:layout_height="wrap_content"
        android:orientation="horizontal" >

        <ImageView
            android:id="@+id/a31"
            android:layout_width="match_parent"
            android:layout_height="wrap_content"
            android:layout_weight="1"
            android:scaleType="centerCrop" />

        <ImageView
            android:id="@+id/a32"
            android:layout_width="match_parent"
            android:layout_height="wrap_content"
            android:layout_weight="1"
            android:scaleType="centerCrop" />

        <ImageView
            android:id="@+id/a33"
            android:layout_width="match_parent"
            android:layout_height="wrap_content"
            android:layout_weight="1"
            android:scaleType="centerCrop" />
    </LinearLayout>

</LinearLayout>
share|improve this question
    
While it is nice to see you incorporating my review in your code, I would like you to refrain from editing parts of the code, that I talked about. We call this answer invalidation. More information in What you can and cannot do after receiving answers. In accordance to the rules defined there I rolled back your edit. I bet you will get another answer on this question in the next 30 hours. Have patience ;) –  Vogel612 Jul 11 at 11:10
    
Ok Thank you... –  Dedaniya hirenkumar Jul 11 at 11:19
2  
"You can directly use it, with Internet uses permission". We don't have the Picasso class, whatever that is. And why is Internet permissions that important for this code? –  Simon André Forsberg Jul 11 at 11:20
    
@Dedaniyahirenkumar: We are allowed to use it anyway. –  Jamal♦ Jul 11 at 21:40

2 Answers 2

up vote 4 down vote accepted

Variables

int width = 0;
int position = 0;
ArrayList<String> arr_string = new ArrayList<String>();

These variables should be private final and the ArrayList should be declared as List to allow easier switch to another List implementation.

private final int width = 0;
private final int position = 0;
private final List<String> arr_string = new ArrayList<String>();

XML

Considering the pattern of your XML files, you might want to create the layouts dynamically, with code, instead of using XML files.

I don't really see why you're wrapping all your inner LinearLayouts inside an outer LinearLayout in your layout_one-three.xml I think that you can remove

<LinearLayout xmlns:android="http://schemas.android.com/apk/res/android"
    android:layout_width="match_parent"
    android:layout_height="match_parent"
    android:orientation="vertical" >

in those files without any difference in behavior.

I am not a big fan of your naming scheme for your android IDs, such as android:id="@+id/a22".

If-else-if-else-if-else...

Your if-else-if-else-if-else-if can be switched to a switch (size) statement which would make it a bit more clear what's going on there. Also, it is very easy to miss that the first if's don't use braces, which leads us to the common suggestion always use braces. If you switch to a switch, you won't use braces but break; instead.

I believe this part of your code can be simplified a lot by using some mathematics instead of hardcoding the system. I haven't found out the exact solution yet though. But I do believe that the if (size == 9) { specific case is useless as that is exactly what will happen with your last else-block, which btw can be a bit simplified:

int mod = (size % 3 == 0 ? 0 : 1);
for (int i = 0; i < size / 3 + mod; i++) {
    girdviewcustom.addView(getViewThree());
}

Edit:

Instead of for your whole sequence of if-else:

public static int[] rows(int i) {
    final int firstRow;
    if (i == 1) {
        firstRow = 1;
    }
    else firstRow = i % 3 == 0 ? 3 : 2;
    final int secondRow;
    if (i <= 3) {
        secondRow = 0;
    }
    else secondRow = i % 3 == 1 ? 2 : 3;
    final int thirdRow = i >= 7 ? 3 : 0;

    return new int[]{ firstRow, secondRow, thirdRow };
}

private void makeRow(int columns) {
    switch (columns) {
       case 1:
           girdviewcustom.addView(getViewOne());
           break;
       case 2:
           girdviewcustom.addView(getViewTwo());
           break;
       case 3:
           girdviewcustom.addView(getViewThree());
           break;
       default: throw new IllegalArgumentException("Invalid count: " + columns);
    }
}

And then the code in your activity:

int size = arr_string.size();
int[] rows = rows(size);
for (int i : rows) {
    if (i > 0) {
        makeRow(i);
    }
}
share|improve this answer
    
Thank you for your response and I put mockup screen for understanding –  Dedaniya hirenkumar Jul 11 at 13:19
    
@Dedaniyahirenkumar Thanks for the mockup screen, that helped a lot. See the edit I made. –  Simon André Forsberg Jul 11 at 15:52
    
Thanks for your optimized code. Its really helpful for me. Thanks again. –  Dedaniya hirenkumar Jul 12 at 5:42
up vote 3 down vote 
    arr_string
            .add("http://www.bluesodapromo.com/static/DesignCenter/Image/dc_images/clipart/nature/nature_1.png");
    arr_string
            .add("http://www.bluesodapromo.com/static/DesignCenter/Image/dc_images/clipart/nature/nature_1.png");
    arr_string
            .add("http://www.bluesodapromo.com/static/DesignCenter/Image/dc_images/clipart/nature/nature_1.png");
    arr_string
            .add("http://www.bluesodapromo.com/static/DesignCenter/Image/dc_images/clipart/nature/nature_1.png");
    arr_string
            .add("http://www.bluesodapromo.com/static/DesignCenter/Image/dc_images/clipart/nature/nature_1.png");
    arr_string
            .add("http://www.bluesodapromo.com/static/DesignCenter/Image/dc_images/clipart/nature/nature_1.png");
    arr_string
            .add("http://www.bluesodapromo.com/static/DesignCenter/Image/dc_images/clipart/nature/nature_1.png");
    arr_string
            .add("http://www.bluesodapromo.com/static/DesignCenter/Image/dc_images/clipart/nature/nature_1.png");
    arr_string
            .add("http://www.bluesodapromo.com/static/DesignCenter/Image/dc_images/clipart/nature/nature_1.png");
    arr_string
            .add("http://www.bluesodapromo.com/static/DesignCenter/Image/dc_images/clipart/nature/nature_1.png");
    arr_string
    .add("http://www.bluesodapromo.com/static/DesignCenter/Image/dc_images/clipart/nature/nature_1.png");

Just three little things that strike me in this code:

Repetition of doom / copy pasta

Whenever you copy/paste, in your head there should be a signal lamp flashing. Don't copy paste. There are some moments when you can ignore it, but all else equal you should extract a method / change to a loop.

A code that does the exact same thing:

for(int i = 0; i < 11; i++) {
    arr_string.add("http://" /* */);
}

Indentation

The last .add() is placed oddly.
all others are subdented, but that one is on the outer level...

Naming

I don't know about android, but usually the java-convention for fields, variables and methods is camelCase, your arr_string uses snake_case. And the name could definitely be better in itself.

share|improve this answer
1  
That I know, It just for demo purpose. I just want help in arranging view with optimized code. All other things which you mention that I follow. Thanks for that. If you can help me to optimize code then please tell me. –  Dedaniya hirenkumar Jul 11 at 9:59
    
Please See my edit –  Dedaniya hirenkumar Jul 11 at 10:35

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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