1
\$\begingroup\$

I've a piece of code down below that I'm really unsure about. Basically an user can type the url (urlStr param of the method) he wants. From this url I need to :

  • get the data as Base64 so I can display it in an html img src tag.
  • Being able to crop the image afterward and save the image as a file

What I'm a bit unsure about is the way I deal with the extension and the fact that I use a BufferedImage.

public Object getAsObject(FacesContext arg0, UIComponent arg1, String urlStr) {
    //extension of the image
    String extension = urlStr.substring(urlStr.lastIndexOf(".")+1);
    //object containing some infos I need
    UrlImage urlImage = new UrlImage();
    if(("jpg").equals(extension)||("jpeg").equals(extension)||("png").equals(extension)){
        urlImage.setStringUrl(urlStr);
        ByteArrayOutputStream os = null;
        try {
                final URL url = new URL(urlStr);
                final HttpURLConnection connection = (HttpURLConnection) url
                        .openConnection();
                connection.setRequestProperty("User-Agent","Mozilla/5.0 (Macintosh; Intel Mac OS X 10_7_5) AppleWebKit/537.31 (KHTML, like Gecko) Chrome/26.0.1410.65 Safari/537.31");

                // I'm not sure I should be doing that here
                BufferedImage image = ImageIO.read(connection.getInputStream());

                urlImage.setBuffImg(image);

                connection.disconnect();
                String str;

                os = new ByteArrayOutputStream();
                ImageIO.write(image, extension, Base64.getEncoder().wrap(os));
                str =  os.toString(StandardCharsets.ISO_8859_1.name());
                urlImage.setBase64image("data:image/"+extension+";base64,"+str);
                System.out.println("data:image/"+extension+";base64,"+str);
        } catch (MalformedURLException e) {
        } catch (IOException e) {
        }finally {
            IOUtils.closeQuietly(os);
        }
    }
        return urlImage;

}

UrlImage is just:

public class UrlImage{
    private String stringUrl;
    private String base64image;
    private BufferedImage buffImg; // this will be used to crop the image.
}

Basically there is an input field on the website. When user types an URL in the input field, an ajax request is made and the method above is called with the URL the user typed (He could change 10 times the URL which means it will go 10 times through that method). When I've the data as base 64 I send it back to the browser of the user which displays the image. Then the user can crop the image as he pleases and when he is done he presses OK. Afterward I crop the image on server side with the BufferedImage (that I already have because of the previous ajax req) and save the file.

I would have done the process of converting an URL to base 64 on client side if I could but after some tries it seems like it is impossible because of cross-origin issues.

Whole code works though.

\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

I'm assuming you have some way to handle other image formats, like .bmp or .gif; if not to actually handle them, at least a way to tell the user "this service does not support that image type" or something of the sort.


public Object getAsObject(FacesContext arg0, UIComponent arg1, String urlStr) {

Looks like you've got some overhead in there, you don't use the arg0 or arg1, why are they in here? Why is your method name getAsObject when instead you're doing something like downloadImageFromUrlToBase64?

If your framework is troubling you like this, consider putting the actual work in a separate class or function and having a simple method that only passes what you need. That way you're less attached to the framework which you are not using.


String extension = urlStr.substring(urlStr.lastIndexOf(".")+1);

Here you have a bug; if I pass "jpeg" in here, you say 'the last index of "." is non-existent, so the return value will be -1, add one, is 0'. Meaning we substring from the start of the string, returning the full string.

You'll silently swallow the resulting MalformedURLException, so if there is going to be any feedback to the user, it's going to be along the lines of "something went wrong".


            final URL url = new URL(urlStr);
            final HttpURLConnection connection = (HttpURLConnection) url
                    .openConnection();
            connection.setRequestProperty("User-Agent","Mozilla/5.0 (Macintosh; Intel Mac OS X 10_7_5) AppleWebKit/537.31 (KHTML, like Gecko) Chrome/26.0.1410.65 Safari/537.31");

            // I'm not sure I should be doing that here
            BufferedImage image = ImageIO.read(connection.getInputStream());

            urlImage.setBuffImg(image);

            connection.disconnect();

Here you indeed have the option of moving this to a separate method. I think it would help.

private BufferedImage downloadImageFromUrl(String url){ ... }

String str;

os = new ByteArrayOutputStream();
ImageIO.write(image, extension, Base64.getEncoder().wrap(os));
str =  os.toString(StandardCharsets.ISO_8859_1.name());

This looks like it has seen some heavy editing. There's a variable str declared, but oh wait, gotta do something else first, namely filling the buffer, and only then can we put it in the string. There's also a double space between the = and os.toString, which means (to me) that you should probably run your autoformatter more often or configure it to fix double spaces like that.

Anyway, there's no need to create a local variable before you're going to use it in this case, so just reorder the statements:

os = new ByteArrayOutputStream();
ImageIO.write(image, extension, Base64.getEncoder().wrap(os));
String str = os.toString(StandardCharsets.ISO_8859_1.name());

And give the local a better name too, I almost thought you were appending 'ISO_8859_1' to your urlImage.

os = new ByteArrayOutputStream();
ImageIO.write(image, extension, Base64.getEncoder().wrap(os));
String imageInBase64 = os.toString(StandardCharsets.ISO_8859_1.name());

Lastly, I don't know what you're doing with that user-agent string, but I don't think you're 4 browsers at once and it probably needs a comment.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.