1
\$\begingroup\$

I'm making an Android app where users can create posts. This process of creating a new post can potentially be a few activities/screens, so I need a way to save and pass the data between the activities. My first (bad) solution was just to pass the data with the intent in a Bundle, but this soon got very messy as I really needed to pass a Post object between the activities. I switched from that solution to using a Singleton class called, DataHolder.

In every activity, I call DataHolder.getInstance() and when the create post button is clicked, I create a new Post object by executing: DataHolder.getInstance().setPost(new Post()). In the following activities, as the user enters more information about a post, I set the Post's properties.

This was all working well until I ran into this issue. When I would return to my app (presumably it had been killed) I would get a NullPointerException because the Post object was null. I was looking through the Android docs on passing data between activites/services and needed a little help.

Should I be using the Singleton class pattern here? What would be the most efficient and easiest way to pass this Post object between the activities? Should I use Parcelable? (I don't like it though because doesn't it violate the single responsibility principle?)

Here is the DataHolder class:

import com.my_name.app_name.models.Post;

public class DataHolder {

    private static final DataHolder instance = new DataHolder();

    private Post post;
    private Venue venue;
    private int mediaType;
    private Uri imageUri;
    private Uri videoUri;

    // define the media types for a new post here
    public static final int PHOTO = 0;
    public static final int VIDEO = 1;
    public static final int TEXT = 2;
    public static final int PLACE = 3;

    public Uri getImageUri() {
        return imageUri;
    }

    public void setImageUri(Uri iu) {
        this.imageUri = iu;
    }

    public Uri getVideoUri() {
        return videoUri;
    }

    public void setVideoUri(Uri vu) {
        this.videoUri = vu;
    }

    public int getMediaType() {
        return mediaType;
    }

    public void setMediaType(int mt) {
        this.mediaType = mt;
    }

    public Venue getVenue() {
        return venue;
    }

    public void setVenue(Venue v) {
        this.venue = v;
    }

    public Post getPost() {
        return post;
    }

    public void setPost(Post p) {
        this.post = p;
    }

    public static DataHolder getInstance() {
        return instance;
    }
}

I would appreciate any feedback. Let me know if you would like to see more or have any questions.

\$\endgroup\$

closed as off-topic by Simon Forsberg, Mathieu Guindon, syb0rg, palacsint, MrSmith42 Jun 3 '14 at 4:13

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Questions containing broken code or asking for advice about code not yet written are off-topic, as the code is not ready for review. Such questions may be suitable for Stack Overflow or Programmers. After the question has been edited to contain working code, we will consider reopening it." – Simon Forsberg, Mathieu Guindon, syb0rg, palacsint, MrSmith42
If this question can be reworded to fit the rules in the help center, please edit the question.

  • \$\begingroup\$ I have voted to close this question because of the following reasons: 1) You're essentially asking about fixing a problem that your current code does not handle well. Because of the lack of code you have provided, it's possible that that problem is not related to the singleton pattern at all. 2) You haven't provided your Post or Venue classes. \$\endgroup\$ – Simon Forsberg Jun 3 '14 at 0:00
  • \$\begingroup\$ 1) I disagree. I explained the flow of creating a new post in my app, what I attempted and the results of those attempts, and asked for feedback. I don't see how this is outside the scope of code review. 2) I isolated the problem and its not the Post or Venue class. I said if you would like to see more, you could just ask for those classes. I thought the question could be answered without having to see those classes. \$\endgroup\$ – lschlessinger Jun 3 '14 at 0:20
  • \$\begingroup\$ Then I wonder why you mentioned the NullPointerException problem you were having at all. Code Review is not for fixing such problems in your code (that belongs on StackOverflow). Either way, I have re-read your question and added a point in my answer about fragments. At the moment, I think using Fragments is what I would recommend the most. \$\endgroup\$ – Simon Forsberg Jun 3 '14 at 0:33
1
\$\begingroup\$

Fixing a NullPointerException is not under the scope of Code Review, but you should be able to solve that problem by looking at a previous answer of mine where I'm describing How to make a singleton in Android. This might fix your problem. (The short answer is: Use the built-in Application class that Android manages for you)

I have some other comments regarding your questions:

  • Avoid the Singleton pattern in Android everywhere.
  • I personally haven't used Parcelable, but I believe it is the recommended approach. If you think implementing it would violate Single Responsibility Principle, then create a delegating wrapper object that implements Parcelable and just more or less stores a reference to the real object.
  • Or serialize things using the Jackson Library (I wouldn't do this for the simple task of passing data between activities though)
  • Another alternative for you is to use Fragments. By using fragments properly, you could keep the Post state in your activity, and let the Fragments use a callback on your activity to save parts of the post step-by-step, until it is finally finished in which case you perform the final save of it.

As for the rest of the code in your question, I got just a few comments:

  • DataHolder is a too generic name. What kind of data is it holding? Sooner or later you might end up with multiple DataHolders.

  • What if your mediaType would accidentally end up with the value 4? Use an enum to make sure that doesn't happen. And to make your code better. Limited alternatives should always point you to an enum.

    public enum MediaType {
        PHOTO, VIDEO, TEXT, PLACE;
    }
    
\$\endgroup\$
  • \$\begingroup\$ I'm confused about your solution of extending the Application class. Why do you say avoid the singleton pattern yet your answer is "How to make a singleton in Android". Isn't that solution just an application singleton? Also, unfortunately, making the objects implement Parcelable or Serializable is not an option for me because my Post model is extending ParseObject from the Parse SDK. \$\endgroup\$ – lschlessinger Jun 2 '14 at 23:25
  • 1
    \$\begingroup\$ It is an singleton, yes. But the big difference is that Android handles it for you. I do think you should avoid the singleton pattern, yes. But if you really really need it, do it that way. Android keeps a singleton Application object either way, whether you'd like it or not. Use that rather than create a new one. \$\endgroup\$ – Simon Forsberg Jun 2 '14 at 23:56

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