Displaying a city name with concatenations

Question

I am learning MVC and Android. I have create a sample program which is supposed to implement the MVC philosophy. I plan to build my codes on top of this.

I wish to know whether I have got the concept more or less, or if there are any fundamental issues in my understanding of the MVC.

Summary: This application will get a city name and display it with a few concatenations (country name etc).

Model -> Model.java

/**
 * 
 */
package abc.samples.mvc;
/**
 * @author Rahul
 *
 */
public class Model {

    String mData;
    String mInput;
    String mOutput;

    public void set(String temp) {
        mInput = temp;
        mData = mInput + ", India.";
    }

    public String get() {
        mOutput = "You are from "+mData;
        return mOutput;
    }

}

View -> view.xml

<RelativeLayout xmlns:android="http://schemas.android.com/apk/res/android"
    xmlns:tools="http://schemas.android.com/tools"
    android:layout_width="match_parent"
    android:layout_height="match_parent"
    tools:context="${relativePackage}.${activityClass}" >

    <Button
        android:id="@+id/vControl"
        android:layout_width="fill_parent"
        android:layout_height="wrap_content"
        android:layout_alignParentLeft="true"
        android:layout_alignParentTop="true"
        android:text="Control" />

    <EditText
        android:id="@+id/vInput"
        android:layout_width="fill_parent"
        android:layout_height="wrap_content"
        android:layout_alignParentLeft="true"
        android:layout_below="@+id/vControl"
        android:ems="10" >
        <requestFocus />
    </EditText>

    <TextView
        android:id="@+id/vOutput"
        android:layout_width="fill_parent"
        android:layout_height="wrap_content"
        android:layout_alignParentLeft="true"
        android:layout_below="@+id/vInput"
        android:text="Large Text"
        android:textAppearance="?android:attr/textAppearanceLarge" />
</RelativeLayout>

Controller -> Controller.java

package abc.samples.mvc;

import android.app.Activity;

import android.os.Bundle;
import android.view.Menu;
import android.view.MenuItem;
import android.view.View;
import android.widget.Button;
import android.widget.EditText;
import android.widget.TextView;

public class Controller extends Activity {

    EditText cInput;
    TextView cOutput;
    Button cControl;
    Model cData;

    @Override
    protected void onCreate(Bundle savedInstanceState) {
        super.onCreate(savedInstanceState);
        setContentView(R.layout.view);

        cInput = (EditText) findViewById(R.id.vInput);
        cOutput = (TextView) findViewById(R.id.vOutput);
        cControl = (Button) findViewById(R.id.vControl);

        cControl.setOnClickListener(new View.OnClickListener() {

            @Override
            public void onClick(View v) {

                cData = new Model();

                cData.set(cInput.getText().toString());
                cOutput.setText(cData.get());
            }
        });
    }
}

Answer 1

A model should not have presentation logic, and be mostly about containing data. The text "You are from ..." Doesn't belong in a model. As it is, your model includes logic that concerns representation, and so you're violating MVC. You could use another class to be in charge of formatting a Model instance. In other words, it would be best if your Model just contained fields and did nothing else. Furthermore, all fields can be private final, for example:

public class City {

    private final String name;

    public City(String name) {
        this.name = name;
    }

    public String getName() {
        return name;
    }
}

I removed the other fields which seemed quite pointless for a model. And I renamed Model to City, because "Model" is way too abstract and not useful.

In the activity it's strange to create fields when you only use them in onCreate. It's best to limit variables to the smallest scope necessary. As such, all these fields should be local variables in onCreate, like this:

EditText input = (EditText) findViewById(R.id.vInput);
TextView output = (TextView) findViewById(R.id.vOutput);
Button control = (Button) findViewById(R.id.vControl);

These names are terrible by the way. (Now I realize your question may have been rather hypothetical, not a real program.) Give them better names.

Answer 2

I'd say you've more or less gotten the right idea. What i would like to change is in your onClick method you initialize a new Model. Move this initialization outisde of onClick.(You can still call for model inside onClick by writing:

Controller.model.this.set("string");

Another thing i react to is all instance variables have default visibility. You should have every single one private, both in the controller and in the model.