Core and space-syste-model

[Lauri Kimmel]

I'm working on https://github.com/JohannesKlug/hbird/issues/1 and discovered few issues with current API and implementation:

1. TmTcGroup has methods like addXXXParameter(String qualifiedName, Parameter<XXX> parameter) and Parameter has method getQualifiedName(). If there is no explicit need I'd like to change addXXXParameter(String qualifiedName, Parameter<XXX> parameter) to addXXXParameter(Parameter<XXX> parameter). Otherwise it will be possible to map parameter to wrong name - group.addIntegerParameter("int", new SomeParameterImpl<Integer>("other-int")). Other option is to check this in TmTcGroup implemention (in HummingbirdParameterGroup in current case) which doesn't make much sense to me at the moment.

2. I'd suggest refactoring methods replaceParameterInGroup and copyAllParameterValues out from the TmTcGroup. I think they are more related to "business logic" than "domain objects" (which doesn't necessarily mean they have to be removed from the space-system-model).

3. In several places like HummingbirdParameterGroup.copyAllParameterValues() and XcteSpaceSystemModelFactory.createSpaceSystemModel() System.exit() is called. My personal opinion is that may cause problems for Hbird users in the future. I had one similar case in the past - jar fail had to be signed on servlet request and sent back to browser as HTTP response. I was using JDK-s jar signer. Each time signing failed for some reason it called System.exit() and killed running Tomcat server by doing so. For example if Hbird is deployed to some OSGI container it will kill the whole system at once. That's why I strongly suggest to review all System.exit() calls and use appropriate exceptions instead – eg. RuntimeException can be handled by higher level components if needed without killing the whole app. Also it is impossible to fully unit test Java code which is shutting down the JVM at one point.

[Mark Doyle]

1) There id definitely a need for the Qualified name. I can't think of a quick example but we looked at this for a while.

2) Again, I've forgotten the specific reason but I know it was related to changing object values in a collection using runtime untyped "<?>" Parameters. Java pass by reference isn't the same as C++, the reference is equivalent to a pointer that is copied. Perhaps i'll find time to get the actual use-case. You might be correct that these exist only for another component but it isn't an app level one. The payload codec uses them to take values from it's decorated space system model (with CodecAwareParameters<T>) and populate the original model. This means it could be payload codec specific but i'm not sure. It's certainly not harming anything and doesn't require implementing again as the XtceSpaceSystemModelFactory already does so. If you haev a use case where you need to replace/copy parameter values in the models collections AND can do so without using those methods just don't use them. Comments in this code certainly need to be improved. We don't expect major changes though so a guide to creating an XTCE model will probably help more.

3) The System exits were probably left from a template before we looked at exception handling. I think they were all at a fundamental point where there was no way to fox the problem. If the Space system definition input (e.g. xtce file) is incorrect there is no way to save the system or run it. It's pointless. That said, they shouldn't really be there. There will be a better alternative.

I think all this is correct. I've just got home and don't have the code in-front of me here.

[Mark Doyle]

Quick addition; most of the complexities down here exist to ease the life of the application level developer and circumvent some generics shortcoming.

Things like the qualified name should only effect development of space system model factories which have to be very well unit tested anyway.

[Lauri Kimmel]

I'm writing unit tests for the HummingbirdParameterGroup at the moment.

1. Behavior of addXXXParameter(String, Parameter) and replaceParameterInGroup(String, Parameter) are inconsistent – add accepts case where qualifiedName in not equal to parameter.getQualifiedName(), replace is not working in this case. AFAIK model should not accept parameters without qualifiedName (Parameter.getQualifiedName() == null) – so I don't see the point of duplicating this information in addXXXParameter signature.

2. Functionality of methods copyAllParameterValues and replaceParameterInGroup is out of doubt. Still I'm certain they can live without problems as static methods in utility class – eg. ParameterGroups.copyAllParameterValues(source, target) and ParamterGroups.replaceParemeterInGroup(source, qualifiedName, parameter). This way the logic can be implemented and tested only once regardless number of ParameterGroup implementations.

3. I will add TODO-s to all System.exit() calls and create new issue in github.

[Mark Doyle]

1) Agreed. After a quick glance at the code it looks like we can simply pass the parameter in and add it to the Map using the qualified named from the Parameter itself.

2) Agreed, if you can be bothered moving it without breaking the system please do :) Otherwise a low priority issue will suffice.

3) Cool.

[Lauri Kimmel]

1, 2 - done and pushed to the github. 

3 - created issue https://github.com/JohannesKlug/hbird/issues/5

  

Added unit test. 

Maven build was successful. 

So I hope i didn't break anything =)

[Mark Doyle]

Looks great.

I've been thinking about our unit tests recently and how they've become project tests. Found out about Mockito recently too and wondered about using it so your test is perfect reading :)

[Johannes Klug]

Great news! I'll have a look at the code after work once I'm home.

Am 2012-03-29 09:15, schrieb Lauri Kimmel:
> 1, 2 - done and pushed to the github.
> 3 - created issue https://github.com/JohannesKlug/hbird/issues/5
>
> Added unit test.
> Maven build was successful.
> So I hope i didn't break anything =)
>
> On Tuesday, March 27, 2012 3:15:53 PM UTC+3, Mark wrote:
>
>> 1) Agreed. After a quick glance at the code it looks like we can
>> simply pass the parameter in and add it to the Map using the
>> qualified named from the Parameter itself.
>> 2) Agreed, if you can be bothered moving it without breaking the
>> system please do :) Otherwise a low priority issue will suffice.
>> 3) Cool.
>>

>> On 27 March 2012 11:20, Lauri Kimmel <lauri....@gmx.com [4]>

>>>> On 26 March 2012 22:44, Mark Doyle <markjo...@gmail.com [3]>

>>>>> [2]> wrote:
>>>>>
>>>>>> I'm working on

>>>>>> https://github.com/JohannesKlug/hbird/issues/1 [1] and

> Links:
> ------
> [1] https://github.com/JohannesKlug/hbird/issues/1
> [2] mailto:lauri....@gmx.com
> [3] mailto:markjo...@gmail.com
> [4] mailto:lauri....@gmx.com