6
\$\begingroup\$

I wrote these classes and I would like to know if this is a correct way. I created a new Project with Blank activity and "Scrollable Tabs + Swipe" as Navigation type.

My main activity:

public class MyMainActvity extends FragmentActivity {

    private static String url = "http://www.myurl.it";

    static JSONObject jObj = null;
    /**
     * The {@link android.support.v4.view.PagerAdapter} that will provide
     * fragments for each of the sections. We use a
     * {@link android.support.v4.app.FragmentPagerAdapter} derivative, which
     * will keep every loaded fragment in memory. If this becomes too memory
     * intensive, it may be best to switch to a
     * {@link android.support.v4.app.FragmentStatePagerAdapter}.
     */
    SectionsPagerAdapter mSectionsPagerAdapter;

    /**
     * The {@link ViewPager} that will host the section contents.
     */
    ViewPager mViewPager;

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

        new JSONParse().execute();

        // Create the adapter that will return a fragment for each of the three
        // primary sections of the app.
    }

    @Override
    public boolean onCreateOptionsMenu(Menu menu) {
        // Inflate the menu; this adds items to the action bar if it is present.
        getMenuInflater().inflate(R.menu.menu_main_activity, menu);
        return true;
    }

    /**
     * A {@link FragmentPagerAdapter} that returns a fragment corresponding to
     * one of the sections/tabs/pages.
     */
    public class SectionsPagerAdapter extends FragmentPagerAdapter {

        public SectionsPagerAdapter(FragmentManager fm) {
            super(fm);
        }

        @Override
        public Fragment getItem(int position) {
            // getItem is called to instantiate the fragment for the given page.
            // Return a DummySectionFragment (defined as a static inner class
            // below) with the page number as its lone argument.

            switch (position) {
            case 0:
                {
                    HomeSection homeFrag= new HomeSection();
                    homeFrag.newInstance(jObj);
                    return homeFrag;
                }
            case 1:
            {
                ServiceSection servFrag= new ServiceSection();
                servFrag.newInstance(jObj);
                return servFrag;
            }
            case 2:
                {
                    Fragment fragment = new DummySectionFragment();
                    Bundle args = new Bundle();
                    args.putInt(DummySectionFragment.ARG_SECTION_NUMBER, position + 1);
                    fragment.setArguments(args);
                    return fragment;
                }
            case 3:
            {
                Fragment fragment = new DummySectionFragment();
                Bundle args = new Bundle();
                args.putInt(DummySectionFragment.ARG_SECTION_NUMBER, position + 1);
                fragment.setArguments(args);
                return fragment;
            }
        }
        return null;
        }

        @Override
        public int getCount() {
            // Show 4 total pages.
            return 4;
        }

        @Override
        public CharSequence getPageTitle(int position) {
            Locale l = Locale.getDefault();
            switch (position) {
            case 0:
                return getString(R.string.title_section1).toUpperCase(l);
            case 1:
                return getString(R.string.title_section2).toUpperCase(l);
            case 2:
                return getString(R.string.title_section3).toUpperCase(l);
            case 3:
                return getString(R.string.title_section4).toUpperCase(l);
            }
            return null;
        }
    }

    /**
     * A dummy fragment representing a section of the app, but that simply
     * displays dummy text.
     */
    public static class DummySectionFragment extends Fragment {
        /**
         * The fragment argument representing the section number for this
         * fragment.
         */
        public static final String ARG_SECTION_NUMBER = "section_number";

        public DummySectionFragment() {
        }

        @Override
        public View onCreateView(LayoutInflater inflater, ViewGroup container,
                Bundle savedInstanceState) {
            View rootView = inflater.inflate(
                    R.layout.fragment_dummy, container, false);
            TextView dummyTextView = (TextView) rootView
                    .findViewById(R.id.section_label);
            dummyTextView.setText(Integer.toString(getArguments().getInt(
                    ARG_SECTION_NUMBER)));
            return rootView;
        }
    }

    private class JSONParse extends AsyncTask<String, String, JSONArray> {
        private ProgressDialog pDialog;
        @Override
        protected void onPreExecute() {

            super.onPreExecute();
            pDialog = new ProgressDialog(MyMainActivity.this);
            pDialog.setMessage("Getting Data ...");
            pDialog.setIndeterminate(false);
            pDialog.setCancelable(true);
            pDialog.show();
        }
        @Override
        protected JSONArray doInBackground(String... args) {

            JSONParser jParser= new JSONParser();
            JSONArray json =jParser.getJSONFromUrl(url);

            return json;
        }
        @Override
        protected void onPostExecute(JSONArray json) {

            pDialog.dismiss();
            Log.d("JSONARRAY:", json.toString());

            try {
                JSONObject json_data = json.getJSONObject(0);
                jObj= json_data;

                mSectionsPagerAdapter = new SectionsPagerAdapter(
                        getSupportFragmentManager());

                // Set up the ViewPager with the sections adapter.
                mViewPager = (ViewPager) findViewById(R.id.pager);
                mViewPager.setAdapter(mSectionsPagerAdapter);
            }

            catch(JSONException exception) {

                Log.e("ERROR", exception.getMessage());
            }
        }
    }
}

And this is one of my subclassed Fragments:

public class HomeSection  extends Fragment {

    JSONObject _jObj;
    public HomeSection(){}

    public void newInstance(JSONObject jObj) {
        Bundle args = new Bundle();

        _jObj= jObj;

        try{
            String content= _jObj.getString("descrizione");
            args.putString("description", content);
        }
        catch(JSONException exception){

            Log.e("ERROR JSON HOME", exception.getMessage());
        }

        // Put any other arguments
        this.setArguments(args);
    }

    @Override
    public View onCreateView(LayoutInflater inflater, ViewGroup container,
            Bundle savedInstanceState) {
        View rootView = inflater.inflate(
                R.layout.fragment_dummy, container, false);
        TextView dummyTextView = (TextView) rootView
                .findViewById(R.id.section_label);
        try {
            dummyTextView.setText(_jObj.getString("descrizione"));
        }
        catch(JSONException exception){

            Log.e("ERROR JSON HOME", exception.getMessage());
        }

        return rootView;
    }
}

It works, but I would like if it is a correct way to populate the views in my app. Then I didn't know if in the HomeSection class, the Bundle that I created in the newIstance method is effectively necessary because I set my text in the view within the method onCreateView.

\$\endgroup\$
3
\$\begingroup\$

I'm not too familiar with Android, so just some generic notes about the Java code:

  1. Instead of using magic numbers in the switch-case use named constants. It would help maintenance a lot. There is more than one switch-case with the same 0-3 case branches. A reader might wonder: are they the same? Help them!

    Might be useful here too:

  2. @Override
    public CharSequence getPageTitle(int position) {
        Locale l = Locale.getDefault();
        switch (position) {
        case 0:
            return getString(R.string.title_section1).toUpperCase(l);
        case 1:
            return getString(R.string.title_section2).toUpperCase(l);
        case 2:
            return getString(R.string.title_section3).toUpperCase(l);
        case 3:
            return getString(R.string.title_section4).toUpperCase(l);
        }
        return null;
    }
    

    Is it considered to be an error in the program if position is not between 0 and 3? If it is there might be no reason to continue processing with wrong state. Consider throwing an exception or logging the error. (The Pragmatic Programmer: From Journeyman to Master by Andrew Hunt and David Thomas: Dead Programs Tell No Lies.)

  3. Some duplication could be eliminated from the switch-case above with a helper method:

    @Override
    public CharSequence getPageTitle(int position) {
        switch (position) {
        case 0:
            return getUpperCaseString(R.string.title_section1);
        case 1:
            return getUpperCaseString(R.string.title_section2);
        case 2:
            return getUpperCaseString(R.string.title_section3);
        case 3:
            return getUpperCaseString(R.string.title_section4);
        }
        return null;
    }
    
    private CharSequence getUpperCaseString(final String input) {
        final Locale locale = Locale.getDefault();
        return getString(input).toUpperCase(locale);
    }
    
  4. jObj should have have a more descriptive name. What's the purpose of this field? Name it accordingly.

  5. A name constant instead of 4 (TOTAL_PAGES_COUNT, for example) could eliminate the comment here:

    @Override
    public int getCount() {
        // Show 4 total pages.
        return 4;
    }
    
  6. Field and variable naming is not consistent:

    ViewPager mViewPager;
    ...
    JSONObject _jObj;
    ...
    JSONObject json_data = json.getJSONObject(0);
    

    The m prefixes to access fields are not really necessary and it's just noise. Modern IDEs use highlighting to separate local variables from fields.

    Field names should not start with underscore. See Effective Java, 2nd edition, Item 56: Adhere to generally accepted naming conventions and The Java Language Specification, Java SE 7 Edition, 6.1 Declarations:

    Names of fields that are not final should be in mixed case with a lowercase first letter and the first letters of subsequent words capitalized.

|improve this answer|||||
\$\endgroup\$

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.