Picasso's Transformation class to Crop borders from Image

Question

It's a implementation of Transformation from Picasso library looks for the first different pixel color in both sides, in the vertical and horizontal, and crops the bitmap from there.

Here is the Bitmap class doc from Android library.

public class CropMiddleFirstPixelTransformation implements Transformation {
    private int mWidth;
    private int mHeight;


    @Override
    public Bitmap transform(Bitmap source) {
        int width = source.getWidth();
        int height = source.getHeight();

        int[] horizontalMiddleArray = new int[width];
        source.getPixels(horizontalMiddleArray, 0, width, 0, height / 2, width, 1);

        int[] verticalMiddleArray = new int[height];
        source.getPixels(verticalMiddleArray, 0, 1, width / 2, 0, 1, height);

        int left = getFirstNonWhitePosition(horizontalMiddleArray);
        int right = getLastNonWhitePosition(horizontalMiddleArray);

        int top = getFirstNonWhitePosition(verticalMiddleArray);
        int bottom = getLastNonWhitePosition(verticalMiddleArray);

        mWidth = right - left;
        mHeight = bottom - top;


        if (!isNegative(left, right, top, bottom)) {
            return source;
        }

        Bitmap bitmap = Bitmap.createBitmap(source, left, top, mWidth , mHeight);
        source.recycle();
        return bitmap;

    }

    private boolean isNegative(int... values) {
        for (int i : values) {
            if (i < 0) {
                return false;
            }
        }
        return true;

    }

    private int getFirstNonWhitePosition(int[] horizontalMiddleArray) {
        int left = 0;
        for (int i = 0; i < horizontalMiddleArray.length; i++) {
            if (i == 0) {
                left = horizontalMiddleArray[i];
            }
            if (left != horizontalMiddleArray[i]) {
                return i;
            }
        }
        return -1;
    }

    private int getLastNonWhitePosition(int[] horizontalMiddleArray) {
        int right = 0;
        int length = horizontalMiddleArray.length;
        for (int i = length - 1; i > 0; i--) {
            if (i == length - 1) {
                right = horizontalMiddleArray[i];
            }
            if (right != horizontalMiddleArray[i]) {
                return i;
            }
        }
        return -1;
    }


    @Override
    public String key() {
        return "CropMiddleFirstPixelTransformation(width=" + mWidth + ", height=" + mHeight + ")";
    }
}

What could be improved in this implementation?

I think I could eliminate one of this methods: getLastNonWhitePosition, getFirstNonWhitePosition using Java 8 and functional programming, but I cannot do that.

Answer 1

To be fair, I think it is quite readable and I do not know of a way to really improve it using Java 8 constructs. I have a tip or two anyway:

• In the getXNonWhitePosition methods, I'd remove the internal check to assign the first value, assigning it before the loop:

  private int getFirstNonWhitePosition(int[] horizontalMiddleArray) {
      if(horizontalMiddleArray.length == 0) {
          return -1;
      }
      int left = horizontalMiddleArray[0];
      for (int i = 1; i < horizontalMiddleArray.length; i++) {
          if (left != horizontalMiddleArray[i]) {
              return i;
          }
      }
      return -1;
  }
  

• You could implement the isNegative with a Java 8 stream, e.g.:

  private boolean isNegative(final int... values) {
      return IntStream.of(values).allMatch(value -> value > 0);
  }
  

• Why do you save mWidth and mHeight in the fields? Because it seems to me like the transformation has nothing to do with it. These values will change after each transformation application. Using them for the toString feels quite odd.

If I would really want to use Java 8 for the getXNonWhitePosition methods, I think something like the following would do:

private int getFirstNonWhitePosition(final int[] horizontalMiddleArray) {
    if(horizontalMiddleArray.length == 0) {
        return -1;
    }
    final int left = horizontalMiddleArray[0];
    return IntStream.range(1, horizontalMiddleArray.length)
            .filter(index -> horizontalMiddleArray[index] != left)
            .findFirst()
            .orElse(-1);
}

private int getLastNonWhitePosition(final int[] horizontalMiddleArray) {
    if(horizontalMiddleArray.length == 0) {
        return -1;
    }
    final int right = horizontalMiddleArray[horizontalMiddleArray.length - 1];
    return IntStream.rangeClosed(2, horizontalMiddleArray.length)
            .map(index -> horizontalMiddleArray.length - index)
            .filter(index -> horizontalMiddleArray[index] != right)
            .findFirst()
            .orElse(-1);
}

This is not tested, though I think it works. But again, I do not think this is much of an improvement. You could technically extract to a function which receives some value to check against, and some kind of indexing stream, but that feels like over engineering to me.