Hello All,
Who thinks Openfire codes needs refactoring? Anyone here who supports Agile Methology and Extreme programming. There so much comments in code we can remove these comments with refactoring and code can be easily unstable by other devlopers.
We ask everone who contributes to Openfire that in future iteration we make code more refactored
So, I really hope that was a typo. You want to remove all the comments and refactor it to make it unstable?
Personally, I think comments are a good idea. They help make the code understandable, and with Javadoc they create a nice documented API. If you want to refactor, what is it exactly you want to refactor?
Hi Vishawjeet,
I agree completely with slushpupie.
Agile and extreme programming methodologies do not state that programmers should not comment their code and the ability to refactor code is not based on the presence (or lack thereof) of well documented and commented code.
The comments and javadocs are extremely beneficial for developers in situations where they are going to be developing against a particular program, such as the case when developing plugins for Openfire, or Spark for that matter.
Cheers,
Ryan
Hi,
I agree that some parts of the code should be refactored.
The pool JDBC connection pool was replaced recently with Proxool to make it much better.
Openfire still makes no or very little use of RDBMs features like foreign keys, constraints and sequences. This makes updates and deletes a little bit tricky.
Also the database design itself should be reviewed.
There may be some other things which one may want to refactor. The comment have nothing to do with the code itself so I wonder why you mention them.
LG
Some suggestions (I’d be happy to have these features, but I don’t insist they must be in trunk):
RosterItemProvider class should be an interface to enable custom roster<->DB intergration. This is also more logical, cos we have UserProvider interface, etc.
Presence subscriptions management is a bit tricky now. I believe it needs to be simplified (particularly, PresenceSubscribeHandler class). Simplified not in terms of efficient code, but it just should be easy to understand for everyone who read RFCs.
Loading of custom Admin, Auth and User providers must be fixed somehow. When I tried to install my plugin, I got ClassNotFoundException complaining about my custom provider. The only solution I found is to put plugin-myplugin.jar file into lib/ dir. This is not about the code refactoring itself, but quite relevant.
Also I agree with it2000.
On a kinda related note, It would be cool to refactor Openfire to use some kind dependency injection (Spring,Google Juice). It would make swapping out most implementations as easy as changing an annotation or bean definition. I think it might also be good for making the app more testable. I got pretty frustrated trying to write functional tests for my plugin because of all the statics and factories that are used. I could be way off base just throwing it out there.
-Nate
Hi Nate,
Yeah, testing of plugins is a challenge alright.
If I had a vote I would prefer to see Guice used rather than Spring.
Spring is nice but Guice is so much more lightweight and built for
speed.
Cheers,
Ryan
First of All… I want to know… Are you know What is re-factoring… Although I am not so much experienced as you are… But I am fan of “Agile Methodology and Extreme programming”. They provide a good approach to software development. In XP we have test before actual
development. But these code is not testable at Unit Testing levels.
Refactoring definition that I taken from a book on refactoring by Martin Fowler
Refactoring: a change made to the internal structure of software to make it easier to understand and cheaper to modify without changing its observable behaviour.
I find this code need more concentration to understand. It also seems unmanageable to modify. There are too redundancy in the code.
About comments my views as following: I not say that there is not any Javadoc comment. Javadoc comments is necessary things to create a nice documented API
Martin Fowler said as about comments:
Don’t worry, we aren’t saying that people shouldn’t write comments. In our olfactory analogy, comments aren’t a bad smell; indeed they are a sweet smell. The reason we mention comments here is that comments often are used as a deodorant. It’s surprising how often you look at thickly commented code and notice that the comments are there because the code is bad.
Comments lead us to bad code that has all the rotten whiffs. Our first action is to remove the bad smells by refactoring. When we’re finished, we often find that the comments are superfluous.
If you need a comment to explain what a block of code does, try Extract Method. If the method is already extracted but you still need a comment to explain what it does, use Rename Method. If you need to state some rules about the required state of the system.
Tip: When you feel the need to write a comment, first try to refactor the code so that any comment becomes superfluous.
A good time to use a comment is when you don’t know what to do. In addition to describing what is going on, comments can indicate areas in which you aren’t sure.
A comment is a good place to say why you did something. This kind of information helps future modifiers, especially forgetful ones."
I am not so much expert in refactoring as I am learning these methodology, But I try to give some example by
modifying the DefaultAuthprovider after applying refactoring. I attached source here so that you can see the difference. That databased part most of classes is redundant. We can make it more manageable as other people also talked about database.
modified DefaultAuthProvider.java (7563 Bytes)
orginal DefaultAuthProvider.java (8477 Bytes)
Hi vishawjeet,
I think it’s too much of unnecessary work here. Especially, method checkNullForUsernameAndPassword is used only once in the code.
I’m not sure how Java handles methods calls, but Openfire is a SERVER. It must be a fast as possible (this is main openfire’s advantage I think) . But if we’re going to split everything into small methods it can become a performance nightmare.
But I agree that removeDomainNameIfContain may be useful here.
Basic strategy here is: do not break working things unless you know what you’re doing. Moreover, DefaultAuthProvider is just too plain simple. You may do everything you want in your custom AuthProvider.
Vlad
P.S. Compiled code doesn’t care about comments, but it does about superfluous methods calls.
Hi Vlad!
Althought method checkNullForUsernameAndPassword called one time but its according to Extermene programming . Now compiler are so smart they do some optimization themself. They make this type of code inline when find good for optimization. So we needn’t to worry so much about it. But by spliting like this remove the need of comment and more understandable.
The following is my personal opinion based on my meanderings through the sysadmin, programming, and business world. It has little basis on my academic background (which was, oddly enough, in programming).
Two things here:
First, is in my mind, is the overuse of methods. When trying to debug a program, it can get really hard to follow the logic when you are jumping back and forth between lots of tiny methods. I think the methods are useful if they get used more than once (or at least might be). Related to this is the is the issue of javadoc comments. In the AuthProvider example you posted its not too noticeable- since the public methods are already documented in the provider. Some private methods should be javadoc’d as well, when their meaning needs extra explanation. If you increase the number of methods, the number of javadoc comments will need to increase as well. There are just some method names that you wont be able to adequately describe in a few words (and using a whole sentence for a method name isnt my favorite idea either). When that happens, you need to use a comment- preferably a javadoc comment.
Second, is the cost of refactoring. Openfire has a fairly significant code base, and a fairly significant user base. There are people who have customizations of Openfire, too. So what is the cost of refactoring? To change the database or public API will have direct impact on users of Openfire you will need to provide a clean upgrade path. The other cost is that of actually making the changes. The time it takes to change all the code, the time it takes the Jive developers to understand the new code, and the time it takes to track down newly introduced bugs. You must weigh this cost against the benefit of making the changes. I see a benefit to some of the changes you made in your example, but not others.
Lets look at one of the changes you made. The name “checkNullForUsernameAndPassword” might be obvious to you, but its not to me (I think thats partly a language barrier). You quote Martin Fowler that if you need to comment a block of code to explain what it does, you should have a separate method for it. In this case, you didnt change the actual code, and the method name is more cryptic than the comment used to explain it. There might even be a (very) slight impact on performance (the stack is increased, unless the compiler can in-line it).
Another method, “removeDomainName…” makes more sense to me, though I would have shortened its name some. The logic here is slightly complex, and splitting it up does help the readability. I would say that a little deeper evaluation needs to happen to evaluate its performance, however. The problem with string manipulation is the String object is immutable, so the more we copy or modify the string, the more objects are used, and thus more memory.
Hi Vlad,
Java has a nice and powerful Just-In-Time compiler which can inline methods. So one does not need to limit the number of methods used.
Anyhow debugging becomes more painful.
LG
Hi LG,
Nice. OK. Let’s do something more constructive.
vishawjeet wants to refactor the code? Great! But first we need a plan.
vishawjeet should post a “roadmap” or plan of refactoring.
We should involve other interested developers into the process.
We need someone experienced (a jiver, maybe) who can supervise the process.
Vlad.
Except that we do need to worry about optimization in a server that expects high performance and heavy loads. Understanding how method calls affect performance is important. If you can use some profiler, it can provide some of the best evidence you can get if a new implementation is faster or slower.
In the case of the AuthProvider, this code will only get executed during authentications, which should be rare compared to other operations, so I doubt your example would have any noticeable impact on performance. Other things such as the packet parsers, routers, etc get called a lot more frequently, and I would be very hesitant to add more method calls there.
The XP method results in readable source code. Readable source code is not necessarily efficient (indeed, it often will not be). Not all problems can be solved efficiently with the XP method. Additionally, since the XP method is akin to religion, getting a diverse group of programmers to all adopt it en masse will be much like getting the Middle East to adopt a single religion.
My advice to you is this:
If you want to submit new code using this ideology, feel free. New code here is rarely (ever?) rejected on that premise alone.
If you want to refactor existing code, you are going to need to make sure of these things:
It does not hurt performance in any way (do not make assumptions- make certain)
It does not break any existing parts (we have a lot of moving parts, so test extensively)
It does not introduce any new bugs
It was not done the original way for a specific purpose (often that means asking the original authors)
It does not change so much at once so that the existing developers have time to comprehend your changes
Hey everyone,
Thanks for your interest in Openfire. I personally agree with Slushpupie comments. I see that many of you are willing to help us in this project so you are more than welcome!
Since there are many new features to add to Openfire or bugs to fix I would prefer to focus your time and effort in solving existing problems or adding highly requested features from the community. IMHO, that would be a much better usage of your time in favor of the community. Once we are done with other more important things we can reconsider doing changes that will make things nicer but are not solving problems. Or may be I missed the problem that you are trying to solve.
Thanks,
– Gato
Hi Jay,
You said that code is rarely rejected. But take a look at this thread (3 years ago!)
And the code was rejected obviously.
Hi Gato,
I think you get it right.
How to determine which features are highly requested from the community?)
Ex. I don’t understand why RosterItemProvider class is not an interface. And why I cannot implement it in my custom way. Is this highly requested feature?)
Vlad
Hey Vlad,
How to determine which features are highly requested from the community?)
Good question. One way that works for me is to spend some time each day for a few weeks reading the forums, listening to what people are reporting or saying or thinking. Another source of input is the XMPP mailing list where you can get feedback about the direction where XMPP is going. For instance, microbloging is a hot topic.
Ex. I don’t understand why RosterItemProvider class is not an interface. And why I cannot implement it in my custom way. Is this highly requested feature?)
In general, our idea is to solve the problem that we need to solve instead of creating models that could be extended just because they can be extended. I’ve been in that path for 10 years and I then realized that that was one source of complexity. The ironic thing is that doing things simple is a lot more complex than doing them complex. Anyway, I’m not saying that that class should not be an interface. I’m just saying that that was not our goal since there was no need to do that. As you can see in the other thread, Matt was asking for a business problem that was being solved with that patch. And as the author said, the contributed patch was something not fully tested. Since then we have not heard many people asking for this feature.
Regards,
– Gato
Gato,
Thanks for clarification. I don’t want to turn this thread into discussion of my custom problem, but if you touched upon that…I need it to be an interface, cos we already have a table in our DB containing relationships between users. And it’s much more easy for us to change table schema so it can fit XMPP subs than using Openfire’s standart table and be forced to replicate it into our custom table programmatically…
Microblogging is a hot topic. but does it mean that USERS really need it?) Is this just another buzzword or it’s something really going on here? complex questions, Gato!
Vlad
P.S. I’m ready to rethink the piece of code you talking about, fix it and test it (if someone will guide me how to do comprehensive testing)
Yes optimization matters in servers. but As LG said " Java has a nice and powerful Just-In-Time compiler which can inline methods. So one does not need to limit the number of methods used." So we need n’t worry about increased method call and still we can use profiler to est new implementation.
Yes, It is not necessary readable/refactored code is effcient. But it is easily modified for future requirement easily and very less chances are there. A code which is not readable soon results a lot of bugs while introducing new features and its designs also blurs as there are change in code. I am not forced to anyone to adapt XP. I am not so much expert in XP. But I am learning this.
To make it sure existing parts not breaks, XP suggest to make test first. Test which are run when we made a small changes to code. So
using test driven approach we can refactor code easily and there is no chance of new bugs.