Add a method to CrimeLab to return a position given a UUID


#1

The last thing we do in CrimePagerActivity is set the current item using a for loop:

for(int i = 0;i<mCrimes.size();i++){ if(mCrimes.get(i).getId().equals(crimeId)){ mViewPager.setCurrentItem(i); break; } }

I was wondering, would there be any reason against taking care of this in the CrimeLab class?
Would it be ok to introduce a method in CrimeLab which returns a position in the mCrimeList for a Crime when given an ID?

Say like this (in CrimeLab.java):

public int getPosition(UUID id){ int position = 0; for(int i = 0;i<mCrimes.size();i++){ if(mCrimes.get(i).getId().equals(id)){ position = i; break; } } return position; }

and then in CrimePagerActivity, you could put this:


#2

That does make the code nicer in your Fragments/Activities.

Only downside I can think of is that In a real app, you may be doing some sorting or filtering of that crime List in your Fragment/Activity. If that happens, this position method will not work. So, it just depends on how you plan to use the data.


#3

This seems to be symptomatic relief; the deeper problem is that the CrimeHolder and CrimePagerActivity are communicating in terms of UUIDs when they ought to be communicating in terms of positions. The interface definition (Intent) that used a UUID made sense in the earlier versions of CriminalIntent, when the CrimeActivity was thereby shielded from having to know that each Crime had some position within a larger list – that knowledge was confined to CrimeLab and CrimeListFragment. However, now the CrimePagerActivity is squarely in the business of knowing about the positional nature of Crimes, so it doesn’t make any sense to use the UUID as the means of communication – both of the communicating parties are working in terms of a positional list, so why shouldn’t they communicate in those terms? The following unified diff shows the changes I made to CriminalIntent to effect this redesign of the interface. As you can see, the changes are quite minor aside from getting rid of that loop that searches through the list looking for a particular UUID:

[code]— CriminalIntent/app/src/main/java/com/bignerdranch/android/criminalintent/CrimeListFragment.java 2015-11-24 17:50:25.000000000 -0600
+++ CriminalIntent Positional/app/src/main/java/com/bignerdranch/android/criminalintent/CrimeListFragment.java 2015-11-24 17:58:22.000000000 -0600
@@ -93,7 +93,7 @@

     @Override
     public void onClick(View v) {
  •        Intent intent = CrimePagerActivity.newIntent(getActivity(), mCrime.getId());
    
  •        Intent intent = CrimePagerActivity.newIntent(getActivity(), getAdapterPosition());
           startActivity(intent);
       }
    

diff -ur CriminalIntent/app/src/main/java/com/bignerdranch/android/criminalintent/CrimePagerActivity.java CriminalIntent Positional/app/src/main/java/com/bignerdranch/android/criminalintent/CrimePagerActivity.java
— CriminalIntent/app/src/main/java/com/bignerdranch/android/criminalintent/CrimePagerActivity.java 2015-11-24 17:53:37.000000000 -0600
+++ CriminalIntent Positional/app/src/main/java/com/bignerdranch/android/criminalintent/CrimePagerActivity.java 2015-11-24 17:58:22.000000000 -0600
@@ -17,14 +17,14 @@
*/
public class CrimePagerActivity extends FragmentActivity {

  • public static final String EXTRA_CRIME_ID = “com.bignerdranch.android.criminalintent.crime_id”;
  • public static final String EXTRA_POSITION = “com.bignerdranch.android.criminalintent.position”;

    private ViewPager mViewPager;
    private List mCrimes;

  • public static Intent newIntent(Context packageContext, UUID crimeId) {
  • public static Intent newIntent(Context packageContext, int position) {
    Intent intent = new Intent(packageContext, CrimePagerActivity.class);
  •    intent.putExtra(EXTRA_CRIME_ID, crimeId);
    
  •    intent.putExtra(EXTRA_POSITION, position);
       return intent;
    
    }

@@ -33,7 +33,7 @@
super.onCreate(savedInstanceState);
setContentView(R.layout.activity_crime_pager);

  •    UUID crimeId = (UUID) getIntent().getSerializableExtra(EXTRA_CRIME_ID);
    
  •    int position = getIntent().getIntExtra(EXTRA_POSITION, 0);
    
       mViewPager = (ViewPager) findViewById(R.id.activity_crime_pager_view_pager);
    

@@ -52,11 +52,6 @@
}
});

  •    for (int i = 0; i < mCrimes.size(); i++) {
    
  •        if (mCrimes.get(i).getId().equals(crimeId)) {
    
  •            mViewPager.setCurrentItem(i);
    
  •            break;
    
  •        }
    
  •    }
    
  •    mViewPager.setCurrentItem(position);
    
    }
    }
    [/code]

#4

In Criminal Intent, it is true that the position and the UUID both identify a particular Crime. It’s also true that the code can be a little nicer and more efficient if you drop the UUID and just use the position as an identifier.

That said, I strongly recommend using some kind of identifier for your model objects other than position. Or at least, have some identifier that is stored on the model object itself rather than its container. In our case, the position is tied to the container of our model objects (the ArrayList) and not the model objects themselves. This limits your ability to modify the app in the future.

The part that makes me uneasy about the position is that it’s just where the Crime happens to be. It’s not necessarily a stable value. It can change.

Here are a few situations where the position breaks down (if the position is not stored on the Crime object itself):

  • You want to change how you store the data. If you want to use a HashMap instead, for example. Or you want a server to own the data. Or a database. The position value needs to be explicitly stored somewhere (it currently isn’t).
  • Any case where CrimeLab could return a subset of data. You may want the CrimeLab to return search results or to allow the app to support multiple users.
  • Any case where you save the identifier of a particular crime to a storage mechanism that has a longer lifetime than the in-memory ArrayList. The position in the ArrayList is not a long-term identifier of the Crime (unless the position value is a part of the Crime itself).

Since the identifier is such a fundamental thing that will be used throughout your entire app (very hard to change later), I would encourage you to code defensively with the expectation that your requirements may change in the future.

I hope that helps to explain our reasoning for the UUID. In any real application that you develop, you should have some persistent identifier for your model objects.


#5

Correct me if I’m wrong. If you insist on using the position as the parameter for “viewPager.setCurrentItem(position);”, you can pass in the position variable as an extra when you create the activity CrimePagerActivity in the onClick listener inside CrimeHolder.

the onClick() method inside CrimeHolder:

[code]public void onClick(View v){
int position = getAdapterPosition(); //get the position of the container (viewHolder) that is clicked
Intent intent = CrimePagerActivity.newIntent(getActivity(), position);

        startActivity(intent);
    }[/code]

In CrimePagerActivity:

public static Intent newIntent(Context packageContext, int position){ Intent i = new Intent(packageContext, CrimePagerActivity.class); i.putExtra(EXTRA_POSITION, position); //put position variable as an extra return i; }

In CrimePagerActivity when you want to call the setCurrentItem() method:

int position = getIntent().getIntExtra(EXTRA_POSITION, 0); viewPager.setCurrentItem(position);

Since the position is related to the container but not the model object itself (as stated by cstewart in the above post), this method should be okay if you want to set the current item of the ViewPager with the position of the container. This is because it doesn’t matter what is the UUID of the crime, you only wanted to set the crime inside the container that was clicked as the current item of the viewPager.


#6

As I was reading the explanation in the book to set current position (before reading the actual code) I wrote the following code:

Crime currentCrime = CrimeLab.get(this).getCrime(crimeId);
int currentCrimeIndex = crimes.indexOf(currentCrime);
viewPager.setCurrentItem(currentCrimeIndex);

When I got to the code in the book, I was surprised to see that the search method was repeated again (already available from one of the model class - CrimeLab.java). Is there good reason for the duplicated code?

I have another question:

When we instantiate the singleton CrimeLab object, we would create a list of crime objects via an ArrayList at a memory location in the heap space. When we initialize variables in the fragment using the CrimeLab.get(context).getCrimes() aren’t we passing the pointer value and hence pointing to the same underlying data (ArrayList’s contents)? If yes, how would filtering or sorting the list make it inconsistent, i.e., wouldn’t the fragment/activity and CrimeLab be looking at the same data?


#7

For the first question: what code is repeated?

For the second question: You are right that everything would be modifying the same underlying data. In a larger app, you could have multiple screens that show data from that same dataset. When this is the case, you would want the data to remain the same as the user navigates through the app.

In some programming books, you’ll even see examples where you would make a copy of the array and all of the data and only send out that copy from the CrimeLab so that no other classes can modify the data. I think this is a good idea because you want the CrimeLab to be the one and only source of truth for your crimes. Reducing the side effects of other classes modifying the data is only going to make your app more resistant to issues.


#8

CodePagerActivity.java

for (int i = 0; i < crimes.size(); i++) {
        if (crimes.get(i).getId().equals(crimeId)) {
            viewPager.setCurrentItem(i);
            break;
        }
    }

CrimeLab.java

public Crime getCrime(UUID id) {
    for (Crime crime : crimes) {
        if (crime.getId().equals(id)) return crime;
    }
    return null;
}

The above code snippets from the two classes does more or less the same. So, instead of writing a new one in CodePagerActivity.java I used the one from the CrimeLab.java.

As for the answer to my second question, if I understand correctly, having one source of truth is always a good idea :bulb: because this way we can maintain data integrity and as a result imbue the app with resistance to issues pertaining to data inconsistencies. So, if possible having copies of the data set in classes besides the central repository to prevent these classes from modifying the data, is a good design philosophy :ok_hand::+1: