0

I'm trying to create an instance of a class that is used in the constructor of its outer class. Referring to the code below, I need a UserData object but I also need a TimeOnlineInfo object to create it, and I don't see a way of getting TimeOnlineInfo object without first having an instance of UserData because TimeOnlineInfo is not static. I can't make it static though because it needs to access a method from its outer class. Is there anyway I could get this to work or get the most similar effect? I did realize that I could just make the classes static and not save the data directly in the addTime method but i was already halfway through this question and I'm curious to see if there is a way of doing this.

Here is a very simplified version of my code:

class UserData {
    TimeOnlineInfo timeOnline;

    public UserData(Object data1, Object data2, Object data3, Object data4, Object data5, TimeOnlineInfo timeOnlineInfo){
        this.timeOnlineInfo = timeOnlineInfo;
    }

    public class TimeOnlineInfo {
        private int time;

        public TimeOnlineInfo(int time){
           this.time = time;
        }

        public void addTime(int time){
            this.time += time;
            UserData.this.saveData();
        }
    }
}


UserData userData = new UserData(new UserData.TimeOnlineInfo());//Doesn't work because PlayInfo is not a static class
UserData userData = new UserData(userData.new TimeOnlineInfo());//This is just a stupid because i'm using the uncreated object in its own constructor
kmecpp
  • 2,371
  • 1
  • 23
  • 38
  • I don't think this is a particularly good arrangement, if `TimeOnlineInfo` and `UserData` are in a strict 1-to-1 relationship (as they appear to be), why have a separate, publicly visible `TimeOnlineInfo` class in the first place? There could be reasons for that of course, but from what you posted, the easiest solution would be to get rid of `TimeOnlineInfo` altogether. – biziclop Jul 06 '15 at 15:56

3 Answers3

2

You've got some general confusion about a few things. Let's start at the beginning.

First, this isn't a constructor.

public void UserData(TimeOnlineInfo timeOnlineInfo){
    this.timeOnlineInfo = timeOnlineInfo;
}

You meant to drop the void declaration.

public UserData(TimeOnlineInfo timeOnlineInfo){
    this.timeOnlineInfo = timeOnlineInfo;
}

Second, it doesn't make structural sense to instantiate the outer class with an instance of the inner class, since the only way to get an instance of the inner class is to use the outer class to begin with.

For example, you would have to use new UserData().new TimeOnlineInfo(int) for an instance of TimeOnlineInfo, but that's only:

  • if you elected to create a no-arg constructor for UserData
  • if you really didn't care about the instance of UserData you were getting back

If you really want to keep this design, then consider passing in an int to your construtor instead, so it can be supplied to the instance of TimeOnlineInfo.

class UserData {
    TimeOnlineInfo timeOnlineInfo;

    public void saveData() {
        // stub
    }

    public UserData(int value) {
        this.timeOnlineInfo = new TimeOnlineInfo(value);
    }


    public class TimeOnlineInfo {
        private int time;

        public TimeOnlineInfo(int time){
            this.time = time;
        }

        public void addTime(int time){
            this.time += time;
            UserData.this.saveData();
        }
    }
}
Makoto
  • 104,088
  • 27
  • 192
  • 230
  • I would still make the `TimeOnlineInfo` private :) – biziclop Jul 06 '15 at 16:27
  • Was just a typo, I wrote the whole thing in browser. I guess this is probably the best solution even though it kinda hurts readability. I suppose thats my fault though for coming up with such an odd question – kmecpp Jul 06 '15 at 16:30
  • @kmecpp It's not your fault, these sort of problems are difficult to simplify for a post without oversimplifying. – biziclop Jul 06 '15 at 16:38
1

First of all, your UserData constructor isn't one.

You've declared it as a void method, so no UserData constructor would compile, parametrized with a TimeOnlineInfo instance.

But this might only be a typo.

Then, if you can implement a parameterless constructor for UserData on top of the existing one, you could use the following idiom:

UserData ud = new UserData(new UserData().new TimeOnlineInfo(42));

However, this seems to indicate a more general problem within your design, as you're basically having to initialize UserData twice to get a usable instance.

The idea here is, you either inject TimeOnlineInfo in a constructor, in which case TimeOnlineInfo is likely to be used by broader scopes than just UserData, or you only use TimeOnlineInfo within UserData, in which case use an empty constructor for UserData and initialize your internal TimeOnlineInfo within it.

Mena
  • 47,782
  • 11
  • 87
  • 106
  • Yes sorry about that, it was a typo. A parameterless constructor doesn't really seem like a good idea because of your reason but i also don't really want to have the need to create an empty, useless, object aside from its sole purpose of creating a nested class. – kmecpp Jul 06 '15 at 16:07
  • @kmecpp yes it looks like an ugly workaround. That's why I insist on the last paragraph about the design part. – Mena Jul 06 '15 at 16:08
  • 1
    @kmecpp - it's not just useless; it'll link to the wrong outer object. – ZhongYu Jul 06 '15 at 16:17
0

You could change your constructor to this:

public void UserData(int time){
    this.timeOnlineInfo = new TimeOnlineInfo(time);
}

You can also make TimeOnlineInfos constructor private to stop others instantiating rogue TimeOnlineInfo instances.

But based on the snippet you posted, I'd probably get rid of TimeOnlineInfo completely.


I've been warned that what I thought was a constructor isn't one. So first things first, never create regular methods with the same name as the class.

The rest still stands though, the problem with this arrangement is that anyone can create a TimeOnlineInfo object, which will trigger UserData.this.saveData() even if it isn't referenced by the UserData it was created with.

So either:

  1. Make the TimeOnlineInfo instantiation a job for UserData.
  2. Get rid of TimeOnlineInfo. (Not feasible if your class is already big, but then again if your class is that big, you probably need to do other things too.)
  3. Make your inner class static and manage the connection explicitly:

    public void setTimeOnlineInfo(TimeOnlineInfo timeOnlineInfo){
    if (timeOnlineInfo.userData != null) {
        throw new IllegalArgumentException( "TOI already belongs to other UserData" );
    }
     if (this.timeOnlineInfo != null) {
         this.timeOnlineInfo.userData = null;
     }
     timeOnlineInfo.userData = this;
     this.timeOnlineInfo = timeOnlineInfo;
    

    }

    public static class TimeOnlineInfo { private int time; private UserData userData;

     public TimeOnlineInfo(int time){
        this.time = time;
     }
    
     public void addTime(int time){
         this.time += time;
         userData.saveData();
     }
    

    }

Far from ideal, but it would solve a couple of other problems too.

biziclop
  • 48,926
  • 12
  • 77
  • 104