Use Keywords in Param classes

[sazzad16]

I would have set these (STORE &STOREDIST) as Keywords so you won't have to re-serialize them on each call

Originally posted by @gkorland in https://github.com/redis/jedis/pull/2157#discussion_r392667981

None of the Params classes use Keyword yet. May be we can create a separate issue to improve this?

Originally posted by @sazzad16 in https://github.com/redis/jedis/pull/2157#discussion_r396091103

[MrFrydae]

As I am unfamiliar with this codebase, could you please give me an example of how you would want this to be implemented?

Instead of addParam(STORE, key); with STORE being a String, would you want it to be converted to a Keyword?

[sazzad16]

@MrFrydae Yes.

[dengliming]

Currently, All fields of Params were put into a HashMap. e.g.

public class ZIncrByParams extends Params {

  private static final String XX = "xx";
  private static final String NX = "nx";
  private static final String INCR = "incr";
  ....
  public byte[][] getByteParams(byte[] key, byte[]... args) {
    ArrayList<byte[]> byteParams = new ArrayList<>();
    byteParams.add(key);

    if (contains(NX)) {
      byteParams.add(SafeEncoder.encode(NX));
    }
    if (contains(XX)) {
      byteParams.add(SafeEncoder.encode(XX));
    }

    byteParams.add(SafeEncoder.encode(INCR));

    Collections.addAll(byteParams, args);
    return byteParams.toArray(new byte[byteParams.size()][]);
  }
}

Wouldn't it be better if we changed it to the following? no need to call contains(XX) and get(xxx) from a Map and something like INCR will be place in Keyword.

public class ZIncrByParams extends Params {

  private boolean xx = false;
  private boolean nx = false;
  private boolean incr = false;

  public ZIncrByParams nx() {
    this.nx = true;
    return this;
  }
  .....

  public byte[][] getByteParams(byte[] key, byte[]... args) {
    ArrayList<byte[]> byteParams = new ArrayList<>();
    byteParams.add(key);

    if (nx) {
      byteParams.add(SafeEncoder.encode(NX));
    }
    if (xx) {
      byteParams.add(SafeEncoder.encode(XX));
    }
    if (incr) {
      byteParams.add(SafeEncoder.encode(INCR));
    }

    Collections.addAll(byteParams, args);
    return byteParams.toArray(new byte[byteParams.size()][]);
  }

}

@sazzad16 @gkorland Can you comment on that? Thanks.

[sazzad16]

@dengliming For the particular Params class that you mentioned, yes, your approach is better. But keeping in a Map allowed all Params class to have a base class.

However, many of the Params classes have their own format of getByteParams method. So day-by-day significance of base class is deceasing.

[Razi007]

interested to work here.Guidance needed.

[sazzad16]

@Razi007 as a new contributor, I think you can start with Redis 7 commands to implement.

[Razi007]

@sazzad16 thanks for the suggestion.I will start with it.

[sazzad16]

@Razi007 To get some ideas, you can check PRs that are already merged. E.g. about implementing regular commands, you can check #2843 which is pretty straightforward. #2771 is a bit more complex one.

[Razi007]

@sazzad16 I got the context here.Got idea of codebase by raising previous PR. Can i contribute here now?

[sazzad16]

@Razi007 Sure.