Ignition 8 Connector v2 - Pull Request #3

[alexjhawk]

This is PR 3 for V2.0.0 of the Ignition connector. PR 4 will (should) be the last one and will include the implementation of all new functionality + additional Javadocs, etc.

[TomKimsey]

@alexjhawk do you want to just make these changes as some of my review is redundant.

[alexjhawk]

@alexjhawk do you want to just make these changes as some of my review is redundant.

I'll make these changes and re-request your review if you want to hold off.

[TomKimsey]

@alexjhawk do you want to just make these changes as some of my review is redundant.

I'll make these changes and re-request your review if you want to hold off.

Yeah i think thats the best way forward.

[alexjhawk]

@it-hms Will add the appropriate throws statements where applicable.

The idea with the response/request classes was to implement them in an SDK format to be portable or split into a separate library.

Adding try/catch statements removes the calling application from part of the decision process on how to handle the error. In some cases, this would force us to add log output which reduces the portability of the code significantly too.

[alexjhawk]

@it-hms @TomKimsey Previously, the topic of unchecked and checked exceptions has come up. I think the outcome at that point was to document the @ throws in Javadocs.

I think at some point in the future, we need to re-organize and refine our SC Git guidelines, preferably in Markdown format in GitHub for ease of access and manageability.

Unchecked vs. checked exception throwing/catching is inconsistent across our various projects. In some cases, I think guidelines should be defined where unchecked exceptions need/need not be identified or thrown. Some methods we implement may negate the possibility of an unchecked exception for example.

[TomKimsey]

@it-hms @TomKimsey Previously, the topic of unchecked and checked exceptions has come up. I think the outcome at that point was to document the @ throws in Javadocs.

I think at some point in the future, we need to re-organize and refine our SC Git guidelines, preferably in Markdown format in GitHub for ease of access and manageability.

Unchecked vs. checked exception throwing/catching is inconsistent across our various projects. In some cases, I think guidelines should be defined where unchecked exceptions need/need not be identified or thrown. Some methods we implement may negate the possibility of an unchecked exception for example.

What are the circumstances you see where known possible exceptions do "need not be identified or thrown"?

[alexjhawk]

@it-hms @TomKimsey Previously, the topic of unchecked and checked exceptions has come up. I think the outcome at that point was to document the @ throws in Javadocs. I think at some point in the future, we need to re-organize and refine our SC Git guidelines, preferably in Markdown format in GitHub for ease of access and manageability. Unchecked vs. checked exception throwing/catching is inconsistent across our various projects. In some cases, I think guidelines should be defined where unchecked exceptions need/need not be identified or thrown. Some methods we implement may negate the possibility of an unchecked exception for example.

What are the circumstances you see where known possible exceptions do "need not be identified or thrown"?

There are many instances, but one example off the top of my head is:

• We use a function that throws IllegalArgumentException if a specified integer parameter is a negative value.
  • The input we provide to the function is from a UI number spinner control that only allows values above 0.
  • or the input we provide to the function is from a field already checked elsewhere and cannot be negative.

In infinitely similar scenarios, explicitly handling unchecked exceptions may be redundant and unnecessary. In these cases, the developer should consider whether the method/function is being used as intended (nothing else) and whether the input supplied is valid. Unchecked exceptions typically indicate that a function or method is being misused, which involves some responsibility of the developer for not properly guarding the call, if applicable.

An example in this PR would be https://github.com/hms-networks/IgnitionEwonConnector/pull/142#discussion_r1224421180, as the unchecked exceptions thrown via getBoolean() should not be applicable given the getter method implementation. They should only be applicable when we are unsure that the field is present. In this case, something else would have to gone wrong for an exception to occur here.

[it-hms]

@it-hms @TomKimsey Previously, the topic of unchecked and checked exceptions has come up. I think the outcome at that point was to document the @ throws in Javadocs. I think at some point in the future, we need to re-organize and refine our SC Git guidelines, preferably in Markdown format in GitHub for ease of access and manageability. Unchecked vs. checked exception throwing/catching is inconsistent across our various projects. In some cases, I think guidelines should be defined where unchecked exceptions need/need not be identified or thrown. Some methods we implement may negate the possibility of an unchecked exception for example.

What are the circumstances you see where known possible exceptions do "need not be identified or thrown"?

There are many instances, but one example off the top of my head is:

• We use a function that throws IllegalArgumentException if a specified integer parameter is a negative value.

  • The input we provide to the function is from a UI number spinner control that only allows values above 0.
  • or the input we provide to the function is from a field already checked elsewhere and cannot be negative.

In infinitely similar scenarios, explicitly handling unchecked exceptions may be redundant and unnecessary. In these cases, the developer should consider whether the method/function is being used as intended (nothing else) and whether the input supplied is valid. Unchecked exceptions typically indicate that a function or method is being misused, which involves some responsibility of the developer for not properly guarding the call, if applicable.

An example in this PR would be #142 (comment), as the unchecked exceptions thrown via getBoolean() should not be applicable given the getter method implementation. They should only be applicable when we are unsure that the field is present. In this case, something else would have to gone wrong for an exception to occur here.

We can have a through discussion on this matter, but here is my quick take :

I prefer a bubble up approach. Catching should occur where a decision on what to do next occurs. This means exceptions "bubble up". So where to catch depends on the exception, and what the code can do to move on. Generally, catching the exception where it occurs is a bad, because the code doesn't really know what to do to move on.

Most exceptions should be identified. Please keep in mind that code may be changed or worked on by other developers. We should never trust API responses, user supplied data and general defensive coding should not trust passed parameters that may result in exceptions.

In the contrived example getLong can throw an exception with a bad parameter, but is not really worth identifying because we are fixing the startDate and the Field parameters.

private static final LocalDateTime startDate =   LocalDateTime.of(1994, Month.APRIL, 15, 11, 300);

    public long getStartHour(){
        return startDate.getLong(ChronoField.CLOCK_HOUR_OF_DAY);
    }