Add field and value into an object by reflection

Question

original code:

• import, hide some irrelevant company package

import cn.hutool.core.date.LocalDateTimeUtil;
import java.lang.reflect.Field;
import java.time.LocalDateTime;
import java.util.Date;
import java.util.Objects;
import lombok.extern.slf4j.Slf4j;
import org.apache.ibatis.binding.MapperMethod;
import org.apache.ibatis.executor.Executor;
import org.apache.ibatis.mapping.MappedStatement;
import org.apache.ibatis.mapping.SqlCommandType;
import org.apache.ibatis.plugin.Interceptor;
import org.apache.ibatis.plugin.Intercepts;
import org.apache.ibatis.plugin.Invocation;
import org.apache.ibatis.plugin.Signature;
import org.springframework.stereotype.Component;

• code:

private static final int ACTION_CREATE_FLAG = 0;
private static final int ACTION_UPDATE_FLAG = 1;
private static final String FIELD_ZONE_CODE = "zoneCode";
private static final String FIELD_ZONE_NAME = "zoneName";
private static final String FIELD_CREATE_USER_ID = "createUserId";
private static final String FIELD_CREATE_USER_NAME = "createUserName";
private static final String FIELD_CREATE_TIME = "createTime";
private static final String FIELD_UPDATE_USER_ID = "updateUserId";
private static final String FIELD_UPDATE_USER_NAME = "updateUserName";
private static final String FIELD_UPDATE_TIME = "updateTime";
private Object procUserInfo(Object obj, int type) {
 OauthUserInfo authUser = GpJwtUtils.current();
 if (obj == null) {
 return null;
 }
 if (authUser == null) {
 authUser = new OauthUserInfo();
 authUser.setUserId("system");
 authUser.setUserName("systemUser");
 }
 Field[] fields = ConvertUtils.getAllFields(obj);
 for (Field field : fields) {
 try {
 if (!FIELD_ZONE_CODE.equals(field.getName())
 && !FIELD_ZONE_NAME.equals(field.getName())
 && !FIELD_CREATE_USER_ID.equals(field.getName())
 && !FIELD_CREATE_USER_NAME.equals(field.getName())
 && !FIELD_CREATE_TIME.equals(field.getName())
 && !FIELD_UPDATE_USER_ID.equals(field.getName())
 && !FIELD_UPDATE_USER_NAME.equals(field.getName())
 && !FIELD_UPDATE_TIME.equals(field.getName())) {
 continue;
 }
 field.setAccessible(true);
 if ((type == ACTION_CREATE_FLAG && FIELD_CREATE_TIME.equals(field.getName()))) {
 if (LocalDateTime.class == field.getType()) {
 field.set(obj, LocalDateTimeUtil.now());
 } else {
 field.set(obj, new Date());
 }
 } else if (type == ACTION_UPDATE_FLAG && FIELD_UPDATE_TIME.equals(field.getName())) {
 if (LocalDateTime.class == field.getType()) {
 field.set(obj, LocalDateTimeUtil.now());
 } else {
 field.set(obj, new Date());
 }
 }
 if (field.get(obj) != null) {
 continue;
 }
 // zoneCode
 if (FIELD_ZONE_CODE.equals(field.getName())) {
 Object value = field.get(obj);
 String zoneCode = (String) ThreadContextHandler.get(CommonConstant.CONTEXT_ZONE_CODE);
 if (type == ACTION_CREATE_FLAG && value == null && zoneCode != null) {
 field.set(obj, zoneCode);
 }
 }
 if (FIELD_ZONE_NAME.equals(field.getName())) {
 Object value = field.get(obj);
 String zoneName = (String) ThreadContextHandler.get(CommonConstant.CONTEXT_ZONE_NAME);
 if (type == ACTION_CREATE_FLAG && value == null && zoneName != null) {
 field.set(obj, zoneName);
 }
 }
 // createUser
 if (type == ACTION_CREATE_FLAG && FIELD_CREATE_USER_ID.equals(field.getName())) {
 field.set(obj, authUser.getUserId());
 }
 if (type == ACTION_CREATE_FLAG && FIELD_CREATE_USER_NAME.equals(field.getName())) {
 field.set(obj, authUser.getUserName());
 }
 // updateUser
 if (FIELD_UPDATE_USER_ID.equals(field.getName())) {
 field.set(obj, authUser.getUserId());
 }
 if (FIELD_UPDATE_USER_NAME.equals(field.getName())) {
 field.set(obj, authUser.getUserName());
 }
 } catch (Exception e) {
 log.warn("procUserInfo fail:", e);
 }
 }
 return obj;
}

• method call:

MappedStatement ms = (MappedStatement) invocation.getArgs()[0];
int actionFlag = ms.getSqlCommandType() == SqlCommandType.INSERT ? ACTION_CREATE_FLAG : ACTION_UPDATE_FLAG;
procUserInfo(parameterObject, actionFlag);

my try:

private static final int ACTION_CREATE_FLAG = 0;
private static final int ACTION_UPDATE_FLAG = 1;
private static final String FIELD_CREATE_TIME = "createTime";
private static final String FIELD_UPDATE_TIME = "updateTime";
private static final LinkedList<String> ZONE_CODE_NAMES = (LinkedList<String>)
 Arrays.asList("zoneCode","zoneName");
private static final LinkedList<String> FIELD_CREATE_NAMES = (LinkedList<String>)
 Arrays.asList("createUserId", "createUserName", FIELD_CREATE_TIME);
private static final LinkedList<String> FIELD_UPDATE_NAMES = (LinkedList<String>)
 Arrays.asList("updateUserId", "updateUserName", FIELD_UPDATE_TIME);
/**
* add zone info and createUser or updateUser info into an object
*
* @param obj any database related obj
* @param type 0 createUser,1 updateUser
* @return obj with zone info and createUser or updateUser info
*/
private Object procUserInfo(Object obj, int type) throws NoSuchFieldException {
 if (type != ACTION_UPDATE_FLAG && type != ACTION_CREATE_FLAG) {
 return obj;
 }
 if (obj == null) {
 return null;
 }
 for (Map.Entry<String,Object> fieldInfo:getFieldInfos(type,obj,getAuthUser()).entrySet()) {
 try {
 setField(obj,fieldInfo.getKey(),fieldInfo.getValue());
 } catch (Exception e) {
 log.warn("procUserInfo fail", e);
 }
 }
 return obj;
}
private OauthUserInfo getAuthUser(){
 OauthUserInfo authUser = GpJwtUtils.current();
 if (authUser == null) {
 authUser = new OauthUserInfo();
 authUser.setUserId("system");
 authUser.setUserName("systemUser");
 }
 return authUser;
}
private void setField(Object obj, String fieldName, Object value) throws NoSuchFieldException {
 if (value == null) {
 return;
 }
 Field field = obj.getClass().getField(fieldName);
 ReflectionUtils.makeAccessible(field);
 ReflectionUtils.setField(field,obj,value);
}
private Map<String,Object> getFieldInfos(int type,Object obj,OauthUserInfo authUser) throws NoSuchFieldException {
 LinkedList<String> fieldNames = ZONE_CODE_NAMES;
 fieldNames.addAll(type == ACTION_CREATE_FLAG ? FIELD_CREATE_NAMES : FIELD_UPDATE_NAMES);
 List<Object> values = getValues(obj,authUser);
 return IntStream.range(0, fieldNames.size()).boxed().collect(Collectors.toMap(fieldNames::get, values::get));
}
private LinkedList<Object> getValues(Object obj,OauthUserInfo authUser) throws NoSuchFieldException {
 LinkedList<Object> result = new LinkedList<>();
 result.add(ThreadContextHandler.get(CommonConstant.CONTEXT_ZONE_CODE));
 result.add(ThreadContextHandler.get(CommonConstant.CONTEXT_ZONE_NAME));
 result.add(authUser.getUserId());
 result.add(authUser.getUserName());
 result.add(LocalDateTime.class == obj.getClass().getField(FIELD_CREATE_TIME).getType() ?
 LocalDateTimeUtil.now() : new Date());
 result.add(LocalDateTime.class == obj.getClass().getField(FIELD_UPDATE_TIME).getType() ?
 LocalDateTimeUtil.now() : new Date());
 return result;
}

description:

1. I know the method name is not the best name, but I didn't find a better name now.
2. Control a method by int type maybe a code smell, but I didn't find a better solution either.
3. Declare a static variable combine with enum and string maybe a bad practice, such as FIELD_CREATE_NAMES, any better solution?

• morgwai: this really really needs some context so ppl are even able to look at this code: - this seems to be somehow related to OAuth and JWT, but it's nowhere explained how; - there's a lot of constants defined with zero explanation (javadocs) what's their meaning and where they should be used; - the title of the post seems to correspond to a small helper method setField(): what's the purpose of all the other code then?; - you've omitted all imports so, guessing the context is even harder: please bring 'em back ;-)
• leaf_yakitori: @morgwai ok I'll edit it to add more details, but OAuth really has no relation with it in this situation, it just a method get a userid and username.

Answer 1

It's too bad that the review context omits import statements. They would have helped with looking up relevant documentation.

EDIT: Ok, I see those recently added Apache Ibatis imports, thank you.

length

This method is entirely too long. Fortunately, I see that below there's a revision which breaks out helpers, good.

javadoc

This is super vague:

private Object procUserInfo(Object obj, int type) {

No idea what that object is all about, so I will just keep reading. Maybe it is a "user info" object?

The type looks like it wants to be an Enum.

I guess we have an abbreviation for the verb "process", here? But that's a pretty vague verb, so I don't know what promise this method makes to its caller. Spell it out, if I guessed correctly. Maybe use a verb like "alter"? Maybe ditch the type parameter, and turn this into a pair of methods, so an engineer can call

• createUserInfo(), or
• updateUserInfo()

Most importantly, if a three-word identifier cannot communicate to the reader what this function does, then offer a one-sentence /** javadoc */ explanation of what it does. What's the post-condition? How could I tell that the right thing happened, after calling it?

Oh good, I see that such a sentence has been added in the revised version.

defer work

 OauthUserInfo authUser = GpJwtUtils.current();

In the case that obj is null, there's no need to make this call. So just defer it until after that test.

Making someone the systemUser in the case that the .current() call failed seems an odd default. We don't want to make it harder to become systemUser?

skip unknown field

Testing for unknown field name seems tediously long. Maybe throw those known names into a HashSet, and test for membership? Couldn't an Enum accomplish the same thing for us?

Consider turning some ifs into a switch statement.

It's not clear why we need the INSERT / UPDATE type flag, nor why it's OK for caller to specify mismatched type and name and then we just silently ignore it. Maybe we don't need that type parameter at all? The field name seems to already specify what the caller intended.

re-throw

At the end we log any exception that happened and then swallow it, so caller believes we succeeded. This seems Bad. Prefer to log it and re-throw it, perhaps first wrapping it in a RuntimeException if that makes the signatures more convenient.

---

Moving on to the revised version.

javadoc

This is an improvement over nothing, but it's still not great.

/**
* add zone info and createUser or updateUser info into an object
*
* @param obj any database related obj
* @param type 0 createUser,1 updateUser
* @return obj with zone info and createUser or updateUser info
*/

Let's start with that "any database related obj" description, which makes it clear that we chose the wrong identifier. It isn't any old object obj. It's a database object dbObj.

I still wish you'd given me a hint about some representative class to which it might belong. The review context just offers parameterObject, which doesn't bring me any closer to enlightenment. Offering unit tests would have been helpful -- they have instructional value.

When I try to parse the sentence, "add zone info and createUser or updateUser info into [a database] object," I get a little closer to understanding the promise made, better than "process user info," but I'm still left wanting more detail, perhaps from a second sentence. I at least now know the verb "process" implies the side effect of either creating or updating some user info.

helpers

Breaking out four helper methods, starting with getAuthUser, is a big win, thank you.

I appreciate that the contract is "getAuthUser shall never return null", which simplifies logic up in the caller. We validate pre-conditions, then loop through some reads and writes, very straightforward.

After logging an error we should re-throw, rather than swallow the exception.

fatal error

In setField it appears it may be better to throw fatal RuntimeException, rather than silently ignore a null input which caller should never have sent us in the first place.

Writing lots of code involves writing some bugs. When we write bugs we prefer that they be shallow bugs. It should be easy to pin the blame on where the bug started, and fix it there, rather than debugging through a call stack in search of the root cause.