Vaadin web application

Question

I am creating a web application for an enterprise and I would like to improve my way of coding, know what I do wrong and what I do well. I'll leave the main class' code here:

package com.puntobile;

import com.google.gson.Gson;
import com.google.gson.JsonIOException;
import com.google.gson.JsonSyntaxException;
import com.google.gson.reflect.TypeToken;
import com.google.gwt.event.dom.client.KeyPressHandler;
import com.vaadin.annotations.Theme;
import com.vaadin.client.ui.layout.Margins;
import com.vaadin.data.Property.ValueChangeEvent;
import com.vaadin.data.Property.ValueChangeListener;
import com.vaadin.data.util.IndexedContainer;
import com.vaadin.event.Action;
import com.vaadin.event.Action.Handler;
import com.vaadin.event.ShortcutAction;
import com.vaadin.server.*;
import com.vaadin.shared.ui.datefield.Resolution;
import com.vaadin.ui.*;
import com.vaadin.ui.Button.ClickEvent;
import com.vaadin.ui.Button.ClickListener;
import com.vaadin.ui.Upload.Receiver;
import com.vaadin.ui.Upload.SucceededEvent;
import com.vaadin.ui.Upload.SucceededListener;

import java.io.BufferedWriter;
import java.io.FileNotFoundException;
import java.io.IOException;
import java.io.OutputStream;
import java.util.*;
import java.io.*;
import java.lang.reflect.Type;

import org.quartz.*;

@Theme("puntobiletheme")
public class PuntobileUI extends UI implements Receiver, SucceededListener {

    private static final long serialVersionUID = 1L;
    private final String[] fieldNames = { "Nombre", "IP", "Directorio",
    "Estado" };

    public static PuntobileUI INSTANCE;

    Table signs = new Table("Signs");
    IndexedContainer signsContainer = new IndexedContainer();

    List<Sign> signList = new ArrayList<Sign>();

    Table schedule = new Table();

    String owner = "guest";

    List<User> userList = new ArrayList<User>();

    @Override
    protected void init(VaadinRequest request) {
        INSTANCE = this;
        loadUsers();
        FormLayout loginLayout = new FormLayout();
        this.setContent(loginLayout);
        Label lblLogin = new Label("Iniciar sesión");
        lblLogin.setStyleName("boldLabel");
        final TextField username = new TextField("Nombre de usuario: ");
        final PasswordField password = new PasswordField("Contraseña: ");
        Button btnLogin = new Button("Iniciar sesión");
        btnLogin.addClickListener(new ClickListener() {
            private static final long serialVersionUID = 8727890390426574530L;

            @Override
            public void buttonClick(ClickEvent event) {
                boolean worked = false;
                for(User u : userList)
                    if ((username.getValue().equals(u.username) && password
                            .getValue().equals(u.password))) {
                        initLayout(username.getValue());
                        initSigns();
                        initSchedule();
                        worked = true;
                    } else {

                    }
                if(!worked) Notification.show("Usuario o contraseña erróneos",
                        Notification.Type.WARNING_MESSAGE);
            }
        });
        loginLayout.setMargin(true);
        loginLayout.addComponent(lblLogin);
        loginLayout.addComponent(username);
        loginLayout.addComponent(password);
        loginLayout.addComponent(btnLogin);
    }

    @SuppressWarnings("serial")
    private void initLayout(final String owner) {
        this.owner = owner;
        HorizontalSplitPanel splitPanel = new HorizontalSplitPanel();
        this.setContent(splitPanel);
        VerticalLayout leftLayout = new VerticalLayout();
        leftLayout.addComponent(signs);
        Button btnManage = new Button("Administrar cuentas");
        BrowserWindowOpener popupOpener = new BrowserWindowOpener(UserManager.class);
        popupOpener.extend(btnManage);
        Button btnDel = new Button("Borrar");
        btnDel.addClickListener(new ClickListener() {
            @Override
            public void buttonClick(ClickEvent event) {
                if(signs.getValue() != null)
                    signList.remove(((Integer) signs.getValue()) - 1);
                refreshSigns();
            }
        });
        HorizontalLayout bottomLayout = new HorizontalLayout();
        bottomLayout.addComponent(btnDel);
        if (this.owner.equals("PuntoBile"))
            bottomLayout.addComponent(btnManage);
        leftLayout.addComponent(bottomLayout);
        splitPanel.addComponent(leftLayout);
        signs.setSizeFull();

        VerticalLayout rightLayout = new VerticalLayout();
        splitPanel.addComponent(rightLayout);
        rightLayout.setMargin(true);

        FormLayout addLayout = new FormLayout();
        addLayout.setMargin(true);
        rightLayout.addComponent(addLayout);
        Label lblAdd = new Label("Añadir");
        lblAdd.setStyleName("boldLabel");
        addLayout.addComponent(lblAdd);

        final TextField txtIP = new TextField("IP: ");
        final TextField txtName = new TextField("Nombre: ");
        final TextField txtRoot = new TextField("Directorio: ");
        Button btnAdd = new Button("Añadir");
        btnAdd.addClickListener(new ClickListener() {
            @Override
            public void buttonClick(ClickEvent event) {
                signList.add(new Sign(txtIP.getValue(), txtName.getValue(),
                        new File(txtRoot.getValue()), owner));
                txtIP.setValue("");
                txtName.setValue("");
                txtRoot.setValue("");
                refreshSigns();
                saveSigns();
            }
        });
        addLayout.addComponent(txtName);
        addLayout.addComponent(txtRoot);
        addLayout.addComponent(txtIP);
        addLayout.addComponent(btnAdd);

        VerticalLayout scheduleLayout = new VerticalLayout();
        Label lblSchedule = new Label("Programación");
        lblSchedule.setStyleName("boldLabel");
        scheduleLayout.addComponent(lblSchedule);
        scheduleLayout.addComponent(schedule);
        rightLayout.addComponent(scheduleLayout);

        FormLayout uploadLayout = new FormLayout();
        Label lblUpload = new Label("Subir vídeo");
        lblUpload.setStyleName("boldLabel");
        uploadLayout.addComponent(lblUpload);
        Upload upload = new Upload(null, this);
        upload.setButtonCaption("Subir");
        upload.addSucceededListener(this);
        uploadLayout.addComponent(upload);
        rightLayout.addComponent(uploadLayout);

        FormLayout schedulePlayLayout = new FormLayout();
        Label lblSchedulePlay = new Label("Programar un vídeo");
        lblSchedulePlay.setStyleName("boldLabel");
        schedulePlayLayout.addComponent(lblSchedulePlay);
        final ComboBox cbVideos = new ComboBox("Elige un vídeo: ");
        signs.addValueChangeListener(new ValueChangeListener() {
            @Override
            public void valueChange(ValueChangeEvent event) {
                cbVideos.removeAllItems();
                if (signs.getValue() != null && signList.get((Integer) signs.getValue() - 1).root
                        .listFiles() != null)
                    for (File f : signList.get((Integer) signs.getValue() - 1).root
                            .listFiles())
                        cbVideos.addItem(f.getName());
            }
        });

        schedulePlayLayout.addComponent(cbVideos);
        final InlineDateField date = new InlineDateField("Horario: ");
        date.setValue(new Date());
        date.setResolution(Resolution.MINUTE);
        schedulePlayLayout.addComponent(date);
        ComboBox cbRepeat = new ComboBox("Repetir: ");
        cbRepeat.addItem("Nunca");
        cbRepeat.addItem("Cada 24 horas");
        Button btnAddSchedule = new Button("Añadir");
        btnAddSchedule.addClickListener(new ClickListener() {
            @Override
            public void buttonClick(ClickEvent event) {
                //TODO: Add schedules
            }
        });
        rightLayout.addComponent(schedulePlayLayout);
    }

    private void initSigns() {
        this.signsContainer.addContainerProperty(fieldNames[0], String.class,
                fieldNames[0]);
        this.signsContainer.addContainerProperty(fieldNames[1], String.class,
                fieldNames[1]);
        this.signsContainer.addContainerProperty(fieldNames[2], String.class,
                fieldNames[2]);
        this.signsContainer
        .addContainerProperty(fieldNames[3], Image.class, new Image(
                "Conectado", new ThemeResource("icons/accept16.png")));
        signs.setContainerDataSource(signsContainer);
        signs.setVisibleColumns(fieldNames);
        signs.setSelectable(true);

        loadSigns();
    }

    private void initSchedule() {
        schedule.addContainerProperty("Horario", String.class, "Horario");
        schedule.addContainerProperty("Video", String.class, "Video");
        schedule.setVisibleColumns(new String[] { "Horario", "Video" });
        schedule.setSizeFull();
        schedule.setSortEnabled(false);
        schedule.setPageLength(schedule.size());
        schedule.addValueChangeListener(new ValueChangeListener() {
            private static final long serialVersionUID = 1L;

            @Override
            public void valueChange(ValueChangeEvent event) {
                schedule.removeAllItems();
            }
        });
    }

    @SuppressWarnings("unchecked")
    private void refreshSigns() {
        signs.removeAllItems();
        signsContainer = new IndexedContainer();
        this.signsContainer.addContainerProperty(fieldNames[0], String.class,
                fieldNames[0]);
        this.signsContainer.addContainerProperty(fieldNames[1], String.class,
                fieldNames[1]);
        this.signsContainer.addContainerProperty(fieldNames[2], String.class,
                fieldNames[2]);
        this.signsContainer
        .addContainerProperty(fieldNames[3], Image.class, new Image(
                "Conectado", new ThemeResource("icons/accept16.png")));
        signs.setContainerDataSource(signsContainer);
        for (Sign s : signList) {
            if (s.owner.equals(this.owner) || this.owner.equals("PuntoBile")) {
                Object id = signsContainer.addItem();
                this.signsContainer.getContainerProperty(id, fieldNames[0])
                .setValue(s.nickname);
                this.signsContainer.getContainerProperty(id, fieldNames[1])
                .setValue(s.url);
                this.signsContainer.getContainerProperty(id, fieldNames[2])
                .setValue(s.root.getPath());
                if (s.isOnline())
                    this.signsContainer.getContainerProperty(id, fieldNames[3])
                    .setValue(
                            new Image("Conectado", new ThemeResource(
                                    "icons/accept16.png")));
                else
                    this.signsContainer.getContainerProperty(id, fieldNames[3])
                    .setValue(
                            new Image("Conectado", new ThemeResource(
                                    "icons/stop16.png")));
            }
        }
    }

    private void saveSigns() {
        File f = new File("signs.json");
        try {
            BufferedWriter bw = new BufferedWriter(new FileWriter(f));
            bw.write(new Gson().toJson(this.signList));
            bw.close();
        } catch (IOException e) {
            Notification.show("Error al crear la base de datos en "
                    + f.getAbsolutePath());
            e.printStackTrace();
        }
    }

    private void loadSigns() {
        try {
            Type t = new TypeToken<ArrayList<Sign>>() {
            }.getType();
            signList = new Gson().fromJson(new FileReader("signs.json"), t);
            if (signList == null)
                signList = new ArrayList<Sign>();
            refreshSigns();
        } catch (JsonIOException e) {
            e.printStackTrace();
        } catch (JsonSyntaxException e) {
            e.printStackTrace();
        } catch (FileNotFoundException e) {
            e.printStackTrace();
        }
    }


    public void saveUsers() {
        File f = new File("users.json");
        try {
            BufferedWriter bw = new BufferedWriter(new FileWriter(f));
            bw.write(new Gson().toJson(this.userList));
            bw.close();
        } catch (IOException e) {
            Notification.show("Error al crear la base de datos en "
                    + f.getAbsolutePath());
            e.printStackTrace();
        }
    }

    private void loadUsers() {
        try {
            Type t = new TypeToken<ArrayList<User>>() {
            }.getType();
            userList = new Gson().fromJson(new FileReader("users.json"), t);
            if (userList == null)
                userList = new ArrayList<User>();
            refreshSigns();
        } catch (JsonIOException e) {
            e.printStackTrace();
        } catch (JsonSyntaxException e) {
            e.printStackTrace();
        } catch (FileNotFoundException e) {
            e.printStackTrace();
        }
    }

    File file;

    @Override
    public OutputStream receiveUpload(String filename, String mimeType) {
        FileOutputStream fos = null;
        try {
            file = new File("/tmp/uploads/" + filename);
            fos = new FileOutputStream(file);
        } catch (final java.io.FileNotFoundException e) {
            Notification.show("No se ha podido encontrar el archivo",
                    Notification.Type.ERROR_MESSAGE);
            return null;
        }
        return fos;
    }

    @Override
    public void uploadSucceeded(SucceededEvent event) {
        file.renameTo(new File(
                signList.get((Integer) signs.getValue() - 1).root
                + File.separator + file.getName()));
        Notification.show("Video subido");
    }

}

Answer 1

Some comments: (I bet other reviewers can find more)

Private and final

As much as possible, fields should be marked with private and final. One of these fields is signs. There are several others that have the "default" visibility when they can probably be marked private.

private final Table signs = new Table("Signs");

Handling exceptions

    } catch (JsonIOException e) {
        e.printStackTrace();
    } catch (JsonSyntaxException e) {
        e.printStackTrace();
    } catch (FileNotFoundException e) {
        e.printStackTrace();
    }

As this seems to be an UI application, don't simply print the stacktrace of an exception. I doubt that the users are able to see the stack trace in the output, and even if they could, most of them would have no idea what it a JsonIOException means. Instead, provide appropriate error messages in dialogs.

Empty else

On one occasion you have this:

                } else {

                }

The else is not needed since it doesn't do anything there. If you plan on doing something there, add a // TODO: Fix this (but with a better message to yourself than "Fix this" otherwise you will have a hundred "Fix this" sooner or later - trust me, I've seen it happen). If you believe that an else cannot happen, either throw an exception or show an error message telling you that it actually did happen.

Long method

A very common code smell is to have a very long method. Although very common with methods that are initializing layouts, consider splitting your initLayout method in multiple pieces.

public static PuntobileUI INSTANCE

This tells me that your PuntobileUI is a singleton. Although singletons are handy sometimes, using UI-singletons regularly breaks the Tell-Don't-Ask principle. I prefer to inform objects about "this is my PuntobileUI instance" rather than having the objects use a singleton instance. By referencing a singleton instance from other classes, you make your code less independent and increase the coupling of your classes which is generally bad because it reduces code re-usability.