Two Fragments in Landscape Mode Challenge

Question

I did the following code challenge for a job interview. They apparently didn't care for my solution and I'm not sure why.

What could I have done better?

Here is the challenge:

Create a small app with the following characteristics:

With the device in the portrait mode, display a fragment with a single EditText instructing the user to enter his or her name. Below that, display five buttons labelled "Button 1", "Button 2", "Button 3", "Button 4" "Button 5". Tapping any of those buttons will swap in a new fragment with a simple text field displaying the message, "Hello ! You tapped Button 1" (or 2, and so on). Tapping the back button returns to the original fragment with the five buttons.

When the device is in the landscape mode, display the two fragment side-by-side within the same activity, but keep the behavior described above (except that the back button will exit the app as there is no fragment to go back to.)

[Feel free to use either the Fragment call introduced in API level 11 (android.app.Fragment) or the Fragment class found in the Android support library (android.support.v4.app.Fragment).]

MainActivity.java:

public class MainActivity extends FragmentActivity {

    @Override
    protected void onCreate(Bundle savedInstanceState) {
        super.onCreate(savedInstanceState);

        //Remove title bar
        this.requestWindowFeature(Window.FEATURE_NO_TITLE);

        setContentView(us.jasonh.fragmentschallenge.R.layout.layout_main);

        if (savedInstanceState != null) {
            String name = savedInstanceState.getString("name");
            EditText editText = (EditText) findViewById(us.jasonh.fragmentschallenge.R.id.editText);
            editText.setText(name);
        }
    }

    @Override
    protected void onSaveInstanceState(Bundle outState) {
        super.onSaveInstanceState(outState);

        EditText editText = (EditText) findViewById(us.jasonh.fragmentschallenge.R.id.editText);
        String name = String.valueOf(editText.getText());
        outState.putString("name", name);
    }
}

DetailActivity.java:

public class DetailActivity extends FragmentActivity {

    @Override
    protected void onCreate(Bundle savedInstanceState) {
        super.onCreate(savedInstanceState);

        //Remove title bar
        requestWindowFeature(Window.FEATURE_NO_TITLE);

        // Add the Detail fragment and pass Intent Extras
        DetailFragment detailFragment = new DetailFragment();
        detailFragment.setArguments(getIntent().getExtras());
        getSupportFragmentManager().beginTransaction().add(android.R.id.content, detailFragment).commit();
    }
}

InputFragment.java:

public class InputFragment extends Fragment {

    @Override
    public View onCreateView(LayoutInflater inflater, ViewGroup container, Bundle savedInstanceState) {

        return inflater.inflate(us.jasonh.fragmentschallenge.R.layout.fragment_input, container, false);
    }

    @Override
    public void onActivityCreated(Bundle savedInstanceState) {
        super.onActivityCreated(savedInstanceState);

        /**
         * Create the buttons programmatically
         */
        int buttonCount = 5;
        Button[] button = new Button[buttonCount];

        // Create layout for button
        int pad = dpAsPixels(40);
        int margin = dpAsPixels(5);
        LinearLayout layoutContainer = (LinearLayout)getActivity().findViewById(us.jasonh.fragmentschallenge.R.id.ll_container);
        LinearLayout.LayoutParams buttonParam = new LinearLayout.LayoutParams(LinearLayout.LayoutParams.WRAP_CONTENT, LinearLayout.LayoutParams.WRAP_CONTENT);
        buttonParam.setMargins(margin, 0, 0, 0);

        for (int i = 0; i < buttonCount; i++) {
            button[i] = new Button(getActivity());
            button[i].setText("Button " + (i + 1));
            button[i].setLayoutParams(buttonParam);
            button[i].setPadding(pad, 0, pad, 0);

            layoutContainer.addView(button[i]);

            final String number = String.valueOf(i + 1);

            button[i].setOnClickListener(new View.OnClickListener() {
                @Override
                public void onClick(View v) {

                    EditText editText = (EditText) getActivity().findViewById(us.jasonh.fragmentschallenge.R.id.editText);
                    String name = String.valueOf(editText.getText());
                    String message = "Hello, " + name + "!  You tapped Button " + number;

                    // Rather than detect orientation, just detect if Detail fragment is currently displayed
                    View detailFrame = getActivity().findViewById(us.jasonh.fragmentschallenge.R.id.detail);
                    boolean isDual = detailFrame != null && detailFrame.getVisibility() == View.VISIBLE;

                    // If in landscape mode, change the message
                    if (isDual) {
                        TextView textView = (TextView) getActivity().findViewById(us.jasonh.fragmentschallenge.R.id.message);
                        textView.setText(message);
                    // If in portrait mode, create an Intent
                    } else {
                        Intent intent = new Intent();
                        intent.setClass(getActivity(), DetailActivity.class);
                        intent.putExtra("message", message);
                        startActivity(intent);
                    }
                }
            });
        }
    }

    private int dpAsPixels(int dp) {
        float scale = getResources().getDisplayMetrics().density;
        return (int) (dp * scale + 0.5f);
    }
}

DetailFragment.java:

public class DetailFragment extends Fragment {

    @Override
    public View onCreateView(LayoutInflater inflater, ViewGroup container, Bundle savedInstanceState) {

        View view = inflater.inflate(us.jasonh.fragmentschallenge.R.layout.fragment_detail, container, false);
        TextView textView = (TextView) view.findViewById(us.jasonh.fragmentschallenge.R.id.message);

        Bundle args = getArguments();
        String message = (args != null) ? args.getString("message") : "";

        textView.setText(message);

        return view;
    }

}

layout/layout_mail.xml:

<FrameLayout xmlns:android="http://schemas.android.com/apk/res/android"
    android:layout_width="match_parent" android:layout_height="match_parent">

    <fragment class="us.jasonh.fragmentschallenge.InputFragment"
            android:id="@+id/input"
            android:layout_width="match_parent"
            android:layout_height="match_parent" />
</FrameLayout>

layout-land/layout_main.xml:

<LinearLayout xmlns:android="http://schemas.android.com/apk/res/android"
    android:orientation="horizontal"
    android:layout_width="match_parent"
    android:layout_height="match_parent"
    android:baselineAligned="false">

    <fragment class="us.jasonh.fragmentschallenge.InputFragment"
            android:id="@+id/input"
            android:layout_width="0dp"
            android:layout_height="match_parent"
            android:layout_weight="1" />

    <fragment class="us.jasonh.fragmentschallenge.DetailFragment"
            android:id="@+id/detail"
            android:layout_width="0dp"
            android:layout_height="match_parent"
            android:layout_weight="1" />

</LinearLayout>

(Full code found at github: https://github.com/jasonhartley/Fragments-Challenge)

Answer 1

Besides what janos mentioned, there is one thing in your code which I really really don't like.

EditText editText = (EditText) getActivity().findViewById(us.jasonh.fragmentschallenge.R.id.editText);
String name = String.valueOf(editText.getText());
String message = "Hello, " + name + "!  You tapped Button " + number;

The call to getActivity() is not the responsibility of the fragment. You are violating 'Tell, don't ask'. Big time. Instead you should read up on Communicating with Other fragments

// Rather than detect orientation, just detect if Detail fragment is currently displayed
View detailFrame = getActivity().findViewById(us.jasonh.fragmentschallenge.R.id.detail);
boolean isDual = detailFrame != null && detailFrame.getVisibility() == View.VISIBLE;

// If in landscape mode, change the message
if (isDual) {
    TextView textView = (TextView) getActivity().findViewById(us.jasonh.fragmentschallenge.R.id.message);
    textView.setText(message);
// If in portrait mode, create an Intent
} else {
    Intent intent = new Intent();
    intent.setClass(getActivity(), DetailActivity.class);
    intent.putExtra("message", message);
    startActivity(intent);
}

What you are doing here is that you are doing something that is the responsibility of the activity. But you are handling it inside the fragment.

This is a bad practice. If anything, this is what I would have rejected you for.

• Avoid calling getActivity() in a Fragment
• Give the Fragment a callback interface which handles this. In this case, MainActivity should be the class implementing the interface.

Read up on Communicating with Other fragments for how to properly do this.

Answer 2

import us.jasonh.fragmentschallenge.R

It's unusual and ugly that your package name us.jasonh.fragmentschallenge appears in many places in the source code:

setContentView(us.jasonh.fragmentschallenge.R.layout.layout_main);

The common approach is to import the generated R file:

import us.jasonh.fragmentschallenge.R

So that you can simplify the source code and get rid of the ugly duplicated package names everywhere:

setContentView(R.layout.layout_main);

Search through your code and make sure to remove all occurrences of your package name (except import statements).

Simplify

In onActivityCreated, you don't need to store an array of buttons. You could just iterate from 1 to 5, and in each cycle create a Button, configure it, including the click listener, and add it to the view. No need for Button[].

Also, the click listener's implementation is not trivial, embedding it inside the onActivityCreated method makes the method's body a bit long, which can be flagged as a warning by static analysis tools. It would be better to use a named private inner class instead of an anonymous class for the click listener implementation.

Duplicated string constants

In onSaveInstanceState, and when you restore from it, you save and load "name". If you write "name" in two places, that's fragile, because if you change it in one place, you have to remember to change in the other too. The common approach is to put this in a static constant variable:

private static final String KEY_NAME = "name";

And use this variable when saving and restoring "name".

The same goes for the "message" key you pass between the fragments.

Misc

Instead of this:

String.valueOf(editText.getText())

I prefer this way:

editText.getText().toString()

For one thing, it's shorter. But maybe it's just a matter of taste.