[CRYPTO] inconsistent naming CryptoRandomFactory.getCryptoRandom : CryptoCipherFactory.getInstance

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

[CRYPTO] inconsistent naming CryptoRandomFactory.getCryptoRandom : CryptoCipherFactory.getInstance

sebb-2-2
As the subject says; the two factories use a different naming convention.

Would it be sensible to standardise on getInstance, given that the
class name says what the instance will be?

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [CRYPTO] inconsistent naming CryptoRandomFactory.getCryptoRandom : CryptoCipherFactory.getInstance

garydgregory
On Wed, Jun 29, 2016 at 4:53 PM, sebb <[hidden email]> wrote:

> As the subject says; the two factories use a different naming convention.
>
> Would it be sensible to standardise on getInstance, given that the
> class name says what the instance will be?
>

+1

Gary


>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>
>


--
E-Mail: [hidden email] | [hidden email]
Java Persistence with Hibernate, Second Edition
<http://www.manning.com/bauer3/>
JUnit in Action, Second Edition <http://www.manning.com/tahchiev/>
Spring Batch in Action <http://www.manning.com/templier/>
Blog: http://garygregory.wordpress.com
Home: http://garygregory.com/
Tweet! http://twitter.com/GaryGregory
Reply | Threaded
Open this post in threaded view
|

Re: [CRYPTO] inconsistent naming CryptoRandomFactory.getCryptoRandom : CryptoCipherFactory.getInstance

Stian Soiland-Reyes
In reply to this post by sebb-2-2
On 30 June 2016 at 00:53, sebb <[hidden email]> wrote:
> As the subject says; the two factories use a different naming convention.
>
> Would it be sensible to standardise on getInstance, given that the
> class name says what the instance will be?

Hm, but CryptoRandomFactory.getCryptoRandom() returns a CryptoRandom,
and CryptoCipherFactory.getInstance() returns a CryptoCipher() (not a
CryptoCipherFactory).

I think it would be misleading to call it getInstance() as that
implies the singleton pattern, e.g. to return the *Factory, but in
fact neither of the factories can be instantiated (private
constructor, only static methods).

Both methods take parameters, so it would be returning different
instances depending on the parameters - so although the ciphers etc.
might always be the same instances, it's not the singleton pattern.
(But then not quite the factory pattern either :) )


Agreed that the two factories should however be consistent, so I would suggest:


public class CryptoRandomFactory {
  public static CryptoRandom getCryptoRandom( .. ) { .. }
}

public class CryptoCipherFactory {
  public static CryptoCipher getCryptoCipher( ..)  { .. }
}


But I understand your argument.. because for the caller it will be
quite repetitive:

CryptoCipher cryptoCipher = CryptoCipherFactory.getCryptoCipher( .. ) ;
CryptoRandom cryptoRandom = CryptoRandomFactory.getCryptoRandom( .. ) ;


Perhaps just call the methods  just  .get(...) instead?


public class CryptoRandomFactory {
  public static CryptoRandom get( .. ) { .. }
}

public class CryptoCipherFactory {
  public static CryptoCipher get( ..)  { .. }
}



--
Stian Soiland-Reyes
Apache Taverna (incubating), Apache Commons
http://orcid.org/0000-0001-9842-9718

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [CRYPTO] inconsistent naming CryptoRandomFactory.getCryptoRandom : CryptoCipherFactory.getInstance

sebb-2-2
On 30 June 2016 at 11:01, Stian Soiland-Reyes <[hidden email]> wrote:

> On 30 June 2016 at 00:53, sebb <[hidden email]> wrote:
>> As the subject says; the two factories use a different naming convention.
>>
>> Would it be sensible to standardise on getInstance, given that the
>> class name says what the instance will be?
>
> Hm, but CryptoRandomFactory.getCryptoRandom() returns a CryptoRandom,
> and CryptoCipherFactory.getInstance() returns a CryptoCipher() (not a
> CryptoCipherFactory).
>
> I think it would be misleading to call it getInstance() as that
> implies the singleton pattern, e.g. to return the *Factory, but in
> fact neither of the factories can be instantiated (private
> constructor, only static methods).

getInstance() does not imply Singleton to me, for example:

Calendar.getInstance()
TimeZone.getInstance()

> Both methods take parameters, so it would be returning different
> instances depending on the parameters - so although the ciphers etc.
> might always be the same instances, it's not the singleton pattern.
> (But then not quite the factory pattern either :) )
>
>
> Agreed that the two factories should however be consistent, so I would suggest:
>
>
> public class CryptoRandomFactory {
>   public static CryptoRandom getCryptoRandom( .. ) { .. }
> }
>
> public class CryptoCipherFactory {
>   public static CryptoCipher getCryptoCipher( ..)  { .. }
> }

That actually results in the fewest changes to the source currently.

It would also allow the following usage:

import static org.apache.commons.crypto.random.CryptoRandomFactory.getCryptoRandom;
...
CryptoRandom random = getCryptoRandom(properties);

>
> But I understand your argument.. because for the caller it will be
> quite repetitive:
>
> CryptoCipher cryptoCipher = CryptoCipherFactory.getCryptoCipher( .. ) ;
> CryptoRandom cryptoRandom = CryptoRandomFactory.getCryptoRandom( .. ) ;
>
>
> Perhaps just call the methods  just  .get(...) instead?
>
>
> public class CryptoRandomFactory {
>   public static CryptoRandom get( .. ) { .. }
> }
>
> public class CryptoCipherFactory {
>   public static CryptoCipher get( ..)  { .. }
> }
>

Cannot then use static import of both.

>
> --
> Stian Soiland-Reyes
> Apache Taverna (incubating), Apache Commons
> http://orcid.org/0000-0001-9842-9718
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [CRYPTO] inconsistent naming CryptoRandomFactory.getCryptoRandom : CryptoCipherFactory.getInstance

Benedikt Ritter-4
sebb <[hidden email]> schrieb am Do., 30. Juni 2016 um 13:16 Uhr:

> On 30 June 2016 at 11:01, Stian Soiland-Reyes <[hidden email]> wrote:
> > On 30 June 2016 at 00:53, sebb <[hidden email]> wrote:
> >> As the subject says; the two factories use a different naming
> convention.
> >>
> >> Would it be sensible to standardise on getInstance, given that the
> >> class name says what the instance will be?
> >
> > Hm, but CryptoRandomFactory.getCryptoRandom() returns a CryptoRandom,
> > and CryptoCipherFactory.getInstance() returns a CryptoCipher() (not a
> > CryptoCipherFactory).
> >
> > I think it would be misleading to call it getInstance() as that
> > implies the singleton pattern, e.g. to return the *Factory, but in
> > fact neither of the factories can be instantiated (private
> > constructor, only static methods).
>
> getInstance() does not imply Singleton to me, for example:
>
> Calendar.getInstance()
> TimeZone.getInstance()
>

Those APIs are just bad IMHO. We shouldn't use them as an example :-)


>
> > Both methods take parameters, so it would be returning different
> > instances depending on the parameters - so although the ciphers etc.
> > might always be the same instances, it's not the singleton pattern.
> > (But then not quite the factory pattern either :) )
> >
> >
> > Agreed that the two factories should however be consistent, so I would
> suggest:
> >
> >
> > public class CryptoRandomFactory {
> >   public static CryptoRandom getCryptoRandom( .. ) { .. }
> > }
> >
> > public class CryptoCipherFactory {
> >   public static CryptoCipher getCryptoCipher( ..)  { .. }
> > }
>
> That actually results in the fewest changes to the source currently.
>
> It would also allow the following usage:
>
> import static
> org.apache.commons.crypto.random.CryptoRandomFactory.getCryptoRandom;
> ...
> CryptoRandom random = getCryptoRandom(properties);
>
> >
> > But I understand your argument.. because for the caller it will be
> > quite repetitive:
> >
> > CryptoCipher cryptoCipher = CryptoCipherFactory.getCryptoCipher( .. ) ;
> > CryptoRandom cryptoRandom = CryptoRandomFactory.getCryptoRandom( .. ) ;
> >
> >
> > Perhaps just call the methods  just  .get(...) instead?
> >
> >
> > public class CryptoRandomFactory {
> >   public static CryptoRandom get( .. ) { .. }
> > }
> >
> > public class CryptoCipherFactory {
> >   public static CryptoCipher get( ..)  { .. }
> > }
> >
>
> Cannot then use static import of both.
>
> >
> > --
> > Stian Soiland-Reyes
> > Apache Taverna (incubating), Apache Commons
> > http://orcid.org/0000-0001-9842-9718
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: [hidden email]
> > For additional commands, e-mail: [hidden email]
> >
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>
>
Reply | Threaded
Open this post in threaded view
|

Re: [CRYPTO] inconsistent naming CryptoRandomFactory.getCryptoRandom : CryptoCipherFactory.getInstance

sebb-2-2
On 30 June 2016 at 12:37, Benedikt Ritter <[hidden email]> wrote:

> sebb <[hidden email]> schrieb am Do., 30. Juni 2016 um 13:16 Uhr:
>
>> On 30 June 2016 at 11:01, Stian Soiland-Reyes <[hidden email]> wrote:
>> > On 30 June 2016 at 00:53, sebb <[hidden email]> wrote:
>> >> As the subject says; the two factories use a different naming
>> convention.
>> >>
>> >> Would it be sensible to standardise on getInstance, given that the
>> >> class name says what the instance will be?
>> >
>> > Hm, but CryptoRandomFactory.getCryptoRandom() returns a CryptoRandom,
>> > and CryptoCipherFactory.getInstance() returns a CryptoCipher() (not a
>> > CryptoCipherFactory).
>> >
>> > I think it would be misleading to call it getInstance() as that
>> > implies the singleton pattern, e.g. to return the *Factory, but in
>> > fact neither of the factories can be instantiated (private
>> > constructor, only static methods).
>>
>> getInstance() does not imply Singleton to me, for example:
>>
>> Calendar.getInstance()
>> TimeZone.getInstance()
>>
>
> Those APIs are just bad IMHO. We shouldn't use them as an example :-)
>

Furthermore, an instance is definitely not a singleton.

If it is true that Factory.getInstance implies singleton then that is
bad also ...

One of the reasons for preferring a static factory method over a
constructor is that it allows the code to return either a singleton
instance or a new instance as appropriate.

Particularly in the case of a method with a parameter, it should be
clear that the returned object is not a singleton.

Singletons only make sense if there is only one possible instance, but
any parameter can take multiple values and must affect the object -
otherwise the parameter is useless.

However this is all moot, as I have made the names consistent.

>>
>> > Both methods take parameters, so it would be returning different
>> > instances depending on the parameters - so although the ciphers etc.
>> > might always be the same instances, it's not the singleton pattern.
>> > (But then not quite the factory pattern either :) )
>> >
>> >
>> > Agreed that the two factories should however be consistent, so I would
>> suggest:
>> >
>> >
>> > public class CryptoRandomFactory {
>> >   public static CryptoRandom getCryptoRandom( .. ) { .. }
>> > }
>> >
>> > public class CryptoCipherFactory {
>> >   public static CryptoCipher getCryptoCipher( ..)  { .. }
>> > }
>>
>> That actually results in the fewest changes to the source currently.
>>
>> It would also allow the following usage:
>>
>> import static
>> org.apache.commons.crypto.random.CryptoRandomFactory.getCryptoRandom;
>> ...
>> CryptoRandom random = getCryptoRandom(properties);
>>
>> >
>> > But I understand your argument.. because for the caller it will be
>> > quite repetitive:
>> >
>> > CryptoCipher cryptoCipher = CryptoCipherFactory.getCryptoCipher( .. ) ;
>> > CryptoRandom cryptoRandom = CryptoRandomFactory.getCryptoRandom( .. ) ;
>> >
>> >
>> > Perhaps just call the methods  just  .get(...) instead?
>> >
>> >
>> > public class CryptoRandomFactory {
>> >   public static CryptoRandom get( .. ) { .. }
>> > }
>> >
>> > public class CryptoCipherFactory {
>> >   public static CryptoCipher get( ..)  { .. }
>> > }
>> >
>>
>> Cannot then use static import of both.
>>
>> >
>> > --
>> > Stian Soiland-Reyes
>> > Apache Taverna (incubating), Apache Commons
>> > http://orcid.org/0000-0001-9842-9718
>> >
>> > ---------------------------------------------------------------------
>> > To unsubscribe, e-mail: [hidden email]
>> > For additional commands, e-mail: [hidden email]
>> >
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: [hidden email]
>> For additional commands, e-mail: [hidden email]
>>
>>

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [CRYPTO] inconsistent naming CryptoRandomFactory.getCryptoRandom : CryptoCipherFactory.getInstance

Emmanuel Bourg-3
In reply to this post by Stian Soiland-Reyes
Would it make sense to move the factory method to the base class?

   CryptoRandom random = CryptoRandom.getInstance();

Emmanuel Bourg


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [CRYPTO] inconsistent naming CryptoRandomFactory.getCryptoRandom : CryptoCipherFactory.getInstance

Stian Soiland-Reyes
In reply to this post by sebb-2-2
On 30 June 2016 at 12:15, sebb <[hidden email]> wrote:

> It would also allow the following usage:
> import static org.apache.commons.crypto.random.CryptoRandomFactory.getCryptoRandom;
> CryptoRandom random = getCryptoRandom(properties);

I didn't consider static import. You've won me over,  the above is best! :)


--
Stian Soiland-Reyes
Apache Taverna (incubating), Apache Commons
http://orcid.org/0000-0001-9842-9718

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [CRYPTO] inconsistent naming CryptoRandomFactory.getCryptoRandom : CryptoCipherFactory.getInstance

sebb-2-2
In reply to this post by Emmanuel Bourg-3
On 30 June 2016 at 13:59, Emmanuel Bourg <[hidden email]> wrote:
> Would it make sense to move the factory method to the base class?
>
>    CryptoRandom random = CryptoRandom.getInstance();

It's an interface, not a base class.

> Emmanuel Bourg
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [CRYPTO] inconsistent naming CryptoRandomFactory.getCryptoRandom : CryptoCipherFactory.getInstance

Matt Sicker
If you were using Java 8, you could totally have the factory in the
interface now.

On 30 June 2016 at 10:12, sebb <[hidden email]> wrote:

> On 30 June 2016 at 13:59, Emmanuel Bourg <[hidden email]> wrote:
> > Would it make sense to move the factory method to the base class?
> >
> >    CryptoRandom random = CryptoRandom.getInstance();
>
> It's an interface, not a base class.
>
> > Emmanuel Bourg
> >
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: [hidden email]
> > For additional commands, e-mail: [hidden email]
> >
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>
>


--
Matt Sicker <[hidden email]>