# Java MVC Implementation

I am trying to write an MVC based application from scratch using Java 8 and Swing... I am having some difficulties with this.

My controller code is:

public class Controllerv2 {
private final Map<String, View> views = new HashMap<>();
private final Map<String, Command> commands = new HashMap<>();

private final ModelContainer model = new ModelContainer(this);

public void addView(String key, View view) {
views.put(key, view);
}

public View getView(String key) {
return views.get(key);
}

public void showView(String key) {
getView(key).showForm();
}

public void addCommand(String commandString, Command cmd) {
commands.put(commandString, cmd);
}

public void executeControllerCommand(String commandString) {
commands.get(commandString).execute();
}
}

public abstract class View {
protected final JDialog dialog = new JDialog();
protected final GUIAction guiAction = new GUIAction();
protected final Controllerv2 controls;


Views are subclassed from this class:

public View(Controllerv2 suppliedController) {
this.controller = suppliedController;
}
// Fires up the form
protected abstract void initComponents();

// Package up the fields into an object of type T and return

// Self explanatory functions
public void showForm() {dialog.setVisible(true);}
public void hideForm() {dialog.setVisible(false);}

// For fields that need updating
public abstract void updateFields(String[] msg);
}


This is an example of a concrete View:

public final class VBasexLoginR extends View {
private JButton connectButton;
private JLabel connectionLabel;
private JTextField hostField;
private JLabel hostLabel;
private JButton pingButton;
private JTextField portField;
private JLabel portLabel;
private JLabel titleLabel;
private JTextField uidField;
private JLabel userId;

super(c);
initComponents();
}

@Override
protected void initComponents() {// Boilerplate to setup the form}

@Override
}

@Override
public void updateFields(String[] msg) {
connectionLabel.setText(msg[0]);
}
}


Am I going in the right direction with this?

Also, I think I may need different models, one for running queries on BaseX and then ones for working with the results. Thus far I have...

public class ModelContainer {
private final Map<String, Model> models = new HashMap<>();
private final Map<String, ModelCommand> commands = new HashMap<>();
private final Controllerv2 controller;

ModelContainer(Controllerv2 suppliedController) {
this.controller = suppliedController;
}

public void addModel(String key, Model model) {
models.put(key, model);
}

public Model getModel(String key) {
return models.get(key);
}

public void addCommand(String key, ModelCommand cmd) {
commands.put(key, cmd);
}

public void executeModelCommand(String key) {
commands.get(key).execute();
}
}


Views can be got at through the getView method in controls

Is this correct or is this overkill?

EDIT -- Taking into account Mat's Mug response, changed the code to reflect the naming conventions suggested.

• The silence says it all... Back to the drawing board! – swshaun Aug 8 '14 at 14:56
• Hi @swshaun! Welcome to code review. It takes some time to write a quality review (we're all about quality here), so it takes a little longer to get an answer than other stack exchange sites. Why not browse some other Q&As while you wait? – RubberDuck Aug 8 '14 at 15:47
• Okey pokes.... Thank you @ckuhn203 Sorry for flipping my lid! Thank you -- I have been frustrated a bit with MVC for a while now. – swshaun Aug 8 '14 at 16:01
• No worries. We've all been there. Feel free to stop by our chatroom if you like too. – RubberDuck Aug 8 '14 at 16:03

I'm only going to scratch the surface here, and leave more seasoned reviewers take a stab at this one, for I've never written a line of - but some concepts transcend language barriers.

Naming in particular.

### Signatures can, and should talk.

public void addView(String viewString, View view) {
views.put(viewString, view);
}

public View getView(String viewString) {
return views.get(viewString);
}


Take this line:

public void addView(String viewString, View view) {


The name viewString is pretty confusing. One has to look at how it's implemented to go "oh, that's a key for a hashmap!". How about this instead?

public void addView(String key, View view) {


Now there's no ambiguity whatsoever, and getView(String key) becomes crystal-clear.

### Consistency is King

This is just a minor thing, but I'd prefer consistency in the choice of verbs used to describe an action. For example, if a form is a view and a form gets shown, then a view should be shown too, not opened:

public void openView(String viewString) {
getView(viewString).showForm();
}


Would be

public void showView(String viewString) {
getView(viewString).showForm();
}


Same with commands - if a command executes, I wouldn't want the verb run to refer to the same thing, I'd prefer it be consistent:

public void runControllerCommand(String commandString) {
commands.get(commandString).execute();
}


Would be:

public void executeControllerCommand(String commandString) {
commands.get(commandString).execute();
}


### Single-letter identifiers

Don't. This:

ModelContainer(Controllerv2 c) {
this.controls = c;
}


Is pretty darn confusing. Does c stand for controller, or for controls? Actually there seems to be something fishy going on here, "controls" is a nice name for a collection of controls, but you have it as ..a Controllerv2 (is there a Controllerv3? Avoid "versioning" classes like this, especially in the early development stages). Notice the puzzled expression in my face? ;)

Anyway, that c would probably be better of as controller.

### Constructor arguments

I like this part - final makes your intent very clear:

private final Map<String, Model> models = new HashMap<>();
private final Map<String, ModelCommand> commands = new HashMap<>();
private final Controllerv2 controls;

ModelContainer(Controllerv2 c) {
this.controls = c;
}


I'd push it a step further though, and provide a way to initialize the class with models and commands, like this:

private final Map<String, Model> models = new HashMap<>();
private final Map<String, ModelCommand> commands = new HashMap<>();
private final Controllerv2 controller;

ModelContainer(Controllerv2 controller, Map<String, Model> models, Map<String, ModelCommand> commands) {
this.controller = controller;
this.commands = commands;
this.models = models;
}


And now the client code doesn't need to call AddCommand 20 times when there's that many commands to add.

### Indentation

Your indentation is a little off, too. Whenever a code file ends like this:

}
}


You know something's gone wrong. I'd blame it on the Java-style braces but that's just the mug talking, just select all but the signature and last closing brace, and give that Tab button some lovin' ;)

• Thank you -- so am I heading in the correct direction using this approach? – swshaun Aug 8 '14 at 18:16
• I don't know enough of the Java UI frameworks to tell, like I said I'm only scratching the surface. I'm sure more answers will come :) – Mathieu Guindon Aug 8 '14 at 18:20
• @swshaun actually looking again there is something that tickles: I don't think it's right for the view to know anything about a controller: seems like you have a circular reference in there; doesn't Java have something like delegates, or events? – Mathieu Guindon Aug 8 '14 at 20:06
• Ah good spot -- to be honest I have a small MVC project I want to refactor into something less coupled (hence the array of views and models) – swshaun Aug 8 '14 at 20:36