I am building an Android application and there are five Activity
classes or if you're familiar with the MVC pattern they would usually be the Controller
classes.
Specifically a User
will enter one of these 5 Activity
classes (by navigating throughout the app) and sometimes they might upload a photo. Now the code for uploading a photo follows a very similar pattern. Please note all this code is repeated 5 times in all 5 classes (YUCK).
Global Variables:
/*
Tracking
*/
private static final int TAKE_PHOTO_REQUEST = 1;
private static final int GET_FROM_GALLERY = 2;
private Uri mUri;
private String mCurrentPhotoPath;
private File mFile;
private TypedFile mTypedFile; // For Retrofit
User hits Photo Upload Button, and a AlertDialog pops up:
private void showFileOptions() {
new AlertDialog.Builder(this)
.setItems(R.array.uploadOptions, new DialogInterface.OnClickListener() {
@Override
public void onClick(DialogInterface dialog, int which) {
switch (which) {
case 0:
dispatchTakePicture();
break;
case 1:
dispatchUploadFromGallery();
break;
}
}
})
.setNegativeButton(android.R.string.cancel, new DialogInterface.OnClickListener() {
@Override
public void onClick(DialogInterface dialog, int which) {
dialog.cancel();
}
})
.show();
}
dispatchTakePicture:
/*
Take picture from your camera
*/
private void dispatchTakePicture() {
Intent intent = new Intent(MediaStore.ACTION_IMAGE_CAPTURE);
// Make sure that there is a camera activity to handle the intent
if (intent.resolveActivity(getPackageManager()) != null) {
// Create the File where the mTypedFile would go
File picFile = null;
try {
picFile = createImageFile();
mFile = picFile;
} catch (IOException e) {
e.printStackTrace();
Toast.makeText(this, e.getMessage(), Toast.LENGTH_LONG).show();
}
// Continue only if the file was successfully created
if (picFile != null) {
intent.putExtra(MediaStore.EXTRA_OUTPUT, Uri.fromFile(picFile));
startActivityForResult(intent, TAKE_PHOTO_REQUEST);
}
}
}
dispatchUploadFromGallery:
/*
Take a mTypedFile from your gallery
*/
private void dispatchUploadFromGallery() {
// Launch gallery intent
startActivityForResult(new Intent(Intent.ACTION_PICK, MediaStore
.Images.Media.INTERNAL_CONTENT_URI), GET_FROM_GALLERY);
}
Please note that startActivityForResult
gets called in both of these methods. Next up is the createImageFile()
method if the user wants to take a picture from the Camera
API:
private File createImageFile() throws IOException {
// Create the Image File name
String timeStamp = new SimpleDateFormat("yyyyMMdd_HHmmss", Locale.getDefault()).format(new Date());
String imageFileName = "JPEG_" + timeStamp + "_";
File storageDir = Environment
.getExternalStoragePublicDirectory(Environment.DIRECTORY_PICTURES);
File image = File.createTempFile(
imageFileName, // Prefix
".jpg", // Suffix
storageDir // Directory
);
// Save the file, path for ACTION_VIEW intents
mCurrentPhotoPath = "file:" + image.getAbsolutePath();
mUri = Uri.fromFile(image);
return image;
}
Now finally our startActivityForResult(...)
method:
@Override
protected void onActivityResult(int requestCode, int resultCode, Intent data) {
super.onActivityResult(requestCode, resultCode, data);
if (requestCode == TAKE_PHOTO_REQUEST && resultCode == RESULT_OK) {
startUploadProgress();
showContainer();
mTypedFile = new TypedFile("image/*", mFile);
RotatePictureHelper.rotatePicture(mFile, ExampleActivity.this, mAttachment); // Helper class to rotate pictures
mBus.post(new LoadUploadFileEvent(mTypedFile));
} else if (requestCode == GET_FROM_GALLERY && resultCode == RESULT_OK) {
startUploadProgress();
showContainer();
mUri = data.getData();
mTypedFile = UriHelper.handleUri(mUri, this); // Helper class to handle bitmap manipulation
mFile = mTypedFile.file();
mBus.post(new LoadUploadFileEvent(mTypedFile));
} else if (resultCode != Activity.RESULT_CANCELED) {
Toast.makeText(this, R.string.generalError, Toast.LENGTH_LONG).show();
}
}
Note that I have already created helper classes to handle bitmap manipulation and picture rotation issues.
STILL, this is very ugly ugly code, and to have this repeated in 5 classes.
I have a few ideas in mind right now:
- Create a service and pass in needed variables to that service to handle this.
- Moving
AlertDialog
options to a helper class and call differentAlertDialogs
based oninstanceOf
whateverActivity
is calling it. - Should I create a parent
Activity
class that has these methods and then extend the 5Activity
child classes and call these methods?
2 Answers 2
If it was me, I should go for the third option.
Make an abstract superclass for your Activity
where you can put this code into.
If some points of the method difference, you can refactor that part to an abstract method, so you can implement this part custom in each class.
Now, up to a little comments on your code.
Overall, pretty nice code with a lot of comments.
Some of the comments are not needed, while other are sure on their place so you can understand it quickly.
Their are 2 small things what I see and I prefer to see otherwise.
- Switch => implement always default case.
e.printStackTrace();
, please use a logger instead.
Edit after your answer :
What I see now is that you call the super
of onActivityResult
and that's good.
The only thing what is bothering me now is that you repeat the case after the super.
I don't know if you can make of CameraActivity
an abstract class, if so try the following :
@Override
protected void onActivityResult(int requestCode, int resultCode, Intent data) {
super.onActivityResult(requestCode, resultCode, data);
startProgress();
switch (requestCode) {
case TAKE_PHOTO_REQUEST:
if (resultCode == RESULT_OK) {
mTypedFile = new TypedFile("image/*", mFile);
doTakePhotoRequest();
}
break;
case GET_FROM_GALLERY:
if (resultCode == RESULT_OK) {
mUri = data.getData();
mTypedFile = UriHelper.handleUri(mUri, this);
doFromGallery();
}
break;
default:
stopProgress();
Toast.makeText(this, R.string.generalError, Toast.LENGTH_LONG).show();
break;
}
}
protected abstract void doTakePhotoRequest(); // give params if needed
protected abstract void doFromGallery(); // give params if needed
Like this you just implement these methods and you don't even have to override onActivityResult
.
If it's not possible for an abstract class => just implement those methods empty and you can override them later.
-
\$\begingroup\$ Can you explain where I should use switch instead? Also I am writing up an answer right now, if you care to look at it \$\endgroup\$AndyRoid– AndyRoid2015年07月14日 05:25:53 +00:00Commented Jul 14, 2015 at 5:25
-
\$\begingroup\$ Points to take note of, I created a
CameraActivity
class to hold the code and it is extending off of anotherabstract
class that holds myEventBus
information. \$\endgroup\$AndyRoid– AndyRoid2015年07月14日 05:26:46 +00:00Commented Jul 14, 2015 at 5:26 -
\$\begingroup\$
private void showFileOptions()
contains a switch statement \$\endgroup\$chillworld– chillworld2015年07月14日 05:27:20 +00:00Commented Jul 14, 2015 at 5:27 -
\$\begingroup\$ Oh you are implying I do not have a default case right? Another point to take note I could probably use a
switch
statement insideonActivityResult()
too. \$\endgroup\$AndyRoid– AndyRoid2015年07月14日 05:34:28 +00:00Commented Jul 14, 2015 at 5:34 -
\$\begingroup\$ Indeed, that's also part of this community, try to make the code better by following some general rules ;) \$\endgroup\$chillworld– chillworld2015年07月14日 05:36:07 +00:00Commented Jul 14, 2015 at 5:36
To solve this, I went with my 3rd option:
I created an activity called CameraActivity
and then extended this Activity
in my other 5 Activity
classes. The code that each of these classes had was 95% similar with maybe 5% differences each respectively.
My CameraActivity
would have the following code:
public class CameraActivity extends Activity {
/*
Tracking
*/
private static final int TAKE_PHOTO_REQUEST = 1;
private static final int GET_FROM_GALLERY = 2;
private Uri mUri;
private String mCurrentPhotoPath;
private File mFile;
private TypedFile mTypedFile; // For Retrofit
@Override
protected void onActivityResult(int requestCode, int resultCode, Intent data) {
super.onActivityResult(requestCode, resultCode, data);
startProgress();
switch (requestCode) {
case TAKE_PHOTO_REQUEST:
if (resultCode == RESULT_OK) {
mTypedFile = new TypedFile("image/*", mFile);
}
break;
case GET_FROM_GALLERY:
if (resultCode == RESULT_OK) {
mUri = data.getData();
mTypedFile = UriHelper.handleUri(mUri, this);
}
break;
default:
stopProgress();
Toast.makeText(this, R.string.generalError, Toast.LENGTH_LONG).show();
break;
}
}
protected void showFileOptions() {
new AlertDialog.Builder(this)
.setItems(R.array.uploadOptions, new DialogInterface.OnClickListener() {
@Override
public void onClick(DialogInterface dialog, int which) {
switch (which) {
case 0:
dispatchTakePicture();
break;
case 1:
dispatchUploadFromGallery();
break;
default:
dispatchUploadFromGallery();
break;
}
}
})
.setNegativeButton(android.R.string.cancel, new DialogInterface.OnClickListener() {
@Override
public void onClick(DialogInterface dialog, int which) {
dialog.cancel();
}
})
.show();
}
// Other code for handling Uri and File goes here....
}
Now I can extend this Activity
in my other Activity
classes and implement the rest of the 5% differences there. Take note that showFileOptions()
has changed to protected
status so I can call it from a child Activity
. As an example here is one activity.
public class PhotoUploadActivity extends CameraActivity {
// Initialize methods
@Override
public void onCreate(Bundle savedInstanceState) {
super.onCreate(savedInstanceState);
showFileOptions(); // Calling AlertDialog from Parent Activity
}
@Override
public void onActivityResult(int requestCode, int resultCode, Intent data) {
// Super call executes code in CameraActivity
super.onActivityResult(requestCode, resultCode, data);
switch (requestCode) {
case TAKE_PHOTO_REQUEST:
if (resultCode == RESULT_OK) {
// Implement logic if user take a photo, do something with mUri or mTypedFile
}
break;
case GET_FROM_GALLERY:
if (resultCode == RESULT_OK) {
// Implement logic if user gets something from gallery, do something with mUri or mTypedFile
}
break;
default:
break;
}
}
}
Now I can simply extend my 5 child Activity
classes call super.onActivityResult(...)
in those classes and factor in the remainder 5% code while CameraActivity
handles the rest.
-
\$\begingroup\$ can
CameraActivity
be an abstract class? \$\endgroup\$chillworld– chillworld2015年07月14日 06:08:46 +00:00Commented Jul 14, 2015 at 6:08 -
\$\begingroup\$ if I already have another abstract class that is being extended by
CameraActivity
wouldn't that cause issues? Maybe I should merge both but thatabstract
class is used by otherActivities
as well that do not need a Camera utility. \$\endgroup\$AndyRoid– AndyRoid2015年07月14日 07:17:23 +00:00Commented Jul 14, 2015 at 7:17 -
1\$\begingroup\$ you can extends an abstract class with an abstract class without issues. \$\endgroup\$chillworld– chillworld2015年07月14日 09:35:19 +00:00Commented Jul 14, 2015 at 9:35