Hi community,
ATM I’m struggeling with the DefaultServer and KernelAdapter design, while I’m trying to integrate some SSLSocketFactory-involving custom SslSelectorKernel and stuff. Being a total JME-noob I’m currently unsure where to put some feedback
I would like to provide some API/architecture feedback and I hope to not piss off somebody, as empathy and social behaviour aren’t my strength. So, I would like to first thank you all for creating JME and keeping it alive and tell you about my API-user-experience:
-
addChannel(int port)seems to be the intended way to add another channel to my server, okay, fine, that’s what I need -
addChannel(int port)useskernelFactory.createKernel(channeIndex, port)for instantiatingKernel- perfect, let’s just pass in my own factory… - …but no constructor argument and no setter for passing my desired factory in? Strange…
Checking the DefaultServer source code, tfw seeing this:
private final KernelFactory kernelFactory = KernelFactory.DEFAULT;
Why is this class member final?
An “immutable default factory” is an oxymoron: factories are “exchangeable objects for flexible instantiations”. If you remove “exchangeable”, you also lose “flexible”.
Okay, next I checked the KernelFactory.DEFAULT-implementation: it turns out it’s implementation, NioKernelFactory, always returns a new SelectorKernel-instance. This is a bit odd, because in this case we have a factory which nobody can replace, always returning instances of the same class.
That’s where I stopped poking around for my implementation any further and started checking some more details about the API/code and just wanted to leave some feedback about the architecture:
- by introducing factories you (normally) agree that API users can pass in their own factory. I can imagine why you made it immutable/idiot-safe, regarding to your “default server always requires reliable and maybe/optional unreliable channel”, but either it’s an exchangeable factory returning case-dependent object instances, or it’s something immutable/unchangeable always returning instances of one and the same class.
- if you introduce factories, then use them consequentially: imagine
KernelFactoryto beKernelFactory<Kernel>, and use the factory everywhere, where you need to instantiate a new instance ofKernel. That means, kick thatDefaultServerconstructor-arguments forKernel reliableandKernel fast, and require the API user to pass in aKernelFactoryinstead, which you then use to instantiateKernels. - the redundant
private static finaldeclarations ofCH_RELIABLE,CH_UNRELIABLEandCH_FIRST, inDefaultServerandDefaultClientoffends the DRY-principle and seems to be no good design decision: as I’m forced to extend fromDefaultServerfor using implement my ownCustomKernelFactory, I now need to declare them a third time for no obvious reason. If both, server and client, need the same constants, you should follow DRY and supply them in a public, transparent and non-redundant way - for me, the choice of hard-coded
int-based “channel ID constants” seem to rise from stopping “one abstraction layer too early”. A much cleaner way, in respect of problem described in 3.), would be to use a builder, which routes some channel-names using strings (more self-explanatory and non-colliding thanint) to some unique (port/channel) number, and in the end, the builder creates an effectively immutableChannelMap<String,Integer>.
Maybe the private static final int-declarations are a result of some “maximum performance desire”, but in my opinion some more generic way wouldn’t impact performance that bad, because we’re talking about IO-operations which take way longer than a map.get(channelName) for obtaining some integer to be used in your API.
Let me show some exemplaric code that demonstrates a bit what I just critizised, and which would have made my task, using your API to implement my requirements much easier and faster. That’s a subjective impression, I know. The following code would be split up in a lot of different class-files, but I’m lazy, so I just dump them in here:
// YOU follow the DRY principle and clearly state some public API constants,
// which have the same, pretty speaking names:
public class DefaultChannel {
// effectively replaces your CH_RELIABLE and CH_UNRELIABLE:
public static final String RELIABLE = "RELIABLE";
public static final String UNRELIABLE = "UNRELIABLE";
}
// that would encourage me, the user of your API principles, to create:
public class SslSecuredChannel {
// some of **my** project specific stuff:
public static final String SECURE = "SECURE";
}
// in order to use the string-based names, we have to change the contract "a bit":
public interface KernelFactory {
// improve later code readability by switching to String instead of int,
// because channelName="SECURE" is much more self-explaining than channelName=2:
public Kernel createKernel(String channelName, int port) throws IOException;
}
// next, we use real factory to completely handle all the DefaultServers
// Kernel-instantiations:
public class DefaultKernelFactory implements KernelFactory {
public Kernel createKernel(String channelName, int port) throws IOException {
if (DefaultChannel.RELIABLE.equals(channelName)) {
return new SelectorKernel(port);
}
if (DefaultChannel.UNRELIABLE.equals(channelName)) {
return new UdpKernel(port);
}
throw new RuntimeException(String.format("Dafuq is '%s'?", channelName));
}
}
// that allows me to reuse YOUR efford/code:
public class SecureDefaultKernelFactory extends DefaultKernelFactory {
@Override
public Kernel createKernel(String channelName, int port) throws IOException {
if (SslSecuredChannel.SECURE.equals(channelName)) {
return new SslSelectorKernel(port);
}
return super.createKernel(channelName, port);
}
}
// and enables me, to use a "ChannelSetup"-class which I can use for client and
// server purposes, associating channel names with zerobased indices and port numbers:
public class MyGameChannelSetup {
public static ChannelMap createChannelMap(ServerArguments args) {
return ChannelMap.createEmptyBuilder()
.route(DefaultChannel.RELIABLE).to(args.getPortTcp()).reliably() // index=0
.route(DefaultChannel.UNRELIABLE).to(args.getPortUdp()).unreliably() // index=1
.route(SslSecuredChannel.SECURE).to(args.getPortSsl()).reliably() // index=2
.build();
}
}
// which would result in the following server constuction:
DefaultServer server = new DefaultServer(
gameName,
gameVersion,
MyGameChannelSetup.createChannelMap(parseArgs(args))
new SecureDefaultKernelFactory());
// and finally, the DefaultServer-constructor would work like this:
public DefaultServer (String gameName, int gameVersion, ChannelMap channelMap, KernelFactory factory) {
this.gameName = gameName;
this.version = gameVersion;
this.services = new HostedServiceManager(this);
addStandardServices();
for (String channelName : channelMap.getChannels()) { // relies on side-effect: getChannels() need
Kernel kernel = factory.createKernel( // to return the String-names "in order of addition"
channelName,
channelMap.getChannelPort(channelName));
channels.add(
new KernelAdapter(this, kernel, dispatcher, channelMap.isChannelReliable(channelName)));
}
}
I know that there are a lot more required refactorings in order to make that work completely, but I just wanted to provide some feedback on how the overall “first API design impression” I got.
Would love to hear your opinions or background information,
cheers,
Toby
Edits: kicked a misplaced “not” int “not offends the DRY…” and corrected a mismatched reference from “in respect of problem described in 2.” to in “respect of problem described in 3.)”, (and then added this comment)