Problem
Honestly, I can’t help but feel that this is done merely to confuse newcomers. Most of the errors on Stack Overflow by complete Android newbies mostly stem from that they have a static inner class Fragment that they don’t understand how it works, why it’s there, and try to use Activities even though they don’t fully understand the concept behind what is happening.
I must admit, I had trouble understanding the PlaceholderFragment
approach too, and using static inner classes isn’t really extensible at all. The first thing you’d have to do is create an actual class outside – but why do newbies have to do that?
I think this could be much more efficient if they used a project structure similar to the following simple fragment-based android project structure:
- src
- wholepackagename
- activity
- MainActivity
- fragment
- FirstFragment
- SecondFragment
- res
- layout
- values
- …
With the code of
src/wholepackagename/activity/MainActivity:
public class MainActivity extends FragmentActivity implements FirstFragment.Callback
{
@Override
protected void onCreate(Bundle savedInstanceState)
{
super.onCreate(savedInstanceState);
setContentView(R.layout.activity_main);
getSupportFragmentManager().addOnBackStackChangedListener(new OnBackStackChangedListener()
{
public void onBackStackChanged()
{
int backCount = getSupportFragmentManager().getBackStackEntryCount();
if (backCount == 0)
{
finish();
}
}
});
if (savedInstanceState == null)
{
getSupportFragmentManager().beginTransaction().add(R.id.main_container, new FirstFragment()).addToBackStack(null).commit();
}
}
@Override
public void firstFragmentCallback()
{
getSupportFragmentManager().beginTransaction().replace(R.id.main_container, new SecondFragment()).addToBackStack(null).commit();
}
}
src/wholepackagename/fragment/FirstFragment.java:
public class FirstFragment extends Fragment implements View.OnClickListener
{
private Callback callback;
private Button firstFragmentButton;
public static interface Callback
{
void firstFragmentCallback();
}
public FirstFragment()
{
super();
}
@Override
public void onAttach(Activity activity)
{
super.onAttach(activity);
try
{
callback = (Callback) activity;
}
catch (ClassCastException e)
{
Log.e(getClass().getSimpleName(), activity.getClass().getSimpleName() + " must implement Callback interface!", e);
throw e;
}
}
@Override
public View onCreateView(LayoutInflater inflater, ViewGroup container, Bundle savedInstanceState)
{
View rootView = inflater.inflate(R.layout.fragment_first, container, false);
firstFragmentButton = (Button) rootView.findViewById(R.id.fragment_first_button);
firstFragmentButton.setOnClickListener(this);
return rootView;
}
@Override
public void onClick(View v)
{
if(v == firstFragmentButton)
{
callback.firstFragmentCallback();
}
};
}
src/wholepackagename/fragment/SecondFragment.java:
public class SecondFragment extends Fragment implements View.OnClickListener
{
private Button secondFragmentButton;
public SecondFragment()
{
super();
}
@Override
public View onCreateView(LayoutInflater inflater, ViewGroup container, Bundle savedInstanceState)
{
View rootView = inflater.inflate(R.layout.fragment_second, container, false);
secondFragmentButton = (Button) rootView.findViewById(R.id.fragment_second_button);
secondFragmentButton.setOnClickListener(this);
return rootView;
}
@Override
public void onClick(View v)
{
if(v == secondFragmentButton)
{
Toast.makeText(getActivity(), getActivity().getString(R.string.example_text), Toast.LENGTH_LONG).show();
}
};
}
Android-Manifest.xml:
<?xml version="1.0" encoding="utf-8"?>
<manifest xmlns:android="http://schemas.android.com/apk/res/android"
package="wholepackagename"
android:versionCode="1"
android:versionName="1.0" >
<uses-sdk
android:minSdkVersion="8"
android:targetSdkVersion="19" />
<application
android:allowBackup="true"
android:icon="@drawable/ic_launcher"
android:label="@string/app_name"
android:theme="@style/AppTheme" >
<activity
android:name="wholepackagename.activity.MainActivity"
android:label="@string/app_name">
<intent-filter>
<action android:name="android.intent.action.MAIN" />
<category android:name="android.intent.category.LAUNCHER" />
</intent-filter>
</activity>
</application>
</manifest>
res/layout/activity_main.xml:
<FrameLayout xmlns:android="http://schemas.android.com/apk/res/android"
xmlns:tools="http://schemas.android.com/tools"
android:id="@+id/main_container"
android:layout_width="match_parent"
android:layout_height="match_parent" />
res/layout/fragment_first.xml:
<?xml version="1.0" encoding="utf-8"?>
<RelativeLayout xmlns:android="http://schemas.android.com/apk/res/android"
android:layout_width="match_parent"
android:layout_height="match_parent" >
<Button
android:id="@+id/fragment_first_button"
android:layout_width="wrap_content"
android:layout_height="wrap_content"
android:layout_centerInParent="true"
android:text="@string/first_button" />
</RelativeLayout>
res/layout/fragment_second.xml:
<?xml version="1.0" encoding="utf-8"?>
<RelativeLayout xmlns:android="http://schemas.android.com/apk/res/android"
android:layout_width="match_parent"
android:layout_height="match_parent" >
<Button
android:id="@+id/fragment_second_button"
android:layout_width="wrap_content"
android:layout_height="wrap_content"
android:layout_centerInParent="true"
android:text="@string/second_button" />
</RelativeLayout>
res/values/strings.xml:
<?xml version="1.0" encoding="utf-8"?>
<resources>
<string name="app_name">Application name</string>
<string name="hello_world">Hello world!</string>
<string name="action_settings">Settings</string>
<string name="first_button">First Button</string>
<string name="second_button">Second Button</string>
<string name="example_text">This is an example!</string>
</resources>
To me, it seems that the static inner class Fragment
doesn’t really support any kind of maintainability, it’s hard to see what is going on, and the overall functionality is not obvious as the Activity and Fragment (display of fragment and logic) are mixed together, which makes it hard to see and oversee for a newbie.
I believe it would provide an easier entry into developing for the Android platform to have an example like above. Are there any benefits to the current approach, providing a static inner class as a Fragment
?
Solution
I totally agree that it is better to have a Fragment in it’s own class rather than as a static inner class. I don’t think I’ve seen any cases where it’s been a static inner class, but perhaps I’m just lucky (or have a bad memory).
Overall your code is very clean. And I really mean very clean. Probably the cleanest code I’ve read today (that includes my own code).
If this is meant to be a tutorial kind of example code though, there’s a few things I’d like to say.
First of all, your braces are using the C# convention. The Java convention is to use:
xxxx {
// stuff
}
That is, the opening brace {
goes on the same line and not on its own.
As you’re coding Java, I’d recommend using the Java convention. At least your style is consistent though, but as it seems to be code for teaching others, it’s important to teach the correct conventions.
public static interface Callback
{
void firstFragmentCallback();
}
Excellent little interface there, I’m just afraid that with this naming you’ll end up with ten interfaces named Callback
. Naming it FirstFragmentCallback
would be better.
These constructors can be removed completely without altering the behavior of your application in any way. As all they do is call super()
, which is implicitly called anyway, and that you don’t have any other constructors, they’re not required at all.
public FirstFragment()
{
super();
}
public SecondFragment()
{
super();
}
Log.e(getClass().getSimpleName(), activity.getClass().getSimpleName() + " must implement Callback interface!", e);
It’s excellent that you’re logging this and re-throwing the exception, I would however log the canonical name (some.package.SomeActivity
) and not just the simple name (SomeActivity
).
An alternative here though would be to provide a setCallback
method. I am not sure what the best practice is actually with regards to this, but well… it’s an alternative. It would add some flexibility as the activity itself doesn’t have to be the one implementing the callback.
FirstFragment fragment = new FirstFragment();
fragment.setCallback(new FirstFragment.Callback(){ ... });
getSupportFragmentManager().beginTransaction().add(R.id.main_container, fragment).addToBackStack(null).commit();
Toast.makeText(getActivity(), "This is an example!", Toast.LENGTH_LONG).show();
OK, OK, I know it’s an example, but still… you have a res/values/strings.xml
already with some additional strings, it would be preferred to use a string resource here as well.
Again, overall it’s very neat and clean. These were all the issues I’ve found. Good job.