Java screengrab

Question

This is a short class to take screenshots on mutltiple monitor systems, I made some modifications to the solution at http://stackoverflow.com/questions/10042086/screen-capture-in-java-not-capturing-whole-screen so that the code coped with monitors at different hights and so on. Any and all suggestions welcome :)

/**
 * Code modified from code given in http://whileonefork.blogspot.co.uk/2011/02/java-multi-monitor-screenshots.html following a SE question at 
 * http://stackoverflow.com/questions/10042086/screen-capture-in-java-not-capturing-whole-screen
 */
import java.awt.GraphicsDevice;
import java.awt.GraphicsEnvironment;
import java.awt.Rectangle;
import java.awt.Robot;
import java.awt.image.BufferedImage;
import java.io.File;
import javax.imageio.ImageIO;

class ScreenCapture {

    static int numberOfMinutesToSleepBetweenScreenshots = 4;

    public static void main(String args[]) throws Exception {
        int i = 1000;
        while (true) {
            takeScreenshot("ScreenCapture" + i++);
            try {
                Thread.sleep(60 * numberOfMinutesToSleepBetweenScreenshots * 1000);
            } catch (Exception e) {
                e.printStackTrace();
            }
        }
    }

    public static void takeScreenshot(String filename) throws Exception {
        // Okay so all we have to do here is find the screen with the lowest x,
        // the screen with the lowest y, the screen with the higtest value of X+
        // width and the screen with the highest value of Y+height
        GraphicsEnvironment ge = GraphicsEnvironment.getLocalGraphicsEnvironment();
        GraphicsDevice[] screens = ge.getScreenDevices();

        Rectangle allScreenBounds = new Rectangle();
        int farx = 0;
        int fary = 0;
        for (GraphicsDevice screen : screens) {
            Rectangle screenBounds = screen.getDefaultConfiguration().getBounds();
            //finding the one corner
            if (allScreenBounds.x > screenBounds.x) {
                allScreenBounds.x = screenBounds.x;
            }
            if (allScreenBounds.y > screenBounds.y) {
                allScreenBounds.y = screenBounds.y;
            }
            //finding the other corner
            if (farx < (screenBounds.x + screenBounds.width)) {
                farx = screenBounds.x + screenBounds.width;
            }
            if (fary < (screenBounds.y + screenBounds.height)) {
                fary = screenBounds.y + screenBounds.height;
            }
            allScreenBounds.width = farx - allScreenBounds.x;
            allScreenBounds.height = fary - allScreenBounds.y;
        }
        Robot robot = new Robot();
        BufferedImage screenShot = robot.createScreenCapture(allScreenBounds);
        ImageIO.write(screenShot, "jpg", new File(filename + ".jpg"));
    }
}

EDIT - thank you everybody, upvotes for all!

Answer 1

public static void takeScreenshot(String filename) throws Exception {

Do not catch or throw generic Exceptions whenever possible. Be explicit about the possible problems.

public static void main(String args[]) throws Exception {

That's bad. Your main function should not throw exceptions, but rather handle them gracefully. I assume this is a off-the-shelf template, you should stop doing that (just adding throws Exception and being done with it).

int i = 1000;
while (true) {
    takeScreenshot("ScreenCapture" + i++);

I know it's minor, but variables with names like i, j or arr give me the creeps. This variable might be better named counter.

Answer 2

I understand the need for descriptive variable names, but perhaps numberOfMinutesToSleepBetweenScreenshots is taking it a bit too far. Using something like minsBetweenShots or something like that is nearly as descriptive, and in the context of the other code will be clear enough for any programmer who puts forth any effort to understand the code.

In general though, the code is organized pretty well and I would feel comfortable working with it if I had to.

Answer 3

public static void takeScreenshot(String filename) throws Exception {

This method is quite long and performs several tasks. I would consider moving some parts to separate methods. For example; move all code used for calculating the allScreenBounds variable into a new method:

private Rectangle getAllScreenBounds() {
    ...
}

Your takeScreenshot method would then look like

public static void takeScreenshot(String filename) throws Exception {
    Robot robot = new Robot();
    BufferedImage screenShot = robot.createScreenCapture(getAllScreenBounds());
    ImageIO.write(screenShot, "jpg", new File(filename + ".jpg"));
}

I would also consider creating a method called something like GraphicsDevice[] getScreens() containing

GraphicsEnvironment ge = GraphicsEnvironment.getLocalGraphicsEnvironment();
GraphicsDevice[] screens = ge.getScreenDevices();

Answer 4

I would start by replacing the call to Thread.sleep(...); with a call to TimeUnit.MINUTES.sleep(...); which is much easier to read, also because magic numbers are evil.

You can simplify your for loop by replacing all of the corner logic with a call to #add(Rectangle), e.g.

for (GraphicsDevice screen : screens) {
    Rectangle screenBounds = screen.getDefaultConfiguration().getBounds();
    allScreenBounds.add(screenBounds);
}

You should pay close attention to the documentation though, as there are some "gotchas" in there.

Of course, min and max would work just as well.

Answer 5

From reading this, your code seems all wrong, I could be mistaken.

If you want to create a screenshot for a specific Screen, then you need to create an instance of Robot with that Screen ( public Robot(GraphicsDevice screen) throws AWTException ) and then do the screen-capture.

As for finding the bounds for that screen, have you empirically found that just using screen.getDefaultConfiguration().getBounds(); is not working? It does not make sense that you derive the bounds by getting the extremes from all available screens.