 |
jMule
|
| View previous topic :: View next topic |
| Author |
Message |
TAHO
Codereviewer
Joined: 28 Dec 2003
Posts: 18
Location: Taiwan, ROC
|
 Posted: Tue Dec 30, 2003 0:37 Post subject: Something about jMule code
|
 |
|
Hi there,
I am a eMule user for serval monthes, and new to jMule. I am interested in Java, so I downloaded the source code of jMule and read a real small part of it. I have some questions and idea about the code. So, I had wrote a private message to andyl, and he told me that I have better post here.
1. I saw you guys use java.util.logging.Logger in many classes, but I didn't see any Handler. Could you tell me where the message log to?
2. The class Messages uses original English message string as key. Why not shorten it, like Messages.getString("jMule_JreVersionError") or Messages.getString("StandAloneCommandLineUI_WelcomeMessage"). Add a
private static final ResourceBundle DEFAULT_BUNDLE = ResourceBundle.getBundle(BUNDLE_NAME, Locale.ENGLISH);
field for default value. ( If user locale bundle can't find the key, then return the value in DEFAULT_BUNDLE. if DEFAULT_BUNDLE also can't fine the key, then return empty string and log.)
3. Why not implements ConnectionManager, UploadManager, DownloadMager, and checkProtocles method in class MainLoop as Thread Object instead of implements class MainLoop as Thread Object. With this, we can setup the time span separately, and suspend, resume individually.
4. Why not use HashMap instead of ArrayList be the org.jmule.ui.sacli.controller.CommandManager.knownCommands field.
5. some "switch' or "instanceof" statements can use Polymorphism and the code like C++ struct instead.
Take org.jmule.coreUploadManager.setManagementMode(int) method for example, we can write a class UploadManagerStatus :
| Code: |
public final class UploadManagerMode{
public final static UploadManagerMode SIMPLE = new UploadManagerStatus(SIMPLE_MODE);
public final static UploadManagerMode P2PSPECIFIC = new UploadManagerStatus(P2PSPECIFIC_MODE);
private int mode;
private final static int SIMPLE_MODE = 1;
private final static int P2PSPECIFIC_MODE = 2;
private UploadManagerStatus(int mode){
this.mode = mode;
}
public UploadQueue getUploadQueue(){
UploadQueue uq = null;
switch (mode){
case SIMPLE_MODE:
uq = new UploadQueueSimple();
break;
case P2PSPECIFIC_MODE:
uq = new UploadQueueSpecific();
break;
}
return uq;
}
}
|
and the org.jmule.coreUploadManager.setManagementMode(int) method becomes,
| Code: |
public void setManagementMode(UploadManagerMode mode) {
if (uploadQueue==null){
uploadQueue = mode.getUploadQueue();
}
}
|
These are my opinions. I just read a little part of the code, and don't understand it totally. So, if I am wrong, please tell me. Thanks.
PS: Sorry for my poor English. If there is anything I wrote you don't understand, please tell me, and I will explain it as possible as I could.
|
|
| Back to top |
|
 |
emarant
Admin / Developer
Joined: 02 Oct 2002
Posts: 135
|
|
Posted: Tue Dec 30, 2003 2:51 Post subject: Re: Something about jMule code
|
|
|
| TAHO wrote: |
| These are my opinions. I just read a little part of the code, and don't understand it totally. So, if I am wrong, please tell me. Thanks |
Most of them are correct and the issues currently only lack of proper application to the sourcecode by someone. So you are welcome to point to those issues and improve the source code. 
| TAHO wrote: |
1. I saw you guys use java.util.logging.Logger in many classes, but I didn't see any Handler. Could you tell me where the message log to?
|
To the Handler defined in the logging.properties file passed on java call.
(default Handler for console and file)
We have also a gui Handler, but it needs some improvement (org.jmule.ui.javaUI.RegistredLogHandler).
| TAHO wrote: |
2. The class Messages uses original English message string as key. Why not shorten it, like Messages.getString("jMule_JreVersionError") or Messages.getString("StandAloneCommandLineUI_WelcomeMessage"). Add a
private static final ResourceBundle DEFAULT_BUNDLE = ResourceBundle.getBundle(BUNDLE_NAME, Locale.ENGLISH);
field for default value. ( If user locale bundle can't find the key, then return the value in DEFAULT_BUNDLE. if DEFAULT_BUNDLE also can't fine the key, then return empty string and log.)
|
We were lazy so it works also if noone add the mapping to messages.properties. But it needs an improvement because in some cases it is ambiguous. So this is a point to improve If you want to do all the path on current cvs, no porblem, but than open a topic in coding forum (you have right to post).
| TAHO wrote: |
3. Why not implements ConnectionManager, UploadManager, DownloadMager, and checkProtocles method in class MainLoop as Thread Object instead of implements class MainLoop as Thread Object. With this, we can setup the time span separately, and suspend, resume individually.
|
This is planned ... but need a review for synchronization of shared objects.
| TAHO wrote: |
4. Why not use HashMap instead of ArrayList be the org.jmule.ui.sacli.controller.CommandManager.knownCommands field.
|
First the whole sacli needs some rework. Second jmule is not fully optimized and this is an example with less importance or do user query commands all day and night x times a second instead of waiting paitently ther downloads finish and doing something else.
| TAHO wrote: |
5. some "switch' or "instanceof" statements can use Polymorphism and the code like C++ struct instead.
Take org.jmule.coreUploadManager.setManagementMode(int) method for example, we can write a class UploadManagerStatus :
| Code: |
public final class UploadManagerMode{
public final static UploadManagerMode SIMPLE = new UploadManagerStatus(SIMPLE_MODE);
public final static UploadManagerMode P2PSPECIFIC = new UploadManagerStatus(P2PSPECIFIC_MODE);
private int mode;
private final static int SIMPLE_MODE = 1;
private final static int P2PSPECIFIC_MODE = 2;
private UploadManagerStatus(int mode){
this.mode = mode;
}
public UploadQueue getUploadQueue(){
UploadQueue uq = null;
switch (mode){
case SIMPLE_MODE:
uq = new UploadQueueSimple();
break;
case P2PSPECIFIC_MODE:
uq = new UploadQueueSpecific();
break;
}
return uq;
}
}
|
and the org.jmule.coreUploadManager.setManagementMode(int) method becomes,
| Code: |
public void setManagementMode(UploadManagerMode mode) {
if (uploadQueue==null){
uploadQueue = mode.getUploadQueue();
}
}
|
|
I don't want to discuss such details, but in "perfect" design OO case every mode than has to be an own class implementing/extending UploadManagerStatus interface/class to skip the switch inside the class UploadManagerStatus above. 
Yes in some cases these are todos, but I doubt if the increased number of classes, interfaces and codelines really help the developers managing the code or improve jMule as whole application.
some more examples:
In some cases we have to parse binary stuff and this is done by mapping integral values (short bit sequences interpreted as integer java primitves) to the OO layer using switches in some case.
The instanceof in ConnectionManger should get removed by improved interface definiton.
|
|
| Back to top |
|
|
TAHO
Codereviewer
Joined: 28 Dec 2003
Posts: 18
Location: Taiwan, ROC
|
|
Posted: Tue Dec 30, 2003 6:41 Post subject: Re: Something about jMule code
|
|
|
| emarant wrote: |
| TAHO wrote: |
| These are my opinions. I just read a little part of the code, and don't understand it totally. So, if I am wrong, please tell me. Thanks |
Most of them are correct and the issues currently only lack of proper application to the sourcecode by someone. So you are welcome to point to those issues and improve the source code.
| TAHO wrote: |
1. I saw you guys use java.util.logging.Logger in many classes, but I didn't see any Handler. Could you tell me where the message log to?
|
To the Handler defined in the logging.properties file passed on java call.
(default Handler for console and file)
We have also a gui Handler, but it needs some improvement (org.jmule.ui.javaUI.RegistredLogHandler).
|
Yes, I saw the properties files, but does it be loaded automatically?
I inserted some code into jMule.start() method, after the
| Code: |
if(mode == MODE_COREONLY) {
log.fine(getAppInfoText() + getAppVersionText());
|
But I found that this log object doesn't have any Handler.
And I didn't have any message logged.
| emarant wrote: |
| TAHO wrote: |
2. The class Messages uses original English message string as key. Why not shorten it, like Messages.getString("jMule_JreVersionError") or Messages.getString("StandAloneCommandLineUI_WelcomeMessage"). Add a
private static final ResourceBundle DEFAULT_BUNDLE = ResourceBundle.getBundle(BUNDLE_NAME, Locale.ENGLISH);
field for default value. ( If user locale bundle can't find the key, then return the value in DEFAULT_BUNDLE. if DEFAULT_BUNDLE also can't fine the key, then return empty string and log.)
|
We were lazy so it works also if noone add the mapping to messages.properties. But it needs an improvement because in some cases it is ambiguous. So this is a point to improve If you want to do all the path on current cvs, no porblem, but than open a topic in coding forum (you have right to post).
|
That's OK, if you need me to do some work. But I don't know how to start.
Should I post my code on coding forum and point out which class invoke this method?
| emarant wrote: |
| TAHO wrote: |
3. Why not implements ConnectionManager, UploadManager, DownloadMager, and checkProtocles method in class MainLoop as Thread Object instead of implements class MainLoop as Thread Object. With this, we can setup the time span separately, and suspend, resume individually.
|
This is planned ... but need a review for synchronization of shared objects.
| TAHO wrote: |
4. Why not use HashMap instead of ArrayList be the org.jmule.ui.sacli.controller.CommandManager.knownCommands field.
|
First the whole sacli needs some rework. Second jmule is not fully optimized and this is an example with less importance or do user query commands all day and night x times a second instead of waiting paitently ther downloads finish and doing something else.
|
If there are some simple code needed to be optiomized and you just don't have time to do these more unimportant things, maybe I can do some help.
| emarant wrote: |
| TAHO wrote: |
5. some "switch' or "instanceof" statements can use Polymorphism and the code like C++ struct instead.
Take org.jmule.coreUploadManager.setManagementMode(int) method for example, we can write a class UploadManagerStatus :
| Code: |
public final class UploadManagerMode{
public final static UploadManagerMode SIMPLE = new UploadManagerStatus(SIMPLE_MODE);
public final static UploadManagerMode P2PSPECIFIC = new UploadManagerStatus(P2PSPECIFIC_MODE);
private int mode;
private final static int SIMPLE_MODE = 1;
private final static int P2PSPECIFIC_MODE = 2;
private UploadManagerStatus(int mode){
this.mode = mode;
}
public UploadQueue getUploadQueue(){
UploadQueue uq = null;
switch (mode){
case SIMPLE_MODE:
uq = new UploadQueueSimple();
break;
case P2PSPECIFIC_MODE:
uq = new UploadQueueSpecific();
break;
}
return uq;
}
}
|
and the org.jmule.coreUploadManager.setManagementMode(int) method becomes,
| Code: |
public void setManagementMode(UploadManagerMode mode) {
if (uploadQueue==null){
uploadQueue = mode.getUploadQueue();
}
}
|
|
I don't want to discuss such details, but in "perfect" design OO case every mode than has to be an own class implementing/extending UploadManagerStatus interface/class to skip the switch inside the class UploadManagerStatus above.
Yes in some cases these are todos, but I doubt if the increased number of classes, interfaces and codelines really help the developers managing the code or improve jMule as whole application.
some more examples:
In some cases we have to parse binary stuff and this is done by mapping integral values (short bit sequences interpreted as integer java primitves) to the OO layer using switches in some case.
The instanceof in ConnectionManger should get removed by improved interface definiton.
|
Yes, you are right. I post this just because I think if the project become larger and popular, someone may misuse this method by giving a wrong parameter value, and it may be hard to read or debug.
|
|
| Back to top |
|
|
Guest
|
|
Posted: Tue Dec 30, 2003 11:39 Post subject:
|
|
|
| emarant wrote: |
We have also a gui Handler, but it needs some improvement (org.jmule.ui.javaUI.RegistredLogHandler).
| TAHO wrote: |
Yes, I saw the properties files, but does it be loaded automatically?
|
|
It will get loaded at the start of the VM. Please look at the jmule.bat|sh and the -Djava.util.logging.xxx parameter.
| TAHO wrote: |
I inserted some code into jMule.start() method, after the
Code:
| Code: |
if(mode == MODE_COREONLY) {
log.fine(getAppInfoText() + getAppVersionText());
|
But I found that this log object doesn't have any Handler.
And I didn't have any message logged.
|
Maybe 'coz you have simply started jMule with
| Code: |
java org.jmule.jMule |
and haven't specified the -Djava.util.logging.xxx parameter there ?
| TAHO wrote: |
2. The class Messages uses original English message string as key. Why not shorten it, like Messages.getString("jMule_JreVersionError") or Messages.getString("StandAloneCommandLineUI_WelcomeMessage"). Add a
private static final ResourceBundle DEFAULT_BUNDLE = ResourceBundle.getBundle(BUNDLE_NAME, Locale.ENGLISH);
field for default value. ( If user locale bundle can't find the key, then return the value in DEFAULT_BUNDLE. if DEFAULT_BUNDLE also can't fine the key, then return empty string and log.)
| emarant wrote: |
We were lazy so it works also if noone add the mapping to messages.properties. But it needs an improvement because in some cases it is ambiguous.
|
|
What you are describing here is the standard java way to handle localised messages. This has the main disagventage, as pointed out by emarant already, that if you forget to add some translated string to your bundle, the end-user gets anoyed by strings like STR_ERR_ME_TEH_GREATE_HAX00R_343 .
| TAHO wrote: |
Take org.jmule.coreUploadManager.setManagementMode(int) method for example, we can write a class UploadManagerStatus :
---snip---
| emarant wrote: |
Yes in some cases these are todos, but I doubt if the increased number of classes, interfaces and codelines really help the developers managing the code or improve jMule as whole application.
|
| TAHO wrote: |
Yes, you are right. I post this just because I think if the project become larger and popular, someone may misuse this method by giving a wrong parameter value, and it may be hard to read or debug.
|
|
As I am not a big fan of such code too, sometimes it is just too 'sexy'. But jMule project has no fear beeing refactored, IF there is a need.
TAHO if you are serious about helping out the projects, we would be more than glad if you could just do some code/JAVADOC review with a fresh eye and document your findings in a separate document or, which would be even better, in the code itself ( as javadoc comments ).
Let's take the futher discution to the Coding/Design section.
|
|
| Back to top |
|
|
|
|
|
You can post new topics in this forum
You can reply to topics in this forum
You cannot edit your posts in this forum
You cannot delete your posts in this forum
You cannot vote in polls in this forum
|
Powered by phpBB 2.0.3 © 2001, 2002 phpBB Group
|
FAQ
Search
Memberlist
Usergroups
Register
Profile
Login to check your private messages
Login
