50onRed / mock-jedis

Mock Jedis is a library for mocking out Jedis clients
MIT License
82 stars 47 forks source link

Rewrite binary operations implementation #21

Closed kshchepanovskyi closed 10 years ago

kshchepanovskyi commented 10 years ago

Previously binary data was stored as new String(byteArray) but it is not correct - as stated in String javadoc, it can replace unmappable characters.

As a result, when you save serialized java object it is not possible to deserialize it later.

idyedov commented 10 years ago

does it make sense to convert everything to byte arrays? ie use that logic for String methods as well

kshchepanovskyi commented 10 years ago

does it make sense to convert everything to byte arrays

Generally, it does not matter - because we must call String.getBytes() on read operations.

ie use that logic for String methods as well

It is used. Each pair of string/binary method uses private method that takes DataContainer type as arguments. For example:

@Override
public synchronized Response<String> set(final String key, final String value) {
    return set(DataContainer.from(key), DataContainer.from(value));
}

@Override
public synchronized Response<String> set(final byte[] key, final byte[] value) {
    return set(DataContainer.from(key), DataContainer.from(value));
}

private Response<String> set(final DataContainer key, final DataContainer value) {
    /// ...
idyedov commented 10 years ago

I guess I'm just concerned that we're storing it twice - one as a String and one as a byte array

kshchepanovskyi commented 10 years ago

Thanks for your comments - I will make improvements soon.

I guess I'm just concerned that we're storing it twice - one as a String and one as a byte array

I don't think so. It is mocking tool, memory usage is not a priority. Also, it is useful for debugging - it is almost impossible to identify your keys/values when they are stored as byte arrays only. Anyway, it is easy to optimize.