copying files in android with input/output stream: the good and the bad way

Question

i have made this two routines to copy files using inputstream and outpustream. they are quite the same however the second one rise ArrayIndexOutOfBoundsException while the first one works flawlessly and i don't know why:

    public void CopyStream(long size, InputStream is, OutputStream os) {
        final int buffer_size = 4096;
        byte[] bytes = new byte[buffer_size];
        try {
            int count,prog=0;
            while ((count = is.read(bytes)) != -1) {
                os.write(bytes, 0, count); //write buffer
                prog = prog + count;
                publishProgress(((long) prog) * 100 / size);
            }
            os.flush();
            is.close();
            os.close();
        } catch (Exception ex) {
            Log.e(TAG,"CS "+ex);
        }
    }

as you may guess the routine is called inside an AsyncTask, therefore the publishProgresss

    public void CopyStream(long size, InputStream is, OutputStream os) {
        final int buffer_size = 4096;
        try {
            byte[] bytes = new byte[buffer_size];
            for (int count=0,prog=0;count!=-1;) {
                count = is.read(bytes);
                os.write(bytes, 0, count);
                prog=prog+count;
                publishProgress(((long) prog)*100/size);
            }
            os.flush();
            is.close();
            os.close();
        } catch (Exception ex) {
            Log.e(TAG,"CS "+ex);
        }
    }

Does anyone know why the while works but the for no ? what am i missing?

Fireback

a short debugging session would show you that you call os.write(bytes, 0, count); with count == -1 – Pyrology

Answer 1

The problem lies in your for loop checking the condition after the first run through. Basically the error occurs when it has read fine the last loop but on the next loop the is.read call returns -1. Afterwards you try to call os.write(bytes,0,-1); -1 is an invalid index. The solution would be:

public void CopyStream(long size, InputStream is, OutputStream os) {
        final int buffer_size = 4096;
        try {
            byte[] bytes = new byte[buffer_size];
            for (int count=0,prog=0;count!=-1;) {
                count = is.read(bytes);
                if(count != -1) {
                  os.write(bytes, 0, count);
                  prog=prog+count;
                  publishProgress(((long) prog)*100/size);
                }
            }
            os.flush();
            is.close();
            os.close();
        } catch (Exception ex) {
            Log.e(TAG,"CS "+ex);
        }
    }

But it is much more readable as the while loop so I would stick with that. For loops should be used either when you know the quantity of times to loop or as a for each where you loop through each individual item of a collection.

Thacher

Answer 2

For loop stop condition is checked before calling is.read(). This allow situation when you try to read bytes, get result in -1 value and try to continue executing for loop code. While stops immediately after is.read() returns -1

Try following:

int count = is.read(bytes);
for (prog=0;count!=-1;) {
     os.write(bytes, 0, count);
     prog=prog+count;
     publishProgress(((long) prog)*100/size);
     count = is.read(bytes);
}

Terbecki

a variation would be to write the for : for (prog=0, count = is.read(bytes);count != -1; count = is.read(bytes)) { for worst readability. – Pyrology

Answer 3

private static final int BASE_BUFFER_SIZE = 4096;

public static void copyFile(InputStream inputStream, OutputStream outputStream)
        throws IOException {
    byte[] bytes = new byte[BASE_BUFFER_SIZE];
    int count;

    while ((count = inputStream.read(bytes)) != -1){
        outputStream.write(bytes, 0, count);
    }

    close(inputStream);
    close(outputStream);
}

public static void close(@Nullable OutputStream stream) {
    if (stream != null) {
        try {
            stream.flush();
        } catch (IOException ignored) {
        }
        try {
            stream.close();
        } catch (IOException ignored) {
        }
    }
}

public static void close(@Nullable InputStream stream) {
    if (stream != null) {
        try {
            stream.close();
        } catch (IOException ignored) {
        }
    }
}

Beckwith