Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PROTON-1352] Trivial casting from UnsignedByte to SenderSettleMode and ReceiverSettleMode #89

Merged
merged 1 commit into from Nov 16, 2016

Conversation

ppatierno
Copy link
Contributor

Refactored ReceiverSettleMode and SenderSettleMode for having trivial casting from/to UnsignedByte
Added message annotations to the String visualization of AMQP message

Copy link
Member

@gemmellr gemmellr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple of tiny niggles around validation, and some tests of those changes would be good.

Also, when updating the commit can you include the JIRA keys so that the JIRAs get marked appropriately when the commits go in.

}

public static ReceiverSettleMode valueOf(UnsignedByte value) {
return map.get(value);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think its worth throwing IAE if an out of bound value is provided, rather than just returning null.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The map here seems unnecessary to me, the compiler adds a values method that returns an array of the enum values which you can just use to just do something like this

    public SenderSettleMode valueof(UnsignedByte value) {
        try {
            return values()[value.intValue()];
        } catch (Exception e) {
            throw new IllegalArgumentException();
        }
    }

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would probably just make it a switch statement. Of course, you could also use such a switch statement wherever this new method is going to be called from :)

Copy link
Contributor Author

@ppatierno ppatierno Nov 16, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tabish121 It's true but it works in our case just because enum values go from 0,1 to 2 (so they are good as indexes in the array). If one of the value was 100 for example, using value.intValue() as index for the values() array will throw an out of bound exception. Your suggestion was the way I was using outside this enum in my application before opening the PR for doing the cast. :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gemmellr I could be ok with switch but provided here as "out of box" :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm actually fine with adding the method here, which is why I didn't say that originally ;)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this case I'd go with the switch statement if we decided we need this method, simple, effective. Given the values are sequential there no need to use a solution based on what they could be.

}

public SenderSettleMode valueof(UnsignedByte value) {
return map.get(value);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As with the other mode.

@@ -769,6 +769,10 @@ public String toString()
sb.append("properties=");
sb.append(_properties);
}
if (_messageAnnotations != null) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should have its own JIRA. We can skip the separate PR, this time ;)

@gemmellr
Copy link
Member

Including a message as to why the IAE is thrown wouldn't go amiss :)

I think you missed my comment about raising a separate JIRA for the unrelated annotations change, then including its key in the commit too. Can you use a simple "key(s): message" format for the log, e.g. "PROTON-1234, PROTON-5678: foo" (see the previous commits for more: https://github.com/apache/qpid-proton/commits/master).

@gemmellr
Copy link
Member

I hadn't realised you had done separate commits...can you add the relevant JIRA key to the first commit, then squash the other two together as a single commit and give it the other key.

…alization of AMQP message

Refactored ReceiverSettleMode and SenderSettleMode for having trivial casting from/to UnsignedByte and added unit tests
@asfgit asfgit merged commit fec0bc8 into apache:master Nov 16, 2016
asfgit pushed a commit that referenced this pull request Nov 16, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants