Making GIFs with Java

I wrote a Java class to make a GIF animation from a list of images. The whole project can be found here.

I am pretty new with GitHub, so I would be very glad if you can give critiques regarding my project structure.

```
package shine.htetaung.giffer;

import java.awt.image.BufferedImage;
import java.io.File;
import java.io.IOException;
import java.util.Iterator;

import javax.imageio.IIOException;
import javax.imageio.IIOImage;
import javax.imageio.ImageIO;
import javax.imageio.ImageTypeSpecifier;
import javax.imageio.ImageWriter;
import javax.imageio.metadata.IIOInvalidTreeException;
import javax.imageio.metadata.IIOMetadata;
import javax.imageio.metadata.IIOMetadataNode;
import javax.imageio.stream.ImageOutputStream;

/*
* Giffer is a simple java class to make my life easier in creating gif images.
*
* Usage :
* There are two methods for creating gif images
* To generate from files, just pass the array of filename Strings to this method
* Giffer.generateFromFiles(String[] filenames, String output, int delay, boolean loop)
*
* Or as an alternative you can use this method which accepts an array of BufferedImage
* Giffer.generateFromBI(BufferedImage[] images, String output, int delay, boolean loop)
*
* output is the name of the output file
* delay is time between frames, accepts hundredth of a time. Yeah it's weird, blame Oracle
* loop is the boolean for whether you want to make the image loopable.
*/

public abstract class Giffer {

// Generate gif from an array of filenames
// Make the gif loopable if loop is true
// Set the delay for each frame according to the delay (ms)
// Use the name given in String output for output file
public static void generateFromFiles(String[] filenames, String output, int delay, boolean loop)
throws IIOException, IOException
{
int length = filenames.length;
BufferedImage[] img_list = new BufferedImage[length];

for (int i = 0; i itr = ImageIO.getImageWritersByF

I agree with Peter Rader that static factories are not a good pattern. For this particular task I would be inclined to use a builder pattern. In particular, I would quite like to have a BufferedImage... images varargs parameter somewhere, and IMO the cleanest way of doing that would be for it to be the sole parameter of a constructor.

generateFromBI opens an output stream, but doesn't seem to close it. The "best practices" way of doing this nowadays would be a try-with-resources statement. I haven't used Java 8, so I can't do better than to point you at the docs.

// Not sure why I need the ImageTypeSpecifier, but it doesn't work without it
    ImageTypeSpecifier img_type = ImageTypeSpecifier.createFromBufferedImageType(BufferedImage.TYPE_INT_ARGB);

Full marks for honest commenting, but you may not have realised that you're making a dangerous assumption. What if the image isn't of TYPE_INT_ARGB? The more robust way of doing this would be to create a fresh IIOMetadata for each image, using new ImageTypeSpecifier(img) to create an appropriate ImageTypeSpecifier for each one.

The disposalMethod is hard-coded to "none". It would be nice to make this configurable, preferably via an enum rather than raw strings, in particular because using "doNotDispose" opens up the possibility of optimising the images to use transparency on areas which don't change.

(And on the subject of exposing nice types rather than cryptic raw data, Java 8 provides Duration, which is nicer than raw centiseconds).

app_node.setUserObject(new byte[]{ 0x1, (byte) (0 & 0xFF), (byte) ((0 >> 8) & 0xFF)});

is a bit odd. It seems to be saying "The structure here is very important and you might want to change it, but I won't document it for you". I would either use magic new byte[] { 1, 0, 0 } or pull out the parameters as new byte[]{ foo, bar, baz } with more descriptive names.

Finally, some of the method names seem slightly misleading to me. get connotes "this object already exists and is stored somewhere", but several of these getXYZ methods are actually creating (or getting, with a creation fallback). I would favour renaming with a create or a findOrCreate prefix as appropriate for each case.

StackExchange Code Review Q#113998, answer score: 4