HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

Two Fragments in Landscape Mode Challenge

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
challengefragmentsmodetwolandscape

Problem

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

Solution

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.

Code Snippets

setContentView(us.jasonh.fragmentschallenge.R.layout.layout_main);
import us.jasonh.fragmentschallenge.R
setContentView(R.layout.layout_main);
private static final String KEY_NAME = "name";
String.valueOf(editText.getText())

Context

StackExchange Code Review Q#64045, answer score: 7

Revisions (0)

No revisions yet.