beanit / asn1bean

ASN1bean (formerly known as jASN1) is a Java ASN.1 BER and DER encoding/decoding library
https://www.beanit.com/asn1/
Apache License 2.0
110 stars 46 forks source link

change encode method's arguement to OutputStream #2

Closed onderson closed 6 years ago

onderson commented 7 years ago

Instead of having method signature as below; public int encode(BerByteArrayOutputStream os) throws IOException I would suggest having the signature like public int encode(OutputStream os) throws IOException This style would be more abstract and for example i would not need such import dependency in my camel-asn1 compiler where this would enable for third party developer's like me to have more loosly-coupled dependency in component library because i would not have such import

See; https://github.com/onders86/camel/blob/CAMEL-ASN1/components/camel-asn1/src/main/java/org/apache/camel/dataformat/asn1/ASN1DataFormat.java#L35

Thanks

sfeuerhahn commented 7 years ago

the only problem I have with your idea, is that I leads to the false believe that the encode function could be used with any output stream. But actually the data is written in reverse order to the output stream and the BerByteArrayOutputStream takes special care of that fact by writing to a byte array starting at its end.

onderson commented 7 years ago

Yes agree, there alreays stands the same point for decode and input stream. This PR is not a big deal but nice to have IMHO.

onderson commented 7 years ago

Any more comments?

onderson commented 7 years ago

Btw, in apache camel project, i implemented asn1 component using this library. If you wanna check out, please go ahead. This context of such proposal was to extend camel component without any direct dependency on jasn1. Please consider the change, let me know..

sfeuerhahn commented 7 years ago

Ok it has the disadvantage that it makes you think you could use any outputstream but I guess I can live with that since it has the advantage that one can more easily use a different outputstream implementation. Can you adjust your patch so that all classes, also those of the jasn1 project like BerInteger use OutputStream instead of BerOutputstream ? This way you don't have to do as much casting..

onderson commented 7 years ago

No no no, i think you make a wrong assumption what it makes me think. I know and am aware that the output stream should BerOutputStream. And what i want to have and suggest why not we could not have Outputstream in the method signature which would help me drop of static import in camel-asn1 component and maybe integrate another asn1 libabries easily. The reason of that the input stream again should be a proper input stream and that would not hurt, i suppose.

I will include what you suggest in this PR. It is fair and rescues us from those ugly castings. You are right

sfeuerhahn commented 7 years ago

I did understand you! I was just saying that if we change to OutputStream in the method signature, that besides the advantage you mentioned, it has the second advantage that it would be theoretically feasible to implement another BerOutputStream that behaves somewhat different and use that one.

sfeuerhahn commented 7 years ago

when do you plan to make the changes to the PR?

onderson commented 7 years ago

I have been traveling since 6th of october till 18th october. That week or over the same weekend, i will be able to look into it.

onderson commented 6 years ago

@sfeuerhahn please see ref

onderson commented 6 years ago

one more question. When do you think the new release may come out? I want to update the component as well. Could you please let me know? thanks