2
\$\begingroup\$

Writing a code to return the last 3 items in a list, if the list has 3 or more elements, otherwise return all list items.

But the code looks really bad. What is a better way to write the same code?

   public void test_arrayListItems() {
        ArrayList<String> arrayList = new ArrayList<>();
        arrayList.add("item1");
        arrayList.add("item2");
        arrayList.add("item3");
        arrayList.add("item4");
        arrayList.add("item5");
        arrayList.add("item6");
        arrayList.add("item7");
        arrayList.add("item8");

        String items = "";
        int j = 0;
        int k = 0;

        if (arrayList.size() <= 2) {
            j = arrayList.size() - 1;
        } else if (arrayList.size() > 2) {
            j = arrayList.size() - 1;
            k = arrayList.size() - 3;
        }
        for (int i = k; i <= j; i++) {
            items = items + "- " + arrayList.get(i) + " ";
        }
        logger.info(items);
    }

thanks

\$\endgroup\$
2
\$\begingroup\$

The problem is a succession of two tasks : first you have the identify the initial index of the loop and iterate over the elements of your list distinguishing between a list having less than 3 elements or plus than 3 elements. For this purpose you can use a ternary operator and implement your loop like the code below:

List<String> arrayList; //previously defined in your code
int size = arrayList.size();
int k = size >= 3 ? size - 3 : 0;
for (int i = k; i < size; ++i) {
     //body defined later in my answer            
}

The second task is about logging elements of your list separated by the string " - ", for this task you can use the class StringJoiner in the body of your loop like the code below:

int size = arrayList.size();
int k = size >= 3 ? size - 3 : 0;
StringJoiner sj = new StringJoiner(" - ");
for (int i = k; i < size; ++i) {
    sj.add(arrayList.get(i));
}
String items = sj.toString();
logger.info(items);
\$\endgroup\$
  • \$\begingroup\$ First time seeing StringJoiner in action, thanks \$\endgroup\$ – Tlink Oct 16 at 15:18
5
\$\begingroup\$
  1. Is a best practice to define a variable as type of the interface the instance implement it(see OOP principles)
  2. Just a little bit out of scope, you can declare your arrayList inline, like in the example I wrote.
  3. Try to use available features of List(in this example) like subList rather than cycle(what if you have thousands of elements?)
  4. toString on a list will return a string with those list, you don't have to concatenate them
  5. it is a bad practice to concatenate strings in cycles because create new strings every time you add + " "

See an alternative solution below:

import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;

public class MainTest {
    public static void main(String[] args) {
        final int wantedSize = 3;
        final List<String> arrayList = new ArrayList<>(Arrays.asList("item1", "item2", "item3", "item4"));
        final int listSize = arrayList.size();
        final List<String> result = (listSize >= wantedSize) ? arrayList.subList(listSize-wantedSize, listSize): arrayList;
        System.out.println(result.toString());
    }
}
\$\endgroup\$
  • \$\begingroup\$ thank you so much, I wasn't able to up vote because of my low reputation, great points made \$\endgroup\$ – Tlink Oct 16 at 15:17

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.