tcpmon improvements

[Sébastien Le Callonnec]

Hi,

I have implemented some minor improvements such as bookmarks in my tcpmon clone there:
http://code.google.com/r/slc-ie-tcpmon/

Not sure whether this would be considered for a merge in the future, but I have found this useful.  Please feel free to comment!

Regards,
Sébastien.

[Inderjeet Singh]

Hi Sebastien, 

I think we should merge the projects. I am happy to grant you developer access, if you would like. 

Can you send me a description of changes, or a change list if possible? 

Inder

[Sébastien Le Callonnec]

Hi Inder,

Sure. So far here is what I have done:

- I have bumped the version of the pom, and added a profile (release)
for signing the jar, so that it doesn�t get done all the time. This is
more for development convenience, I realise this may not be something
you�d want;
- I have also added a test dependency to mockito to be able to use mocks
in unit tests -- again, this is something open for debate;
- I have fixed the issue with the Main-Class not being set in the Manifest;
- I have added a bookmark feature that adds a menu so that details can
be saved and reused later. This is still early stages and quite
simplistic, but it does the trick;
- The main entry point is now the Main class, which creates a
Configuration instance and passes it down to the GUI. This is the basis
to add a headless mode, which I�m thinking of working on next as I
currently need this;
- Finally, the debug level can be set on the command line.

Let me know if you have comments about this.

Thanks,
S�bastien.

On 11/08/2011 22:47, Inderjeet Singh wrote:
> Hi Sebastien,
>
> I think we should merge the projects. I am happy to grant you developer
> access, if you would like.
> Can you send me a description of changes, or a change list if possible?
>
> Inder
>

> On Thu, Aug 11, 2011 at 2:44 PM, S�bastien Le Callonnec <slc...@yahoo.ie
> <mailto:slc...@yahoo.ie>> wrote:
>
> Hi,
>
> I have implemented some minor improvements such as bookmarks in my
> tcpmon clone there:
> http://code.google.com/r/slc-ie-tcpmon/
>
> Not sure whether this would be considered for a merge in the future,
> but I have found this useful. Please feel free to comment!
>
> Regards,

> S�bastien.
>
>

[Inderjeet Singh]

Hi Sebastien,

On Thu, Aug 11, 2011 at 9:13 PM, Sébastien Le Callonnec <slc...@yahoo.ie> wrote:

Hi Inder,


Sure.  So far here is what I have done:

- I have bumped the version of the pom, and added a profile (release) for signing the jar, so that it doesn’t get done all the time.  This is more for development convenience, I realise this may not be something you’d want;

This is fine. Development convenience is nice.

 

- I have also added a test dependency to mockito to be able to use mocks in unit tests -- again, this is something open for debate;

I have never used mockito so dont really know how it works out. I have used EasyMock in the past. Overall, I try to avoid mocks as much as possible. However, in this case, the tests need to mock out network connections so that is a fine use. 

I am open to using mockito, and we can discuss its merits as I get some experience with it.

 

- I have fixed the issue with the Main-Class not being set in the Manifest;

- I have added a bookmark feature that adds a menu so that details can be saved and reused later.  This is still early stages and quite simplistic, but it does the trick;

- The main entry point is now the Main class, which creates a Configuration instance and passes it down to the GUI.  This is the basis to add a headless mode, which I’m thinking of working on next as I currently need this;

- Finally, the debug level can be set on the command line.

All of these seem like fine features. So, lets go for it. Send me your GMail user name and I will add you as a developer on the project. 

Here is the development style I prefer (see https://sites.google.com/site/tcpmonproject/devel): 

1. All code changes should be reviewed by other developers on the project (using Google Code's built-in code review tools). 

2. Limit the amount of work done in each changelist. Generally, one coherent concept per CL is ideal. This makes the reviews go easier.

Thanks

Inder

 

 

Let me know if you have comments about this.

Thanks,

Sébastien.

On 11/08/2011 22:47, Inderjeet Singh wrote:

Hi Sebastien,

I think we should merge the projects. I am happy to grant you developer
access, if you would like.
Can you send me a description of changes, or a change list if possible?

Inder

On Thu, Aug 11, 2011 at 2:44 PM, Sébastien Le Callonnec <slc...@yahoo.ie

<mailto:slc...@yahoo.ie>> wrote:

   Hi,

   I have implemented some minor improvements such as bookmarks in my
   tcpmon clone there:
   http://code.google.com/r/slc-ie-tcpmon/

   Not sure whether this would be considered for a merge in the future,
   but I have found this useful.  Please feel free to comment!

   Regards,

   Sébastien.

[Sébastien Le Callonnec]

Hi Inder,

On Friday, 12 August 2011 08:14:37 UTC+1, Inderjeet Singh wrote:

Hi Sebastien,

On Thu, Aug 11, 2011 at 9:13 PM, Sébastien Le Callonnec <slc...@yahoo.ie> wrote:

Hi Inder,


Sure.  So far here is what I have done:

- I have bumped the version of the pom, and added a profile (release) for signing the jar, so that it doesn’t get done all the time.  This is more for development convenience, I realise this may not be something you’d want;

This is fine. Development convenience is nice.

ok
 

 

- I have also added a test dependency to mockito to be able to use mocks in unit tests -- again, this is something open for debate;

I have never used mockito so dont really know how it works out. I have used EasyMock in the past. Overall, I try to avoid mocks as much as possible. However, in this case, the tests need to mock out network connections so that is a fine use. 

I am open to using mockito, and we can discuss its merits as I get some experience with it.

Sounds good!
 

 

- I have fixed the issue with the Main-Class not being set in the Manifest;

- I have added a bookmark feature that adds a menu so that details can be saved and reused later.  This is still early stages and quite simplistic, but it does the trick;
- The main entry point is now the Main class, which creates a Configuration instance and passes it down to the GUI.  This is the basis to add a headless mode, which I’m thinking of working on next as I currently need this;
- Finally, the debug level can be set on the command line.

All of these seem like fine features. So, lets go for it. Send me your GMail user name and I will add you as a developer on the project. 

My username is actually my email address, cf. http://code.google.com/u/slc...@yahoo.ie/
 

Here is the development style I prefer (see https://sites.google.com/site/tcpmonproject/devel): 

1. All code changes should be reviewed by other developers on the project (using Google Code's built-in code review tools). 

2. Limit the amount of work done in each changelist. Generally, one coherent concept per CL is ideal. This makes the reviews go easier.

It makes perfect sense.  Do you want me to request code reviews from my clone (http://code.google.com/r/slc-ie-tcpmon/) before I merge my changes back into the tcpmon repo?

One thing I still have to sort out is code formatting to make sure my IDE formats according to the rules defined in Eclipse formatting-styles.xml (which I'm assuming is the reference?).


Regards,
Sébastien.

 

[Inderjeet Singh]

Hi Sebastien, 

All of these seem like fine features. So, lets go for it. Send me your GMail user name and I will add you as a developer on the project. 

My username is actually my email address, cf. http://code.google.com/u/slc...@yahoo.ie/

Ok. I gave you commit access. 

It makes perfect sense.  Do you want me to request code reviews from my clone (http://code.google.com/r/slc-ie-tcpmon/) before I merge my changes back into the tcpmon repo?

I would like the code reviews to be at one place. So, since we will be using the main project for future development, it is probably better if we review to code there. 

As this is mercurial, I am hoping that when you merge the clone back in the main tree, your individual changes will be preserved. I can then review them here.  

 

One thing I still have to sort out is code formatting to make sure my IDE formats according to the rules defined in Eclipse formatting-styles.xml (which I'm assuming is the reference?).

Yes, I would like us to use the same formatting styles to prevent code changes that contain only white spaces. Actually, that is another rule I would like to follow: If a file has only white space changes, it should be reverted back before check-in even if those changes were to fix the formatting errors. An exception to this rule is that it is okay to fix formatting when you have made other code changes in the file. 

Also, lets limit the line length to 100 characters.

Happy coding

Inder

[Sébastien Le Callonnec]

Hi Inder,

I have now pushed these changes to the tcpmon repo, let me know when
you�ve got a chance to look at the code there.

I�ll delete my own clone when I get the chance, as it is not needed anymore.

Thanks,
S�bastien.

[Inderjeet Singh]

Hi Sebastien, 

Great. I have reviewed all the changes. Overall, great work! 

However, one major issue that I see that this code will no longer work as a Java applet (or in another sandboxed environment). Please fix that by catching appropriate security exceptions. I have also added review comments to that effect. 

Please go through review comments and indicate your response. If you choose to accept a comment and fix it in a subsequent changelist. Please add a comment in the new changelist like "Incorporating code review feedback for r24".

Thanks

Inder

On Mon, Aug 15, 2011 at 12:24 AM, Sébastien Le Callonnec <slc...@yahoo.ie> wrote:

Hi Inder,

I have now pushed these changes to the tcpmon repo, let me know when you’ve got a chance to look at the code there.

I’ll delete my own clone when I get the chance, as it is not needed anymore.

Thanks,
Sébastien.

[Sébastien Le Callonnec]

Hi Inder,

On Monday, 15 August 2011 10:29:08 UTC+1, Inderjeet Singh wrote:

Hi Sebastien, 

Great. I have reviewed all the changes. Overall, great work! 

However, one major issue that I see that this code will no longer work as a Java applet (or in another sandboxed environment). Please fix that by catching appropriate security exceptions. I have also added review comments to that effect. 

Thanks for that, I'll have a look ASAP.

Regards,
Sébastien.

 

[Sébastien Le Callonnec]

Hi Inder,

I have gone through your comments, and here are a few comments/questions.

- You indicate you’d like Bookmark to be package-private, is this
something you feel strongly about? It is currently used by a class in a
different package, hence the public, but I could add a factory method
(possibly in BookmarkManager).
- Some of the comments are style elements, and I’d like to document them
for potential future contributors; is the Google code Wiki the right
place, or are you maintaining everything in your Google pages?
- Finally, I’d rather have one rev for all the changes following the
review, is that ok with you?

Regards,
Sébastien.

[Inderjeet Singh]

On Mon, Aug 15, 2011 at 12:44 PM, Sébastien Le Callonnec <slc...@yahoo.ie> wrote:

Hi Inder,

I have gone through your comments, and here are a few comments/questions.

- You indicate you’d like Bookmark to be package-private, is this something you feel strongly about?  It is currently used by a class in a different package, hence the public, but I could add a factory method (possibly in BookmarkManager).

No, I dont feel strongly about it. tcpmon is a tool, not a library so the API is not a huge deal.

 

- Some of the comments are style elements, and I’d like to document them for potential future contributors; is the Google code Wiki the right place, or are you maintaining everything in your Google pages?

Yes, I am maintaining everything on Google sites page. I have given you edit access on the sites page.

 

- Finally, I’d rather have one rev for all the changes following the review, is that ok with you?

Yes, that sounds good. 

Thanks

Inder

[Sébastien Le Callonnec]

Hi Inder,

Just to let you know, I have just pushed the changes following your
feedback on the previous revs.

I haven�t found an applet in the codebase, so I created one locally to
test the security manager issues you previously referred to. It is now
addressed, but please confirm that this is now working in your env too.

Thanks a mil,
S�bastien.

[Inderjeet Singh]

Hi Sebastien, 

Thanks for the changes, I will review them soon and let you know. 

Sorry, I was confused a little: I said applet but I actually meant a Java Web Start application.

Inder

On Sat, Aug 20, 2011 at 4:50 AM, Sébastien Le Callonnec <slc...@yahoo.ie> wrote:

Hi Inder,

Just to let you know, I have just pushed the changes following your feedback on the previous revs.

I haven’t found an applet in the codebase, so I created one locally to test the security manager issues you previously referred to.  It is now addressed, but please confirm that this is now working in your env too.

Thanks a mil,
Sébastien.

[Inderjeet Singh]

Hi Sebastien, 

I have reviewed the changelist and it looks good. Thanks for making the changes. 

I have one minor nitpick: 

Please dont use final on method parameters or local variables unless necessary.

While I love Immutable objects, I am not a fan of final method parameters or local variables. They usually mislead people into thinking that the object has become immutable which is not true (only the object reference is immutable). Sure, final is required when you pass those variables to an inner class but thats about the only use-case that is worth using final for. 

Also, when you are resolving comments, it would be great if you can respond there in-line with the decision. You dont have to go back and do it, but something worth doing in future. 

Thanks

Inder