Generic Class where Type Parameter extends Nested Class

[marten koomen]

Hi

I have a pattern (not an evil design pattern, just something that recurs in my design) in my program where an abstract class ThingManager is responsible for managing a collection of Thing which is also an abstract class nested in the ThingManager so that it controls all the mutation on Thing.

So in Eclipse this comes up as okay

public abstract class ThingManager<T> {
	
	private Collection<T> things;
	
	public abstract class Thing {
		
		private Thing() {
			
		}
		//private setters
		//public getters
	}

}

but what I really want is for the Type parameter to be an extension of Thing, as below, but Eclipse tells me that Thing cannot be resolved to a type

public abstract class ThingManager<T extends Thing> {
	
	private Collection<T> things;
	
	public abstract class Thing {
		
		private Thing() {
			
		}
		//private setters
		//public getters
	}
}

I can work around this as the code is safe, but the work around suppresses warnings, uses wildcards, and use the abstract class as type parameters, This is not neat. It would be cool to just have the one parameter <T extends Thing>.

Any suggestions?

Thanks

[Paul Clapham]

I fiddled around a bit and came up with this:

public abstract class ThingManager<T extends ThingManager<T>.Thing>

[Carey Brown]

Why are you trying to nest Thing inside of ThingManager? It works fine if you un-nest it and put it in its own file.

[marten koomen]

Thanks Paul, that is so cool, I would never have thought of that. I implemented it in my class and it now looks so much cleaner and easier to read.
Cheers

[Paul Clapham]

My reasoning was like this:

1. It's a nested class so its name is actually ThingManager.Thing.

2. Try that instead: yes, it compiles. But with a warning that it needs to be parameterized.

3. Parameterize it according to Eclipse's suggestion.

Bingo!

[marten koomen]

Hi Carey

I am using a nested class, and note that it is not static so that each Thing always belongs to its ThingManager. Thing is shared by multiple clients who are able to mutate Thing, but they need to do this through synchronized methods in the ThingManager which is responsible for maintaining the integrity of the Thing collection. So only the ThingManager can mutate Thing when methods are called by clients, but all clients can see the instances of Thing and access its getters whenever they want.

This was my reasoning.

[marten koomen]

Just for completion, I'm showing here the abstract parent class and the concrete child class with its generic types resolved (which took a bit of time). Both these work for me. A tad excited by this.


Abstract Class

public abstract class ThingManager<T extends ThingManager<T>.Thing> {

	private Collection<T> things;

	public abstract class Thing {

		protected Thing() {

		}
		// private setters
		// public getters
	}
}

Concrete subclass

public class ConcreteManager<T extends ThingManager<T>.Thing> extends ThingManager<T> {

	public class ConcreteThing extends ThingManager<ConcreteThing>.Thing {

		public ConcreteThing() {
			super();
			// TODO Auto-generated constructor stub
		}

	}
}

[Paul Clapham]

That's getting pretty close to "Generics Gone Wild" but if it does what generics are supposed to do and doesn't make your code unmanageable then, more power to it.

[marten koomen]

actually it's not without its problems, while it is  neat in the abstract ThingManager it get's a bit more complex in the ConcreteManager. Nevertheless it's not an academic exercise, and for what I'm trying to do, which is to manage lots of different types of Things in a consistent way, it seems an appropriate solution.

[Campbell Ritchie]

If you are only using nested classes, why is the top‑level class parameterised at all? What would be wrong with the following?

public abstract class ThingManager {
 
    private Collection<T extends Thing> things;
 
    public abstract class Thing {
 
        protected Thing() {
 
        }
        // private setters
        // public getters
    }
}

You would probably need to parameterise any methods using that Collection.
Why are you making an inner class public? Why didn't you make that nested class static?

[addition]Why have you got private setters? Wouldn't methods that add something to your Collection have to be public? Not that I would make them into straight “setters” myself.

[marten koomen]

sorry about the edit, i wanted to avoid giving people a bum steer, now I've given them two, doh.

Yours (Campbell) seems like another worthwhile solution that I'm exploring now, the sub class is tricky with the original solution.

You would probably need to parameterise any methods using that Collection.

yes, clients get defensive copies of the Collection, and I'm trying to give them assurance of getting the Type they are expecting. But I haven't implementing that yet.

Why are you making an inner class public

The inner class is public because these are the objects (Things) used by clients, the clients render the Collection in an interface, and the client selects which object(Thing) to mutate and does so through the manager (ThingManager).

Why didn't you make that nested class static?

The ThingManager is a Singleton (or at least is a member of Singleton), and each Thing is attached to its ThingManager.  I've tried to get my head around Joshua Bloch's Item 24 on this. I've revisted now on your questioning and have changed the nested class to static. This works. So the inner class is now static and the class declaration looks like this - although I'm still playing around with the subclass

public abstract class ThingManager<R extends ThingManager.Thing>

Why have you got private setters? Wouldn't methods that add something to your Collection have to be public?

Each Thing is available to multiple clients (through a web interface) where each client can make changes to Thing objects asynchronously, I have designed for changes to be made through a manager to ensure changes from multiple clients are made in an orderly fashion.  The ThingManager is responsible for ensuring integrity of the database to which all changes are instantaneously written.

I also have rogue collections floating about (containing Thing objects marked as deleted but presented to clients after a search for possible un-deletion). So a ThingManager helps to keep track of this in single a Class.

I generally don't know if I'm just not programming very well, or if my domain is somewhat unique, I suspect a bit of both.

[marten koomen]

Unfortunately I can't get this to work

public abstract class ThingManager {
  
    private Collection<T extends Thing> things;
  
    public abstract class Thing {
  
        protected Thing() {
  
        }
        // private setters
        // public getters
    }
}

on line 3, Eclipse says Syntax error on token "extends", , expected

[Stephan van Hulst]

void doSomething() {
  doSomethingElse(int foo = 3);
}

You are doing with generic type parameters what I did with the variable foo in the snippet above. You can't declare a variable in the same place where you use the variable.

Either declare T in your class header, or use a wildcard instead.

[marten koomen]

I think that is the problem (to which Stephan is pointing out) that is inherent in the design. I've now minimised generics, made the inner classes static (why not), and will pass Collections onto clients Typed to the abstract (Thing) class, where it makes more sense for clients to cast them into their concrete types.

I'm happy with the encapsulation that the inner class for Thing provides, and while the typing is not perfect, it's pretty straightforward with minimal use of parameterised types.
Thanks

[Stephan van Hulst]

If you wanted to use a nested class structure, I'd invert the nesting relationship. It reduces redundancy in fully qualified identifiers:

abstract class Thing {

  ...

  static abstract class Manager<T extends Thing> {
    
    private final Collection<T> things;

    ...

  }
}

[Campbell Ritchie]

marten koomen wrote:sorry about the edit . . .

Apologies accepted

. . . defensive copies . . .

Defensive copies are a good idea

The inner class is public because these are the objects (Things) used by clients . . .

In which case, I shall repeat that your nested class should probably be static.

. . . changed the nested class to static. . . .

I should have read that sentence before saying the nested class should be static!

We always worry when we hear about singletons. Not sure I understand the last part of your reply: s

This works. So the inner class is now static and the class declaration looks like this - although I'm still playing around with the subclass

public abstract class ThingManager<R extends ThingManager.Thing>

Why have you got private setters? Wouldn't methods that add something to your Collection have to be public?

Each Thing is available to multiple clients (through a web interface) where each client can make changes to Thing objects asynchronously, I have designed for changes to be made through a manager to ensure changes from multiple clients are made in an orderly fashion.  The ThingManager is responsible for ensuring integrity of the database to which all changes are instantaneously written.

I also have rogue collections floating about (containing Thing objects marked as deleted but presented to clients after a search for possible un-deletion). So a ThingManager helps to keep track of this in single a Class.

I generally don't know if I'm just not programming very well, or if my domain is somewhat unique, I suspect a bit of both.

orry. I see Stephan has also replied with a better solution.

[marten koomen]

So Stephan's suggestion of inverting the nesting relationship solved the problem of parameter references neatly. This design requires the ThingManager to be static, as only one instance of manager needs to be created, not one manager for every Thing.

However, the following code raises two issues.
1) I cannot create a new ConcreteThing from within the nested manager class, with Eclipse giving an error on line 13 of the ContreteThing to that effect.
2) Access to the Thing object by the ThingManager is not private, with the sayHello() only visible to the manager when its modifier is public or protected, but not private (this can be worked around)

abstract class Thing<T extends Thing<T>> {

	public Thing() {
		super();
	}
	protected void sayHello() {
	}

	static abstract class ThingManager<T extends Thing<T>> {

		public ThingManager() {
			super();
		}
		public void addnewThing() {
			T newThing = getNewThing();
			newThing.sayHello();
		}
		public abstract T getNewThing();
	}
}

public final class ContreteThing extends Thing<ContreteThing> {

	public ContreteThing() {
		super();
	}
	public static class ContreteThingManager<ConcreteThing> extends ThingManager<ContreteThing> {	

		public ContreteThingManager() {
			super();
		}
		@Override
		public ContreteThing getNewThing() {
			return new ConcreteThing(); //error here
		}
	}
}

The JavaDocs on nested classes here states that.

Note: A static nested class interacts with the instance members of its outer class (and other classes) just like any other top-level class. In effect, a static nested class is behaviorally a top-level class that has been nested in another top-level class for packaging convenience.

So I'm back to the issue of a static nested class or non-static inner class. My initial choice to go with a non-static inner class was the conceptually strong one(Manager)-to-many(Thing) relationship in the design, a Thing is conceptually not a top-level-class (to me). At this point I cannot understand other's preference for a static inner class given the strength of the relationship, that's not to say there are not good reasons, it's just that I don't understand them as yet.

So this is is then about trade-offs, with encapsulation being my primary consideration.

Campbell wrote

We always worry when we hear about singletons. Not sure I understand the last part of your reply:

I'm using singleton conceptually here to represent a tenant, I have a Collection of tenants maintained in a Singleton as per Item 3 of Joshua Bloch through an enum.
My design has multiple tenants where each Tenant has many Collections of different types of Thing and multiple users that can read or mutate Thing objects that are shared. Tenants can also exchange collections of Thing objects. So external to the ThingManager activity is not very controlled, and I need to encapsulate Thing mutations in a manager so that I can ensure integrity of the database and the view of clients as much as possible.

As to my last line

I generally don't know if I'm just not programming very well, or if my domain is somewhat unique, I suspect a bit of both.

I have said this in other places, but my domain is education where I have seen (in a role in bureaucracy) big projects founder or deemed ill-suited by teachers. My project is exploring this, so when I run into problems I don't know if it's my lack of programming or issues unique to the domain.

[Paul Clapham]

        public ContreteThing getNewThing() {
            return new ConcreteThing(); //error here
        }

Is that error not just because you misspelled ContreteThing in line 13? (Or alternatively because you misspelled ConcreteThing everywhere else?)

[marten koomen]

Sorry Paul you are right, it's a typo, doh.  It get confusing doing multiple changes. Apologies again.  I will not edit the above this time

[marten koomen]

ok, so inverting the nesting works, thanks Stephan.  Thing is now the outer class, Manager the inner class which is static.  Type parameters look normal, all warnings removed without suppression.

I'll now contemplate what I did, and build my suite of Thing sub-classes.

many thanks to all.

[Stephan van Hulst]

You have several issues in the last code you posted.

Thing's generic type parameter T is unused. Remove it.

You have public and protected members in a package private class.

Your constructors are redundant.

ConcreteThingManager's name can be shortened to Manager, because ConcreteThing already provides a separate namespace for it.

ConcreteThingManager's class header doesn't do what you think it does. Instead of using the class ConcreteThing, you introduced a new type variable named ConcreteThing.

So, you can remove the type parameters from all your outer 'thing' classes. However, there are some scenarios where you really DO want to use the nested class in the header of the enclosing class (such as when the enclosing class extends the nested class, or you want to use the nested class as a type bound for a type variable of the enclosing class).

If so, you have two options. First, use the fully qualified name of the nested class when you use it in the header of the enclosing class:

final class Enclosing<T extends com.example.Enclosing.Nested> {

Make an explicit import statement:

import static com.example.Enclosing.Nested;

final class Enclosing<T extends Nested> {

In this particular case though, the 'thing' classes provide a good namespace for the 'manager' classes, so it's neatly solved by inverting the nesting relationship.

Don't use the singleton pattern. Just using the word singleton should send shivers up your spine. If a class is only allowed to interact with one particular instance of another class, just inject that instance in its constructor. Lookups from static variables must be avoided.

[marten koomen]

Thing's generic type parameter T is unused. Remove it.
You have public and protected members in a package private class.
Your constructors are redundant.
ConcreteThingManager's name can be shortened to Manager, because ConcreteThing already provides a separate namespace for it.
ConcreteThingManager's class header doesn't do what you think it does. Instead of using the class ConcreteThing, you introduced a new type variable named ConcreteThing.

When I look at these concerns in my actual code where my Thing (hereafter called Resource) class is over 1000 lines long, I can't find a way of getting around the type headers. Below are some snippets from the actual code, now called Resource to avoid transcription errors.

public abstract class Resource<R extends Resource<R>> 
// static sql stuff
//.Resource fields
protected Resource(ResultSet resultSet) throws SQLException{}
// read in fields, instantiate object from database
// method pattern like this below, getters are public and setters protected and only available to sub and nested classes, private does not work for the nested top-level, and I'm puzzled that protected does, but protected does the job
	public boolean isOnPalette() {
		return isOnPalette;
	}

	protected void setOnPalette(boolean isOnPalette) {
		this.isOnPalette = isOnPalette;
	}
	public static abstract class Manager<R extends Resource<R>> {
//...
		protected final Class<R> resourceClass; //used in calls to tenant to signal the type of Resource  
		private final Map<Integer, R> onPaletteResources = new HashMap<Integer, R>();
		protected final ActiveTenant tenant;  // the Manager has access to the tenant to assist with coordination across Resources
		public Manager(ActiveTenant tenant, String tablename, Collection<SqlField> subClassSqlFields,
				Class<R> resourceClass, HasDelegates hasDelegates) throws TTException {
			super();
                //the constructor mainly prepares Sql strings for prepared statements, based on SqlFields supplied by the subclass
}
		protected abstract R buildResource(ResultSet resultSet) throws SQLException;} //new instances of Resources need to be created by the subclass
// methods along this pattern
		protected final void addResource(R resource) {
			onPaletteResources.put(resource.getResourceId(), resource);
		}

		protected final R getResource(int resourceId) {
			return onPaletteResources.get(resourceId);
		}
		public synchronized final List<R> getOnPaletteResources() {
			// make defensive copy
			return new ArrayList<R>(onPaletteResources.values());
		}

I trust this gives a flavour. I'm keen to get this right because my first subclass is also ~800 lines. The Resource class crosses the nexus of MC of my MVC, where client classes are responsible for generating the view.
The code now looks great (from my perspective) without any warnings, and where type wildcards<?> make sense.

So I've shorten the name to Manager, but when I play around with the Type headers Eclipse becomes a sea of red.

Just using the word singleton should send shivers up your spine

okay, I understand this (I think), thanks

[Stephan van Hulst]

You need to remove the type parameter R from Resource, because you're not using it anywhere (except in the self-referential type bound on R itself). You must leave it on your Manager class, because you actually use it there.

Don't do database stuff from constructors. Constructors must be lightweight. Let factory methods retrieve properties from a database and make the constructor accept the properties you want to set.

Don't use integers for IDs. Even if your database uses auto-incrementing integer IDs, treating them like integers in your code doesn't give you any benefit, and you will kick yourself if you one day decide to switch to GUIDs or something similar. Either use String, Object, or a generic type variable.

Don't expose fields. Sub-classes may access private fields though protected accessors.

Use upper type bounds when accepting a collection from which to retrieve elements.

What is a TTException? Use descriptive names.

Return an Optional from methods instead of null.

Instead of returning defensive copies from methods, return unmodifiable views. The client may then choose to make a copy of the returned view if they want to do more than just quickly inspect it.

What Java version are you on? You should be able to call private methods from nested classes and you can use the diamond operator to infer type arguments from the assignment context.

public abstract class Resource<K> {

  private final K key;

  private final Manager<K, ?> manager;

  protected Resource(K key, Manager<K, ?> manager) {
    if (key == null || manager == null)
      throw new IllegalArgumentException();

    this.key     = key;
    this.manager = manager;
  }

  public final boolean isOnPalette() {
    return this == manager.resourcePalette.get(key);
  }

  public static abstract class Manager<K, R extends Resource<K>> {

    private final Class<R> resourceType;

    private final Map<K, R> resourcePalette = new HashMap<>();

    protected Manager(Class<R> resourceType, HasDelegates hasDelegates) {
      ...
    }

    protected abstract R buildResource(ResultSet resultSet) throws ResourceBuildingException;

    protected final void fillPalette(ActiveTenant tenant, String resourceTableName, Collection<? extends SqlField> resourceFields) throws PaletteFillingException {
      ...
    }

    protected final Optional<R> putResourceOnPalette(R resource) {
      return Optional.ofNullable(resourcePalette.put(resource.key, resource));
    }

    public final Map<K, R> getResourcePalette() {
      return unmodifiableMap(resourcePalette);
    }
  }
}

[marten koomen]

Thanks Stephan

You need to remove the type parameter R from Resource, because you're not using it anywhere (except in the self-referential type bound on R itself). You must leave it on your Manager class, because you actually use it there.

yes, carry over from my first prototype, don't know why I did it then.

Don't use integers for IDs.

I've changed the key to a String, Although I can't for the moment figure out how to abstract a Key into a class, will work on it.

Don't expose fields. Sub-classes may access private fields though protected accessors.

Will tidy up

What Java version are you on?

from eclipse -> C:/Program Files/Java/jdk1.8.0_102/jre/bin\server\jvm.dll

Don't do database stuff from constructors. Constructors must be lightweight. Let factory methods retrieve properties from a database and make the constructor accept the properties you want to set.

In principle I agree here, but for my first prototype I got into all sorts of problems abstracting out the database from the objects. In this design the only way a Resource object is instantiated is when it's read from the database, so it somehow makes sense to pass the Result Set into the constructor. But I'll contemplate this. This project is for a very relational domain, so the objects doing the work all represent complete rows in a database table. Also in my first prototype I got into all sorts of problems creating unmodifiable Resources and other objects containing Resource state. I got into a big mess. I'm trying to avoid that here by encapsulating everything in a manager.

Instead of returning defensive copies from methods, return unmodifiable views

will do, but for the moment I know my clients will want to sort a list and present it.

What is a TTException?

Probably not the best solution, but this is a custom made exception that I use to swallow exceptions thrown by database interactions etc along with a severity indicator. TTException is then thrown so that it can propagate to the dispatcher/controller Servlet can then decide what to do and how - e.g. terminate the session or inform the client that a transaction has not been completed.  I understand this is not ideal, but I don't know how else to treat database exceptions elegantly for the browser based client.

Return an Optional from methods instead of null

I've been working to Joshua Bloch's Item 54, to not return nulls but empty collections, I'll need to double check that.

Use upper type bounds when accepting a collection from which to retrieve elements.

Will do, I think I know what this means, it sounds similar Joshua Bloch Item 31, of using bounded wildcards, so I'm understanding accept a Collection when that will do, rather an List.

I don't understand why Resource needs a reference to the Manager, as it is the Manager that initiates and coordinates mutations to the Resource along with informing any other involved resources. I'll need to get my head around this.

[Stephan van Hulst]

marten koomen wrote:I've changed the key to a String, Although I can't for the moment figure out how to abstract a Key into a class, will work on it.

String is fine.

jdk1.8.0_102

You should then be able to use the diamond operator.

I understand this is not ideal, but I don't know how else to treat database exceptions elegantly for the browser based client.

Writing a custom exception to wrap low-level exceptions is good. My issue was with how descriptive the wrapper exception was. You could write one base exception class for your problem domain, and then write simple subtypes for the various things that could go wrong, such as filling the palette and building a resource.

I've been working to Joshua Bloch's Item 54, to not return nulls but empty collections, I'll need to double check that.

Yes, this is similar. Return an empty collection when a method can return a collection of objects and you don't have any results for a particular invocation of the method. Optional is like a collection, except it's for a single object.

I'm understanding accept a Collection when that will do, rather an List.

Yes, but what I meant is that you should accept a Collection<? extends Whatever> when you're only using the collection to retrieve objects from it. Conversely, use Collection<? super Whatever> if you're only using the collection to put objects into it. The latter isn't common for collections, but you will want to use lower bounds when your method accepts classes such as Comparator. <br /> <br />

I don't understand why Resource needs a reference to the Manager, as it is the Manager that initiates and coordinates mutations to the Resource along with informing any other involved resources. I'll need to get my head around this.

<br /> I did this because my interpretation of the isOnPalette property led me to believe that having this field in Resource provides an opportunity for data inconsistency when its boolean value doesn't agree with whether the object is contained by the palette that the Manager manages. I normalized it by having the accessor return whether the resource was contained by the palette of the resource's manager. <br /> <br /> However, just as how elements don't know that they're contained by a set, it's probably best that a resource doesn't know that it's managed by a manager. I question the wisdom of having an isOnPalette property in the Resource class, if my interpretation of it is correct. Instead, clients should get this property by querying the manager.