patternjavaMinor
Populate views in Android application
Viewed 0 times
viewspopulateandroidapplication
Problem
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 cl
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 cl
Solution
I'm not too familiar with Android, so just some generic notes about the Java code:
-
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:
-
Is it considered to be an error in the program if position is not between
-
Some duplication could be eliminated from the switch-case above with a helper method:
-
-
A name constant instead of
-
Field and variable naming is not consistent:
The
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.
-
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:
- Refactoring: Improving the Design of Existing Code by Martin Fowler: Replacing the Conditional Logic on Price Code with Polymorphism
- Replace Conditional with Polymorphism
-
@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.)-
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);
}-
jObj should have have a more descriptive name. What's the purpose of this field? Name it accordingly.-
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;
}-
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.
Code Snippets
@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;
}@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);
}@Override
public int getCount() {
// Show 4 total pages.
return 4;
}ViewPager mViewPager;
...
JSONObject _jObj;
...
JSONObject json_data = json.getJSONObject(0);Context
StackExchange Code Review Q#37726, answer score: 3
Revisions (0)
No revisions yet.