Idiom to close a cursor

Question

Which of the following two should I be using to make sure that all the cursors are closed?

    Cursor c = getCursor(); 

    if(c!=null && c.getCount()>0){ 
        try{ 
            // read values from cursor 
        }catch(..){} 
        finally{ 
            c.close(); 
        } 
    }//end if

OR

    Cursor c = getCursor(); 
    try{ 
        if(c!=null && c.getCount()>0){ 
            // read values from cursor 
        }//end if 
    }catch(..){
        
    }finally{ 
        c.close(); 
    }

Please advise.

score 13 · Accepted Answer · answered Nov 10 '10 at 15:48

Neither, but the second one was closest.

Option 1 doesn't properly close the Cursor when getCount() == 0
Option 2 leaves the finally block exposed to a null pointer exception

I would use:

Cursor c = getCursor(); 
try { 
    if(c!=null && c.getCount()>0){ 
         // do stuff with the cursor
    }
}
catch(..) {
    //Handle ex
}
finally { 
    if(c != null) {
        c.close(); 
    }
}

... or if you expect the cursor to be null frequently, you could turn it on its head a little bit:

Cursor c = getCursor(); 
if(c != null) {
    try { 
        if(c.getCount()>0) { 
             // do stuff with the cursor
        }
    }
    catch(..) {
        //Handle ex
    }
    finally { 
        c.close(); 
    }
}

i don't think use getCount is a good method. if you user moveToFirst, you can get better performance — wangzhengyi, Sep 13 '14 at 02:03
@wangzhengyi - That's a valid point moveToFirst is more performant AND answers the question of "is there anything in the result set"... but OP used getCount() in their example so I continued it here. — Skylar Sutton, Sep 14 '14 at 22:57

score 3 · Answer 2 · edited May 23 '17 at 12:01

3

This is even better:

does not use c.getCount() - counting might require extra work for the database and is not needed
initialize the cursor before the query block, so failure to create the query is not followed by the finally block

The code:

Cursor c = query(....);
if (c != null) {
   try {        
       while (c.moveToNext()) {  // If empty or after last record it returns false.    
          // process row...
       }
   } 
   finally {
       c.close();
    }
}

Note that c might be null in case of error or empty cursor. See https://stackoverflow.com/a/16108435/952135. I would report null return value in case of empty cursor as a bug, though.

edited May 23 '17 at 12:01

Community

1
1

answered Aug 05 '13 at 07:32

Oliv

10,221
3
55
76

NPE is an issue, `query` could return `null`. – Pin Feb 11 '14 at 15:35
I meant no `c != null` check is necessary in finally. If query returns null, it will fail with NPE, the same as Hemant's snippet would. And I think the `query()` method will never return null. It should create the query, or throw an exception, in which case you don't need to run the finally block. This is normal cleanup pattern: create resource ... try ... do work ... finally ... clean up ... end. If creation fails, it is reported. If work fails or succeeds, finally does the cleanup. If you understand, please remove negative vote. If not, please write. – Oliv Feb 17 '14 at 09:12
1

It can return null and it's specified in the docs (see ContentResolver.query). Also, check this: http://stackoverflow.com/questions/13080540/what-causes-androids-contentresolver-query-to-return-null – Pin Feb 18 '14 at 10:13
Ok, but null return value is not specified. As it is mentioned in the post, it may be due to some error or due to empty cursor. I think this ambiguity is worth reporting as a bug. I'll update my answer. Thanks. – Oliv Feb 24 '14 at 11:43

score 1 · Answer 3 · edited Oct 20 '12 at 07:35

1

Best practice is the one below:

Cursor c = null;    
try {        
   c = query(....);      
   while (c.moveToNext()) {  // If empty or next to last record it returns false.    
      // do stuff..       
   }
} finally {
   if (c != null && !c.isClosed()) {  // If cursor is empty even though should close it.       
   c.close();
   c = null;  // high chances of quick memory release.
}

edited Oct 20 '12 at 07:35

user

86,916
18
197
190

answered May 03 '12 at 07:54

Hemant

11
1

1

I wonder if in this case it is really best practice to set cursor to null, since it is a local variable, the GC should be enough smart to handle it right? – Shyri Jan 19 '17 at 14:31
1

@Shyri Your logic is logical, but without setting the `Cursor` to null, it may not be initialized when entering the `finally` block. You know it is serious when even Android Studio doesn't feel it needs to be explained. – Abandoned Cart Mar 17 '20 at 21:09

score 0 · Answer 4 · answered Nov 10 '10 at 12:18

0

Depends on what you're catching, but I'd say the second one, just in case c.getCount() throws an exception.

Also, some indentation wouldn't go amiss :)

answered Nov 10 '10 at 12:18

MSpeed

8,153
7
49
61

score 0 · Answer 5 · answered Nov 10 '10 at 12:26

0

I'd say the first one, mainly because the second one will try to call c.close() even if c is null. Also, according to the docs, getCount()doesn't throw any exceptions, so there's no need to include it in the try block.

answered Nov 10 '10 at 12:26

Felix

88,392
43
149
167

Do we need to call close() on a cursor which has count of 0? Because in that case for the first idiom, close() will never be called. It assumes that for a cursor having no elements, cursor will never be opened. Is this a valid assumption? – Manish Khot Nov 10 '10 at 12:56
No. A `Cursor` needs to be closed no matter how many items it has. – Felix Nov 10 '10 at 14:41
You can just skip the `c.getCount() > 0` condition. This way, your cursor will always be closed, and your `try` block will simply do nothing. – Felix Nov 10 '10 at 14:44

score -1 · Answer 6 · answered Sep 13 '14 at 01:58

-1

I think my answer is the best one :

    Cursor cursor = null;

    try {
        cursor = rsd.rawQuery(querySql, null);
        if (cursor.moveToFirst()) {
            do {
                // select your need data from database
            } while (cursor.moveToNext());
        }
    } finally {
        if (cursor != null && !cursor.isClosed()) {
            cursor.close();
            cursor = null;
        }
    }

answered Sep 13 '14 at 01:58

wangzhengyi

856
8
8

if this answer is not good, please tell me why you think this model is worse – wangzhengyi Sep 15 '14 at 06:29

score -1 · Answer 7 · answered Aug 29 '16 at 08:39

I think @skylarsutton's is a right answer for the question. However, I want to leave codes for the question (any codes in answers seems to have some flaws). Please consider to use my code.

Cursor c = query(....);
if (c != null) {
   try {        
       //You have to use moveToFirst(). There is no quarantee that a cursor is located at the beginning.
       for(c.moveToFirst();!c.isAfterLast();c.moveToNext()) {  
          // process row...
       }
   } 
   finally {
       c.close();
    }
}

Idiom to close a cursor

7 Answers7

Linked