You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by "Aidan Skinner (JIRA)" <qp...@incubator.apache.org> on 2008/12/09 15:06:44 UTC
[jira] Created: (QPID-1522) Refactor Command classes so that more
common code is shared.
Refactor Command classes so that more common code is shared.
-------------------------------------------------------------
Key: QPID-1522
URL: https://issues.apache.org/jira/browse/QPID-1522
Project: Qpid
Issue Type: Improvement
Components: Java Management : CLI Tool
Affects Versions: M4
Reporter: Aidan Skinner
Assignee: Aidan Skinner
The Command subclasses have some duplicate code which could be shared.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (QPID-1522) Refactor Command classes so that more
common code is shared.
Posted by "Rob Godfrey (JIRA)" <qp...@incubator.apache.org>.
[ https://issues.apache.org/jira/browse/QPID-1522?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12662311#action_12662311 ]
Rob Godfrey commented on QPID-1522:
-----------------------------------
Comments on checking r732307:
CommandImpl.java:
CommandImpl seems to have the Apache license in twice
coding conventions not consistently followed
Commandinfo.java:
modified echo statements do not correct typos:
"You might be quering wrong"...
"Objects might not in the broker currently"
> Refactor Command classes so that more common code is shared.
> -------------------------------------------------------------
>
> Key: QPID-1522
> URL: https://issues.apache.org/jira/browse/QPID-1522
> Project: Qpid
> Issue Type: Improvement
> Components: Java Management : CLI Tool
> Affects Versions: M4
> Reporter: Aidan Skinner
> Assignee: Aidan Skinner
> Attachments: 0001-QPID-1522-Raname-Command-class-to-CommandImpl-and-m.patch, 0002-QPID-1522-Move-common-code-up-to-CommandImpl.-Renam.patch, 0003-QPID-1522-Fix-spelling-error-in-classname.patch, 0004-QPID-1522-Move-command-line-constants-to-individual.patch
>
>
> The Command subclasses have some duplicate code which could be shared.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (QPID-1522) Refactor Command classes so that more
common code is shared.
Posted by "Robert Gemmell (JIRA)" <qp...@incubator.apache.org>.
[ https://issues.apache.org/jira/browse/QPID-1522?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12662340#action_12662340 ]
Robert Gemmell commented on QPID-1522:
--------------------------------------
Rob Godfrey - 09/Jan/09 03:00 AM
Comments on checking r732310:
JMXConnectionFactory:
The code which looks at the exception text seems remarkable fragile to me... At the very least I would expect some constants which could be shared with the code that throws the exception.
==================
Hi Rob,
I wrote that bit originally. It is somewhat fragile, but unfortunately having looked at the code that generates the exceptions i didnt think there was much else i could do. The exception is generated by code within the optional JMXMP addon (jmxremote_optional.jar) and just directly throws an IOException like so:
throw new IOException("The server supported profiles " +
serverProfilesList + " do not " +
"match the client required profiles " +
clientProfilesList + ".");
so there are no constants to match against (though granted i should possibly still have used constants for my own bit).
I have a secure RMI based JMX setup ready to go, so we can remove the user-unfriendly/slightly-proprietary JMXMP stuff in future if desired (I know Aidan wants to :P), and if so then that section of code should only really ever be getting used for connecting to older brokers if they have used the JMXMP based solution, which it seems that at the moment many dont, given how guff the console was in that area.
> Refactor Command classes so that more common code is shared.
> -------------------------------------------------------------
>
> Key: QPID-1522
> URL: https://issues.apache.org/jira/browse/QPID-1522
> Project: Qpid
> Issue Type: Improvement
> Components: Java Management : CLI Tool
> Affects Versions: M4
> Reporter: Aidan Skinner
> Assignee: Aidan Skinner
> Attachments: 0001-QPID-1522-Raname-Command-class-to-CommandImpl-and-m.patch, 0002-QPID-1522-Move-common-code-up-to-CommandImpl.-Renam.patch, 0003-QPID-1522-Fix-spelling-error-in-classname.patch, 0004-QPID-1522-Move-command-line-constants-to-individual.patch
>
>
> The Command subclasses have some duplicate code which could be shared.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (QPID-1522) Refactor Command classes so that more
common code is shared.
Posted by "Aidan Skinner (JIRA)" <qp...@incubator.apache.org>.
[ https://issues.apache.org/jira/browse/QPID-1522?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Aidan Skinner updated QPID-1522:
--------------------------------
Status: Ready To Review (was: In Progress)
> Refactor Command classes so that more common code is shared.
> -------------------------------------------------------------
>
> Key: QPID-1522
> URL: https://issues.apache.org/jira/browse/QPID-1522
> Project: Qpid
> Issue Type: Improvement
> Components: Java Management : CLI Tool
> Affects Versions: M4
> Reporter: Aidan Skinner
> Assignee: Aidan Skinner
> Attachments: 0001-QPID-1522-Raname-Command-class-to-CommandImpl-and-m.patch, 0002-QPID-1522-Move-common-code-up-to-CommandImpl.-Renam.patch, 0003-QPID-1522-Fix-spelling-error-in-classname.patch, 0004-QPID-1522-Move-command-line-constants-to-individual.patch
>
>
> The Command subclasses have some duplicate code which could be shared.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (QPID-1522) Refactor Command classes so that more
common code is shared.
Posted by "Rob Godfrey (JIRA)" <qp...@incubator.apache.org>.
[ https://issues.apache.org/jira/browse/QPID-1522?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12662308#action_12662308 ]
Rob Godfrey commented on QPID-1522:
-----------------------------------
Comments on checkin r732306:
General Comment On Original Code: does not conform to coding standards ({ should be on new line), class and method names do not follow normal Upper/Lower-Camel conventions, member variables are not prefixed with _. Also the classes seem to have a lot of auto-generated cruft ( "To change this template use File | Settings | File Templates") that should be removed
1. CommandImpl itself doesn't seem to be part of this checkin which presumably broke the build for a short time
2 Change to CommandExecusionEngine (which should have it's name fixed to CommandExecutionEngine):
import changed from Command to CommandImpl, however no use of CommandImpl in the file - also it immediately follows an import commands.* which makes the changed import redundant.
> Refactor Command classes so that more common code is shared.
> -------------------------------------------------------------
>
> Key: QPID-1522
> URL: https://issues.apache.org/jira/browse/QPID-1522
> Project: Qpid
> Issue Type: Improvement
> Components: Java Management : CLI Tool
> Affects Versions: M4
> Reporter: Aidan Skinner
> Assignee: Aidan Skinner
> Attachments: 0001-QPID-1522-Raname-Command-class-to-CommandImpl-and-m.patch, 0002-QPID-1522-Move-common-code-up-to-CommandImpl.-Renam.patch, 0003-QPID-1522-Fix-spelling-error-in-classname.patch, 0004-QPID-1522-Move-command-line-constants-to-individual.patch
>
>
> The Command subclasses have some duplicate code which could be shared.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (QPID-1522) Refactor Command classes so that more
common code is shared.
Posted by "Aidan Skinner (JIRA)" <qp...@incubator.apache.org>.
[ https://issues.apache.org/jira/browse/QPID-1522?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Aidan Skinner updated QPID-1522:
--------------------------------
Attachment: 0002-QPID-1522-Move-common-code-up-to-CommandImpl.-Renam.patch
> Refactor Command classes so that more common code is shared.
> -------------------------------------------------------------
>
> Key: QPID-1522
> URL: https://issues.apache.org/jira/browse/QPID-1522
> Project: Qpid
> Issue Type: Improvement
> Components: Java Management : CLI Tool
> Affects Versions: M4
> Reporter: Aidan Skinner
> Assignee: Aidan Skinner
> Attachments: 0001-QPID-1522-Raname-Command-class-to-CommandImpl-and-m.patch, 0002-QPID-1522-Move-common-code-up-to-CommandImpl.-Renam.patch, 0003-QPID-1522-Fix-spelling-error-in-classname.patch, 0004-QPID-1522-Move-command-line-constants-to-individual.patch
>
>
> The Command subclasses have some duplicate code which could be shared.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (QPID-1522) Refactor Command classes so that more
common code is shared.
Posted by "Rob Godfrey (JIRA)" <qp...@incubator.apache.org>.
[ https://issues.apache.org/jira/browse/QPID-1522?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12662321#action_12662321 ]
Rob Godfrey commented on QPID-1522:
-----------------------------------
Comments on checking r732310:
JMXConnectionFactory:
The code which looks at the exception text seems remarkable fragile to me... At the very least I would expect some constants which could be shared with the code that throws the exception.
Command.java:
public static String COMMAND_NAME = null;
????? Why do we need a non-final global variable in this interface, which is initially set to null. At the very least it should have some doc in the class... It seems like this is a bug waiting to happen. I think it is there due to a misunderstanding of how statics work in interfaces. They are not inherited as a template by the implementing classes. References to statics are *always* resolved at compile time, not at run time. echo("Try `" + COMMAND_NAME + " --help ` for more information"); in ComandImpl will *always* evaluate to + null +
CommandExecutionEngine:
Suggestion: instead of using reflection (.newInstance) use a factory interface and have a factory for each command type. That way you won't have to deal with unexpected exceptions, etc...
Commandget:
Usual double license (one backdated to 2006) problem
typo: quering instead of query / return rather than returned
typo: you might be quering wrong
typo: Type Objects might not in the broker currently
Similar typos in Commandset (perhaps the typos can be factored out into some constants/properties since the same string seem to be reproduced in many places... and if we ever did want to provide other languages... etc)
> Refactor Command classes so that more common code is shared.
> -------------------------------------------------------------
>
> Key: QPID-1522
> URL: https://issues.apache.org/jira/browse/QPID-1522
> Project: Qpid
> Issue Type: Improvement
> Components: Java Management : CLI Tool
> Affects Versions: M4
> Reporter: Aidan Skinner
> Assignee: Aidan Skinner
> Attachments: 0001-QPID-1522-Raname-Command-class-to-CommandImpl-and-m.patch, 0002-QPID-1522-Move-common-code-up-to-CommandImpl.-Renam.patch, 0003-QPID-1522-Fix-spelling-error-in-classname.patch, 0004-QPID-1522-Move-command-line-constants-to-individual.patch
>
>
> The Command subclasses have some duplicate code which could be shared.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (QPID-1522) Refactor Command classes so that more
common code is shared.
Posted by "Aidan Skinner (JIRA)" <qp...@incubator.apache.org>.
[ https://issues.apache.org/jira/browse/QPID-1522?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Aidan Skinner updated QPID-1522:
--------------------------------
Attachment: 0003-QPID-1522-Fix-spelling-error-in-classname.patch
> Refactor Command classes so that more common code is shared.
> -------------------------------------------------------------
>
> Key: QPID-1522
> URL: https://issues.apache.org/jira/browse/QPID-1522
> Project: Qpid
> Issue Type: Improvement
> Components: Java Management : CLI Tool
> Affects Versions: M4
> Reporter: Aidan Skinner
> Assignee: Aidan Skinner
> Attachments: 0001-QPID-1522-Raname-Command-class-to-CommandImpl-and-m.patch, 0002-QPID-1522-Move-common-code-up-to-CommandImpl.-Renam.patch, 0003-QPID-1522-Fix-spelling-error-in-classname.patch, 0004-QPID-1522-Move-command-line-constants-to-individual.patch
>
>
> The Command subclasses have some duplicate code which could be shared.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (QPID-1522) Refactor Command classes so that more
common code is shared.
Posted by "Aidan Skinner (JIRA)" <qp...@incubator.apache.org>.
[ https://issues.apache.org/jira/browse/QPID-1522?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12663000#action_12663000 ]
Aidan Skinner commented on QPID-1522:
-------------------------------------
I've split the concerns about JMXConnectionFactory into another jira, QPID-1570
> Refactor Command classes so that more common code is shared.
> -------------------------------------------------------------
>
> Key: QPID-1522
> URL: https://issues.apache.org/jira/browse/QPID-1522
> Project: Qpid
> Issue Type: Improvement
> Components: Java Management : CLI Tool
> Affects Versions: M4
> Reporter: Aidan Skinner
> Assignee: Aidan Skinner
> Attachments: 0001-QPID-1522-Raname-Command-class-to-CommandImpl-and-m.patch, 0002-QPID-1522-Move-common-code-up-to-CommandImpl.-Renam.patch, 0003-QPID-1522-Fix-spelling-error-in-classname.patch, 0004-QPID-1522-Move-command-line-constants-to-individual.patch
>
>
> The Command subclasses have some duplicate code which could be shared.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (QPID-1522) Refactor Command classes so that more
common code is shared.
Posted by "Rob Godfrey (JIRA)" <qp...@incubator.apache.org>.
[ https://issues.apache.org/jira/browse/QPID-1522?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12662312#action_12662312 ]
Rob Godfrey commented on QPID-1522:
-----------------------------------
Comments on checking r732308:
New classes (Test)CommandExecutionEngine have the Apache licence in twice, once backdating the copyright to 2006... (this seems like a common problem across this part of the codebase)
> Refactor Command classes so that more common code is shared.
> -------------------------------------------------------------
>
> Key: QPID-1522
> URL: https://issues.apache.org/jira/browse/QPID-1522
> Project: Qpid
> Issue Type: Improvement
> Components: Java Management : CLI Tool
> Affects Versions: M4
> Reporter: Aidan Skinner
> Assignee: Aidan Skinner
> Attachments: 0001-QPID-1522-Raname-Command-class-to-CommandImpl-and-m.patch, 0002-QPID-1522-Move-common-code-up-to-CommandImpl.-Renam.patch, 0003-QPID-1522-Fix-spelling-error-in-classname.patch, 0004-QPID-1522-Move-command-line-constants-to-individual.patch
>
>
> The Command subclasses have some duplicate code which could be shared.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Issue Comment Edited: (QPID-1522) Refactor Command classes
so that more common code is shared.
Posted by "Rob Godfrey (JIRA)" <qp...@incubator.apache.org>.
[ https://issues.apache.org/jira/browse/QPID-1522?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12662308#action_12662308 ]
rgodfrey edited comment on QPID-1522 at 1/9/09 2:36 AM:
-----------------------------------------------------------
Comments on checkin r732306:
General Comment On Original Code: does not conform to coding standards ({ should be on new line), class and method names do not follow normal Upper/Lower-Camel conventions, member variables are not prefixed with _. Also the classes seem to have a lot of auto-generated cruft ( "To change this template use File | Settings | File Templates") that should be removed
1. CommandImpl itself doesn't seem to be part of this checkin which presumably broke the build for a short time
2 Change to CommandExecusionEngine
import changed from Command to CommandImpl, however no use of CommandImpl in the file - also it immediately follows an import commands.* which makes the changed import redundant.
was (Author: rgodfrey):
Comments on checkin r732306:
General Comment On Original Code: does not conform to coding standards ({ should be on new line), class and method names do not follow normal Upper/Lower-Camel conventions, member variables are not prefixed with _. Also the classes seem to have a lot of auto-generated cruft ( "To change this template use File | Settings | File Templates") that should be removed
1. CommandImpl itself doesn't seem to be part of this checkin which presumably broke the build for a short time
2 Change to CommandExecusionEngine (which should have it's name fixed to CommandExecutionEngine):
import changed from Command to CommandImpl, however no use of CommandImpl in the file - also it immediately follows an import commands.* which makes the changed import redundant.
> Refactor Command classes so that more common code is shared.
> -------------------------------------------------------------
>
> Key: QPID-1522
> URL: https://issues.apache.org/jira/browse/QPID-1522
> Project: Qpid
> Issue Type: Improvement
> Components: Java Management : CLI Tool
> Affects Versions: M4
> Reporter: Aidan Skinner
> Assignee: Aidan Skinner
> Attachments: 0001-QPID-1522-Raname-Command-class-to-CommandImpl-and-m.patch, 0002-QPID-1522-Move-common-code-up-to-CommandImpl.-Renam.patch, 0003-QPID-1522-Fix-spelling-error-in-classname.patch, 0004-QPID-1522-Move-command-line-constants-to-individual.patch
>
>
> The Command subclasses have some duplicate code which could be shared.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (QPID-1522) Refactor Command classes so that more
common code is shared.
Posted by "Aidan Skinner (JIRA)" <qp...@incubator.apache.org>.
[ https://issues.apache.org/jira/browse/QPID-1522?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Aidan Skinner updated QPID-1522:
--------------------------------
Attachment: 0001-QPID-1522-Raname-Command-class-to-CommandImpl-and-m.patch
> Refactor Command classes so that more common code is shared.
> -------------------------------------------------------------
>
> Key: QPID-1522
> URL: https://issues.apache.org/jira/browse/QPID-1522
> Project: Qpid
> Issue Type: Improvement
> Components: Java Management : CLI Tool
> Affects Versions: M4
> Reporter: Aidan Skinner
> Assignee: Aidan Skinner
> Attachments: 0001-QPID-1522-Raname-Command-class-to-CommandImpl-and-m.patch, 0002-QPID-1522-Move-common-code-up-to-CommandImpl.-Renam.patch, 0003-QPID-1522-Fix-spelling-error-in-classname.patch, 0004-QPID-1522-Move-command-line-constants-to-individual.patch
>
>
> The Command subclasses have some duplicate code which could be shared.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (QPID-1522) Refactor Command classes so that more
common code is shared.
Posted by "Aidan Skinner (JIRA)" <qp...@incubator.apache.org>.
[ https://issues.apache.org/jira/browse/QPID-1522?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Aidan Skinner updated QPID-1522:
--------------------------------
Attachment: 0004-QPID-1522-Move-command-line-constants-to-individual.patch
> Refactor Command classes so that more common code is shared.
> -------------------------------------------------------------
>
> Key: QPID-1522
> URL: https://issues.apache.org/jira/browse/QPID-1522
> Project: Qpid
> Issue Type: Improvement
> Components: Java Management : CLI Tool
> Affects Versions: M4
> Reporter: Aidan Skinner
> Assignee: Aidan Skinner
> Attachments: 0001-QPID-1522-Raname-Command-class-to-CommandImpl-and-m.patch, 0002-QPID-1522-Move-common-code-up-to-CommandImpl.-Renam.patch, 0003-QPID-1522-Fix-spelling-error-in-classname.patch, 0004-QPID-1522-Move-command-line-constants-to-individual.patch
>
>
> The Command subclasses have some duplicate code which could be shared.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Assigned: (QPID-1522) Refactor Command classes so that more
common code is shared.
Posted by "Aidan Skinner (JIRA)" <qp...@incubator.apache.org>.
[ https://issues.apache.org/jira/browse/QPID-1522?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Aidan Skinner reassigned QPID-1522:
-----------------------------------
Assignee: Rob Godfrey (was: Aidan Skinner)
I've fixed the review comments that don't pertain to the login code itself, which I've split off into a seperate Jira.
> Refactor Command classes so that more common code is shared.
> -------------------------------------------------------------
>
> Key: QPID-1522
> URL: https://issues.apache.org/jira/browse/QPID-1522
> Project: Qpid
> Issue Type: Improvement
> Components: Java Management : CLI Tool
> Affects Versions: M4
> Reporter: Aidan Skinner
> Assignee: Rob Godfrey
> Attachments: 0001-QPID-1522-Raname-Command-class-to-CommandImpl-and-m.patch, 0002-QPID-1522-Move-common-code-up-to-CommandImpl.-Renam.patch, 0003-QPID-1522-Fix-spelling-error-in-classname.patch, 0004-QPID-1522-Move-command-line-constants-to-individual.patch
>
>
> The Command subclasses have some duplicate code which could be shared.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.