Mailing-List: contact oak-dev-help@jackrabbit.apache.org; run by ezmlm
Precedence: bulk
Reply-To: oak-dev@jackrabbit.apache.org
Received-SPF: pass (nike.apache.org: domain of anchela@adobe.com designates
 64.18.1.21 as permitted sender)
Message-ID: <4FEAA7C2.8030706@adobe.com>
Date: Wed, 27 Jun 2012 08:27:14 +0200
From: Angela Schreiber <anchela@adobe.com>
User-Agent: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US;
 rv:1.9.2.18) Gecko/20110616 Thunderbird/3.1.11
MIME-Version: 1.0
To: <oak-dev@jackrabbit.apache.org>
Subject: 2 comments regarding commit: r1353771
References: <20120625225026.E307023888CD@eris.apache.org>
In-Reply-To: <20120625225026.E307023888CD@eris.apache.org>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit

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