Damian Krzeminski wrote: > line 1: why check for null beanId - if something calls this method > with null I'd rather it fails loudly since we probably have a bug > somewhere else, and if we were to check for null why here and not at > the beginning of the method? I agree about checking for null at the beginning of the method, but not sure I agree about letting it throw a NullPointerException. I would rather have it print an error to the log, but we have had this argument before about reporting an error with logging or some other way vs just letting it throw the exception. I don't think the UI is the place for throwing up stack traces, especially for a NullPointerException.
Although in this case, it should in theory be extremely unlikely for the beanId to be null - the null check was added when a NullPointerException was thrown while running the ServicesTable unit test. So I guess in this case, due to its extremely low likelihood of happening, I'll remove the null check and let it fail loudly. But I reserve the right to argue with you about this in the future on other issues. :-) > line 2 and 3: we remove the service in line 2, why call > m_services.remove in line 3 passing a String and not a Service > object? This one was a mistake on my part - git am could not automatically apply your patch so I applied it by hand - should have removed line 3 altogether - I will correct that. I will make the changes mentioned and commit to SVN. _______________________________________________ sipx-dev mailing list [email protected] List Archive: http://list.sipfoundry.org/archive/sipx-dev Unsubscribe: http://list.sipfoundry.org/mailman/listinfo/sipx-dev
