make nanohtttpd easier to extend

[ritchieGitHub]

as stated in #236 the base class should be cleaned-up for extention

• all private methods to protected?
• all inner classes to static?
• move all object instanciations in creator methods (or a more general factory system)
• still keep the class from growing to big ;-)

any other ideas or objections?
@LordFokas @tuxedo0801

[LordFokas]

You missed the part where functionality is defined by interfaces so that we can implement it in new classes instead of extending existing ones... apart from that, I think that cuts it.

But I'd rather use creator methods than factories. It's clean and simple, and doesn't have the added complexity.

[elonen]

• Changing all privates to protected has the side effect that any change in those function declarations will cause backwards incompatibility. (I'd personally like to be even more careful on what to expose than currently, but then again, I'm not an active user or even developer of the project, so my word shouldn't have that much weight.)
• Inner classes to statics; works for me. Not sure how much this helps extending, though.
• Factory system would grow code, but might make extending easier. Though one.

[LordFokas]

@elonen:

• Backwards compatibility shouldn't be a problem. If you're a dev using NanoHTTPD on your project and you're overriding default functionality it's implicit that you're responsible for updating your code whenever you update the underlying library. Not everything has to be protected, there are some small utility methods that make sense as private, but relevant protocol logic methods should be override-able.
• Inner types are always bound to an instance of their container type, which can make things more complicated when you start to have a larger class hierarchy, mostly related to type casts and and container instances being passed everywhere. Also making it static forces you to pass container types as parameters in method calls which prevents implicit calls on container type methods, thus making the code more explicit and easier to understand.

The main reason for the two points above is that current code has a lot of non-static inner types making implicit references / calls on private container type fields / methods, which makes extending and / or overriding functionality a pretty solid nightmare on pretty much every level.

[ritchieGitHub]

I think we should rebuild fi.iki.elonen.NanoHTTPD.ServerSocketFactory to a general factory and move all factory methods there. That way we don't use any lines of code extra (interface and default implementation are already there) for the factory methods and can still use a general factory system that is compact and easy to use? This would of course change the API but this issue will do that any case so I do not think that's a show stopper. Any thoughts to that?

[jcppdev]

What backward compatibility? Don't say that you care about that because it lost a true backward compatibility a few times already at least. If it will break people will not update and instead they will stick to the old implementations like me. Almost every tutorial and example over the web now is uselses. Just wanted to say: great job on the backward compatibilty issue

[elonen]

One can basically pick any two, but not all three: 1) Fixes and new features 2) Small codebase ("nano") 3) Full backwards compatibility We've picked 1 and 2 for this project. Sticking to an old version is a good choice if you don't need the new developments.

[jcppdev]

Ok. I have managed to restore the backward compatibility by adding some stuff - 2 public constructors and a utility method for never-throwed-exception handling- maybe somebody will find it useful. Screens with what to do, and with a simple manual test in the comment.
By the way, it's a bit shame to enforce the extending of the Response class by making the contructor protected when it's basic instances work great. It's adding complexity to the purest and simplest of the solutions.
BTW, elonen, you are the boss for creating this project :D

[elonen]

Thanks @jcppdev . Actually, you can (/ are supposed to) use the new newFixedLengthResponse() or newChunkedResponse() factory functions, so no need to subclass Response, depending on wether you are streaming your response data or sending a fixed length block of data.

(As for the first part of this thread, @ritchieGitHub and @LordFokas, I think this is one good example of why I'd preferred private instead of protected functions in places where you aren't really supposed to subclass stuff.)

[jcppdev]

And I must agree here.

private means strictly should be used only in declaring class.
protected means strictly should be used in declaring class or inherited class, so it highly suggests a subclassing nature.
public means strictly should be used in declaring class, inherited class or from outside.

A protected constructor works almost like an abstract class - with an optional ability to instantiate an object inside of that class. In C++ it's a practice for enforcing inheritance of a class which doesn't contain any pure virtual (abstract) method because of the way how it works - it can be inherited and not instantiated from outside. That's used mainly because C++ doesn't have an abstract keyword for marking abstract classes which become automatically abstract by having at least one pure virtual method.

One thing what you should remember is that you can change the access modifiers by increasing access without consequences. So changes from private to protected, protected to public or private to public do not cause damage to the users and can be safely applied in every moment in the future.

But when you decrease access other people implementations can start breaking because someone could already be using a protected or public method in a situtation which decreased access will not allow him to use. So changes from public to protected, protected to private or public to private can be seen as a potential backward compatibility problem.

That's why it's better to initially mark a method as private and increase the access if needed than the other way around and don't give away unnecessary access to methods unless you really have to or other developers can start having a hard time.

Lastly a public or protected methods are parts of the classes contract - they should not change. On the other hand private methods are not contracts - they can be changed or removed without any negative effects.

[LordFokas]

While I don't mind gradually opening up the method visibility as needed, anything that is predictably going to be needed in the future can be left as public / protected from day zero to avoid having people coming in weekly asking to open this or that.

In my case, with the websockets, I changed it in such a way that no visibility changes should be needed in the predictable future, although I was really only thinking about my use cases, but we can see people trying other stuff (like SSE, for example) and needing to change large chunks of private stuff, which to be done properly means rewriting part of NanoHTTPD's structure, which is what brought us here in the first place.

From my point of view, we can either open most of the methods up in a way that allows overriding and extending to great measures without breaking anything (while keeping performance in mind) or we can be finicky about what to keep private and stay stuck in this loop forever.

[ritchieGitHub]

As always in such discussions, all sides have positive and negative side effects. After these discussions I conclude that we have 2 main type of users.

• just users (simple nanohttp use-cases)
• deep users

While the simple users just need nanohttp as a simple way to add a functionality to there application, the deep users need the internals and need to bend nanohttp (the hard way) to there needs.
The simple users need to be protected from complexity and directed to the few methods they need to implemented while the deep users need all (and more).

What if we add 1 or 2 annotations, like NanoAPI/NanoDeepAPI (names to be discussed) to separate the methods that are protected but should not be overwritten for normal usecases (And so on ...).

Second what about my proposal to upgrade the ServerSocketFactory to a general factory, then the additional code-lines would be so not high and some existing methods just have to move there, like the new newFixedLengthResponse() or newChunkedResponse() factory functions.

[elonen]

Adding abstraction (API) layers sounds otherwise good but Java is a very verbose language, so it would probably increase the codebase quite a bit. I actually think "power users" will most likely not be too shy about forking the code instead of subclassing if they need something special, so keeping more stuff private (to prevent some future backwards compatibility breaks) might not actually be that bad. I like the ServerSocketFactory generalization idea, to centralize the extension points a bit.

[ghost]

Finally!! I have literally spent days trying to find out why my code (which is seemingly identical to every other example on the web) doesn't work! @jcppdev thanks for pointing that out.

[LordFokas]

I've been thinking about this...

There seems to be a better way to solve this by changing the way the HTTP server works. Instead of using the constructor to configure the server and overriding the serve method (which 404's by default), all the configuration would be sent in the start method including an interface implementor that the serve method would use by default to serve HTTP requests. That way you only need to subclass the HTTP server if you want to change the way it works (like, for example, make it accept WebSockets).

At that point everything needed for the common utilization of NanoHTTPD would have public visibility (as in, all those inner classes that should really be static) and internally most stuff could be protected to ease in changing and extending the server...

So in the end this is what we've got:
Normal Users: Create an instance of NanoHTTP and send it configurations and a serve implementation on the start() method.
Power Users: Create a subclass of NanoHTTP and change the way it works internally to suit their needs. Everything is neatly organized in easily overridable protected methods.

[ritchieGitHub]

That sounds good, thumbs up from me.
My time is limited at the moment, and the foreseeable future. So I would like to reduce my tasks here to maven, travis-ci and releasing. @elonen what about inviting @LordFokas as a developer and give him a free hand (for a while at least ;-) ) ?

[ritchieGitHub]

You are invited as a developer with write access, please try if the permissions are ok.

[LordFokas]

I don't have much time at the moment but I'll probably find some soon and start fixing some things...

[ritchieGitHub]

@LordFokas and @elonen I think we should release once 2.3.0 before @LordFokas starts the refactorings. Do you both think that one of the open issues is worth the wait? (or should be fixed first?)

[LordFokas]

We could leave the refactoring for 3.0
That'd actually be nice because there's some big changes to the structure that I'd like to propose soon and it'd all fit nicely together in a new major version.

[LordFokas]

My proposed changes were expressed in #278 so I don't think this is needed anymore.
I'll be leaving it open in case there's something here you don't feel had a proper closure, @ritchieGitHub.