How to save/get different type of data with less duplicated code

Question

description:

• I'm using thrift to save and get data by filter.
• The data have many types, and the types may increase in the future. So I use switch-case to handle it in save, but it leads to a lot of duplicated code.
• Then when I wrote getByFilter code (basically the reverse of input-output in save), I tried to use reflection and generics to reduce the duplicated code.

origin code in save:

    public void save(List<ThActInfos> info, long timestamp) throws TException {
        for (ThActInfos thActInfos : info) {
            switch (thActInfos.getSetField()) {
                case IMAGE:
                    saveImage(thActInfos.getImage(), timestamp);
                    break;
                case INSTALL:
                    saveInstall(thActInfos.getInstall(), timestamp);
                    break;
                case PID:
                    savePid(thActInfos.getPid(), timestamp);
                    break;
                case RUN_ACTIVE:
                    saveRunActive(thActInfos.getRunActive(), timestamp);
                    break;
                case UNINSTALL:
                    saveUninstall(thActInfos.getUninstall(), timestamp);
                    break;
                case LOGCAT:
                    saveLogcat(thActInfos.getLogcat(), timestamp);
                    break;
                default:

            }
        }
    }

    private void saveImage(ThImageInfo info, long timestamp) {
        if (info.getActionTime() == 0L) {info.setActionTime(timestamp);}
        ImageInfo imageInfo = ImageInfo.fromThrift(info);
        imageInfoMapper.insert(imageInfo);
    }

    private void saveInstall(ThInstallInfo info, long timestamp) {
        if (info.getActionTime() == 0L) {info.setActionTime(timestamp);}
        InstallInfo installInfo = InstallInfo.fromThrift(info);
        installInfoMapper.insert(installInfo);
    }

    private void savePid(ThPidInfo info, long timestamp) {
        if (info.getActionTime() == 0L) {info.setActionTime(timestamp);}
        PidInfo pidInfo = PidInfo.fromThrift(info);
        pidInfoMapper.insert(pidInfo);
    }

    private void saveRunActive(ThRunActivity info, long timestamp) {
        if (info.getActionTime() == 0L) {info.setActionTime(timestamp);}
        RunActivityInfo runActivityInfo = RunActivityInfo.fromThrift(info);
        runActivityInfoMapper.insert(runActivityInfo);
    }

    private void saveUninstall(ThUninstallInfo info, long timestamp) {
        if (info.getActionTime() == 0L) {info.setActionTime(timestamp);}
        UninstallInfo uninstallInfo = UninstallInfo.fromThrift(info);
        uninstallInfoMapper.insert(uninstallInfo);
    }

    private void saveLogcat(ThLogcatInfo info, long timestamp) {
        if (info.getActionTime() == 0L) {info.setActionTime(timestamp);}
        LogcatInfo logcatInfo = LogcatInfo.fromThrift(info);
        logcatInfoMapper.insert(logcatInfo);
    }

what I have tried in getByFilter:

    private Map<String, Object> getTypeMapper() {
        return new HashMap<String, Object>() {{
            put("image",imageInfoMapper);
            put("install",installInfoMapper);
            put("pid", pidInfoMapper);
            put("runActivity", runActivityInfoMapper);
            put("uninstall", uninstallInfoMapper);
            put("logcat", logcatInfoMapper);
        }};
    }
    @Override
    public List<ThActInfos> getByFilter(Map<String, AnyType> filter) {
        Map<String, Object> cond = new HashMap<>();
        for (Map.Entry<String, AnyType> entry : filter.entrySet()) {
            cond.put(entry.getKey(), CoreDataUtils.convertAnyToJava(entry.getValue()));
        }
        String type = cond.getOrDefault("type", "None").toString();
        List<ThActInfos> result = new ArrayList<>();
        Map<String, Object> typeMapper = getTypeMapper();
        try {
            Object obj = typeMapper.get(type);
            String classPath = "com.testing.mapper." + toUpperCase(type) + "InfoMapper";
            Method method = Class.forName(classPath).cast(obj).getClass()
                    .getMethod("selectByCondition", Map.class);
            result = template(Class.forName("com.thrift.entity.Th" + toUpperCase(type) +
                    (toUpperCase(type).equals("RunActivity") ? "" : "Info")),type,method,obj,cond);
        } catch (ClassNotFoundException | NoSuchMethodException e) {
            e.printStackTrace();
        }
        return result;
    }

    private <E,T> List<ThActInfos> template(T thType,String type,Method method,Object obj,Map<String, Object> cond){
        List<ThActInfos> ans = new ArrayList<>();
        try {
            List<E> results = (List<E>) method.invoke(obj, cond);
            ans = new ArrayList<>();
            for (E result : results) {
                ThActInfos temp = new ThActInfos();
                Method toThriftMethod = result.getClass().getMethod("toThrift");
                Method setMethod = temp.getClass()
                    .getMethod("set" + ((type.equals("runActivity")) ? "RunActive" : toUpperCase(type))
                    , (Class<T>) thType);
                setMethod.invoke(temp,toThriftMethod.invoke(result));
                ans.add(temp);
            }
        } catch (IllegalAccessException | InvocationTargetException | NoSuchMethodException e){
            e.printStackTrace();
        }
        return ans;
    }

• It does reduce the duplicated code, but it is obviously much more complicated and hard to read to the co-workers who are not familiar with reflection and generics.

question:

• Any better solution to deal with this situation?

Answer 1

Whenever Java code contains this sort of switch statement and large portions of nearly identical code, then it probably (almost certainly) indicates that you've got your object model wrong.

I don't know anything about Thrift, so can't comment on that aspect, but I'd expect that a good start would be to make much more use of Abstract classes and/or interfaces.

Your various info classes that you retrieve from Thrift should either extend an abstract superclass or implement an interface - let's say ThriftInfo. Then your various type mappers could likewise extend a superclass or implement an interface - say 'InfoMapper' - with an insert(ThriftInfo) method.

Your ThActInfos (perhaps renamed in the singular) class could be an abstract class or implement an interface with a save() method. Subclasses would then be based on the type of data - image, install, etc - they deal with.

The save method could be passed the map of mappers you've already outlined and the specific implementation could pick out the the mapper it needed.

On that basis, given the map exists - as Map<String, InfoMapper> typeMappers - your main loop would look something like:

  for (ThActInfos thActInfos : info) {
    thActInfos.save(typeMappers);
  }