[
https://issues.apache.org/jira/browse/OFBIZ-6783?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15318126#comment-15318126 ]
Jacques Le Roux commented on OFBIZ-6783:
----------------------------------------
Hi Taher,
I just reviewed. For the 1st time, a start component review was (almost) straightforward, with useful comments and all. I remember how I was lost the 1st time I naively started to try to understand OFBiz code by reviewing the start class. Kudos, this was not an easy task!
All works as expected (including the tests and the pos and both options)
I agree with Jacopo's recommendations, I'd maybe just add a comment about the ofbiz.admin.port use in the tops of the classes. This port and its usage are quite important, so better let's new users not miss it.
Not sure why you removed commandExistsInList(), found it was more readeable before.
I'd remove the "StartupControlPanel." prefixes in StartupControlPanel class :)
Ah, I just found this line at the end of a tests run or quitting from the POS using the both option
bq. [java] 2016-06-07 08:57:03,743 FATAL Unable to register shutdown hook because JVM is shutting down.
but I don't think we should mind since after all the tests and the POS work as expected
I also noticed something: you can't stop the backend (not POS part) using "ant stop" when you start with "ant start-both" (this was working before, see R15.12), you get
{code}
stop:
[java] Start.java using configuration file org/ofbiz/base/start/start.properties
[java] Set OFBIZ_HOME to - C:/projectASF-Mars/ofbiz
[java] Shutting down server : FAIL
BUILD SUCCESSFUL
Total time: 1 second
{code}
But you can still quit all using the POS, which is fine with me.
> Refactor the start component
> ----------------------------
>
> Key: OFBIZ-6783
> URL:
https://issues.apache.org/jira/browse/OFBIZ-6783> Project: OFBiz
> Issue Type: Improvement
> Components: framework
> Affects Versions: Upcoming Branch
> Reporter: Taher Alkhateeb
> Assignee: Taher Alkhateeb
> Labels: framework, main, refactoring, start
> Attachments: OFBIZ-6783-hiddenfiles.patch, OFBIZ-6783.patch, OFBIZ-6783.patch, OFBIZ-6783.patch, OFBIZ-6783.patch, OFBIZ-6783.patch, OFBIZ-6783.patch, OFBIZ-6783.patch, OFBIZ-6783.patch, OFBIZ-6783.patch, OFBIZ-6783.patch, OFBIZ-6783.patch, OFBIZ-6783.patch, OFBIZ-6783.patch, OFBIZ-6783.patch, OFBIZ-6783.patch, OFBIZ-6783.patch, OFBIZ-6783.patch, OFBIZ-6783.patch, OFBIZ-6783.patch, StartCommandUtil.java, error.log, ofbiz.log
>
>
> Looking at the main method and design of Start.java and the start component overall looks ugly. The things I would like to fix so far are:
> - the files are too long
> - some variables are not even needed (loaderArgs?)
> - the level of abstraction is wrong
> - main throws an exception!
> - the arguments processing logic is terrible, need to move it to commons-cli
> It's just so messy and ugly to look at. So for me refactoring starts at Start! Given that this is an important component, I will provide a patch to be reviewed by the community before committing just to be on the safe side.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
Locked
