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.

Follow on from this question

This is a follow on question from one I asked a few days ago. I am making (or rather have made) a recursive method that takes a list of ArrayList objects as its parameters. There can be any number of them, but let's pretend there are five. They are numbered from 1 to 5, so the Array contains: [1 2 3 4 5].

The Array is then passed through a recursive method which returns only the numbers at odd indices in the Array, i.e. [2 4] (as they are at indices 1 and 3).

This is the code for the recursive method:

public static ArrayList<Integer> oddList (ArrayList<Integer> tList) {

    ArrayList<Integer> oddList = ListMethods.deepClone(tList);
    int tempStorage = -1;

    if (oddList.size() <= 0)
        return oddList;
    else 
    {                                                               
        if (oddList.size()%2==0)                                    
            tempStorage = oddList.remove(oddList.size()-1); 

        oddList.remove(oddList.size()-1);                           
        oddList = ListMethods.oddList(oddList);                 

        if (tempStorage >= 0)
            oddList.add(tempStorage);                               
    }                                                               
    return oddList;                                                 
}

There are two things I am curious about:

  • Firstly, is there an alternative to that if statement at the very end, to check that tempStorage >= 0.
  • Secondly, and probably more importantly, should I care? In other words, is what I've done there a cheap, easy fix, or is that common coding practice? It just seems strange having an initialised variable that will always be either changed or ignored.

Any feedback would be fantastic, thanks (and to the guys who posted on my previous post, I'm still reviewing all the information - not allowed to use most of it in this assignment, but thanks anyway!)

share|improve this question

1 Answer 1

up vote 3 down vote accepted

I already explained in my other post, not to use this tempStoragevariable in this way. At least (compared to the other version) you do not change the meaning of it in this version.
If you rename it to something like removedItemFromEvenIndex it could be fine. If and really if the requirement is that all values inside the lists are >= 0.

If all values are allowed, I would suggest to stick to the solution with the boolean I suggested in the other post. Because if all values are allowed, You can not find any "check" value to see if you removed or something or not.

From where comes this problem? You try to do 2 things together with the tempStorage:

  • Transport the removed value
  • Status flag if you removed something or not

You can not address both things with the same Integer, because you already need everything from your Integer for the first part.

Rest is the same as for the other question.


To handle this one, same approach as other question:

What we want to do here? Add every second element to the new list, starting with the second.

So a simplified description could be:

function oddList(list)
  if list is empty or has only 1 element
    return empty list
  return new list(second element of list, oddList(all elements from the third to the end))

translate to algorithm in Java:

public static List<Integer> oddList(final List<Integer> list) {
    if (list.size() <= 1)
        return new ArrayList<>();

    final List<Integer> newList = new ArrayList<>(Arrays.asList(list.get(1)));
    newList.addAll(oddList(list.subList(2, list.size())));
    return newList;
}

If we take into account the other question, this could be a clever way (you should always try to reuse existing and working solutions):

public static List<Integer> oddList(final List<Integer> list) {
    final List<Integer> newList = new ArrayList<>(list); //we create a copy, otherwise list is modified for the caller
    newList.remove(0); //all indices shifted one to the left
    return even(newList);
}
share|improve this answer
    
Thanks for this. You've already put a lot of effort into helping me on both questions and I appreciate it. I guess I've been trying to fix the problem whilst changing the bare minimum, because you never know how fussy your lecturers are going to be in marking! –  Andrew Martin Feb 15 '13 at 23:48
    
Meant to add that the reason why I posted this question on the first place was basically because I was hesistant (and still am) about using .addAll and subLists (which I've never used before) simply because I;m worried I'll lose marks due to fussy lecturers not being happy that I've not done exactly what they've requested. However, those techniques look really interesting and I'll look them up in the Java API and if I don't use them here, store them for future reference. –  Andrew Martin Feb 15 '13 at 23:55
    
One last thing (sorry!) - your line addAll(oddList(list.subList(2, list.size()))); Just to ensure I understand propertly, addAll is used to add a Collection. In this case the collection is the entire ArrayList, and sublist is used to send entire array from second element to the end, thus recursively getting smaller each time it is called? –  Andrew Martin Feb 16 '13 at 0:11
    
@AndrewMartin: nearly correct. We add all the results from the method call to the list. And the method is called with the sublist from third until last element (not from second. index 2 is third element) . See the simplified description. And asking questions is completely fine, you do not need to apologize. –  tb- Feb 16 '13 at 1:38

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.