2 comments regarding commit: r1353771

[Angela Schreiber <an...@adobe.com>]

hi jukka

i have two minor comments regarding the memory value optimization:

a) is it on purpose that the value constructors are public?
    as far as i saw they are only used by the factory... (which
    i would prefer)

b) the following method lists PropertyType.DECIMAL twice. not sure
    if that was a copy-paste error or if you actually wanted another
    type here... maybe BOOLEAN? that would make sense IMO as you
    otherwise create boolean values with the GenericValue although
    you have an extra object for that:

> +    @Override
> +    public CoreValue createValue(String value, final int type) {
> +        if (type == PropertyType.BINARY) {
> +            try {
> +                return new BinaryValue(value.getBytes("UTF-8"));
> +            } catch (UnsupportedEncodingException e) {
> +                throw new IllegalStateException("UTF-8 is not supported", e);
> +            }
> +        } else if (type == PropertyType.DECIMAL) {
> +            return createValue(createValue(value).getDecimal());
> +        } else if (type == PropertyType.DECIMAL) {
> +            return createValue(createValue(value).getDecimal());
> +        } else if (type == PropertyType.DOUBLE) {
> +            return createValue(createValue(value).getDouble());
> +        } else if (type == PropertyType.LONG) {
> +            return createValue(createValue(value).getLong());
> +        } else if (type == PropertyType.STRING) {
> +            return createValue(value);
> +        } else {
> +            return new GenericValue(type, value);
> +        }
> +    }
> +
> +}

regards
angela

Re: 2 comments regarding commit: r1353771

[Angela Schreiber <an...@adobe.com>]

hi jukka

On 6/27/12 11:43 AM, Jukka Zitting wrote:
> On Wed, Jun 27, 2012 at 8:27 AM, Angela Schreiber<an...@adobe.com>  wrote:
>> a) is it on purpose that the value constructors are public?

> The value classes are package-private, which also limits the
> visibility of the constructors. Having the constructors public is just
> a personal stylistic preference.

exactly... that's why i was wondering whether it was your intention
to do that or not... since i found the other issue i wasn't sure
about the public constructor . after all i deliberately stated that
the latter just was my preference, didn't i?

kind regards
angela

Re: 2 comments regarding commit: r1353771

[Jukka Zitting <ju...@gmail.com>]

Hi,

On Wed, Jun 27, 2012 at 8:27 AM, Angela Schreiber <an...@adobe.com> wrote:
> a) is it on purpose that the value constructors are public?

The value classes are package-private, which also limits the
visibility of the constructors. Having the constructors public is just
a personal stylistic preference. I normally prefer to control
package-level visibility on class level (package-private methods are
often symptoms of tight coupling that could/should be refactored
away).

> b) the following method lists PropertyType.DECIMAL twice. not sure
>   if that was a copy-paste error or if you actually wanted another
>   type here... maybe BOOLEAN?

Indeed, good catch! I fixed that in revision 1354383.

BR,

Jukka Zitting