12
\$\begingroup\$

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?

\$\endgroup\$
  • 1
    \$\begingroup\$ Nice interesting question ! I hope it will receive a good amount of interest! \$\endgroup\$ – Marc-Andre Jul 20 '14 at 22:57
  • \$\begingroup\$ Glad to have found this less than a week into learning android development. It genuinely did trip me up when I read in a Java tutorial that public classes should each be placed in separate files, and then found the fragments public function (in my template) inside another public function. Thanks a lot! \$\endgroup\$ – Dom Apr 29 '15 at 22:10
  • 1
    \$\begingroup\$ @Dom you're welcome! Although I personally would like to note that there are numerous libraries out there that make code quality a bit (a lot) better, namely Butterknife, Otto, and Dagger2. Also worth looking at Retrofit, Picasso and Ion. \$\endgroup\$ – Zhuinden Apr 29 '15 at 22:16
7
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the comments! The only time I've ever seen it as a static inner class was in the auto-generated code, which (ironically) is meant to encourage newbies to use Fragments rather than simple Activities. I used the Callback approach in this fashion because outside of the Fragment scope, you need to specify the Fragment's class, thus the Callback names won't overlap. It was based on the approach described in the developer tutorial at developer.android.com/training/basics/fragments/… \$\endgroup\$ – Zhuinden Jul 24 '14 at 13:19
  • \$\begingroup\$ While it's true that those constructors are implicitly called (same for super()), a Fragment must have a no-parameter default constructor (although the IDE tends to tell you to use fragment.setArguments(Bundle) instead, because Bundles are kept throughout the lifecycle even after destruction and reconstruction, but constructor parameters are lost). I tend to write .getSimpleName() merely because on Android, apps are defined by their package name, so I figured displaying the fully qualified package name is not a good idea on the long term (I might be wrong on this one though). \$\endgroup\$ – Zhuinden Jul 24 '14 at 13:21
  • \$\begingroup\$ The setCallback() is a smart idea, if you create a FragmentCallback interface and create an anonymous class implementation. Although, if I think about it a bit further, the communication from Fragment -> Activity tends to occur so that the Activity can act as a mediator, and the Fragments wouldn't be directly connected at all. And if the Activity is the one who knows enough of the logical structure that it can convey the Fragment -> Activity -> Fragment communication, it'd be hard to implement this type of callback without having access to what only the Activity knows.. I'll think about it \$\endgroup\$ – Zhuinden Jul 24 '14 at 13:26
  • 1
    \$\begingroup\$ @Zhuinden There's a lot of things to reply on with what you said, I'll go to bed now but I suggest you drop by The 2nd Monitor chat tomorrow (or some day) and I can answer you in chat. That's easier than writing everything in comments :) \$\endgroup\$ – Simon Forsberg Jul 24 '14 at 23:40
  • 1
    \$\begingroup\$ Conclusion: using static inner classes for Fragments would mix the application logic, the view and the state changes into one gigantic überclass, which is bad and really hard to see and therefore not a right approach. \$\endgroup\$ – Zhuinden Jul 29 '14 at 10:05

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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