current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 6 down vote favorite
1

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

I want to know if I am going the correct way about 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
public abstract <T> T readFields();

// 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 JLabel passwordLabel;
private JButton pingButton;
private JTextField portField;
private JLabel portLabel;
private JPasswordField pwField;
private JLabel titleLabel;
private JTextField uidField;
private JLabel userId;

public VBasexLoginR(Controllerv2 c) {
    super(c);
    initComponents();
}

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


@Override
public BaseXCredentials readFields() {
    return new      BaseXCredentials(hostField.getText(),portField.getText(),uidField.getText(),pwField.getPassword());
}

@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.

share|improve this question
    
The silence says it all... Back to the drawing board! –  swshaun Aug 8 at 14:56
3  
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 at 15:47
1  
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 at 16:01
    
No worries. We've all been there. Feel free to stop by our chatroom if you like too. –  RubberDuck Aug 8 at 16:03

1 Answer 1

up vote 4 down vote

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' ;)

share|improve this answer
    
Thank you -- so am I heading in the correct direction using this approach? –  swshaun Aug 8 at 18:16
1  
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 :) –  Mat's Mug Aug 8 at 18:20
1  
@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? –  Mat's Mug Aug 8 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 at 20:36

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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