0
\$\begingroup\$

I'm writing a application that reads from a queue of messages from a legacy mainframe system.

Some characteristics of message in the queue:

  • Message from the Q is always fixed length plain text : 64 char length
  • Each index or group of index indicates some meaning full data
  • The first 20 chars represent first name, next 20 surname, character following that represents gender, then next 8 chars denote date in yyyyMMdd format

I need to map these to Java objects. Here is the sample of what I'm doing.

import lombok.Data;
import lombok.NoArgsConstructor;
import lombok.extern.slf4j.Slf4j;

import java.time.LocalDate;
import java.time.format.DateTimeFormatter;
import java.util.Date;

@Slf4j
public class Solution {

    public static void main(String[] args){
        String input = "JOE                BLOGG                M19880101PX2018010199PNM";
        log.info("Input length 64 = ",input.length());
        log.info(empMapper(input).toString());
    }

    public static boolean boolMapper(char value){
        return (value == 'Y') ? true:false;
    }

    public static LocalDate dateMapper(String value){
        final String dateString = String.format("%s-%s-%s",value.substring(0,4),value.substring(4,6),value.substring(6));
        final DateTimeFormatter formatter = DateTimeFormatter.ofPattern("yyyy-MM-dd");
        return LocalDate.parse(dateString, formatter);
    }

    public static Emp empMapper(String input){
        final Emp emp = new Emp();
        emp.setFirstName(input.substring(0,19).trim());
        emp.setSurName(input.substring(19,39).trim());
        emp.setGender(input.charAt(40));
        emp.setDob(dateMapper(input.substring(41,49)));
        emp.setEmpId(input.substring(49,61));
        emp.setJobType(input.charAt(61));
        emp.setShiftNeeded(boolMapper(input.charAt(62)));
        emp.setEmpLevel(input.charAt(63));
        return emp;
    }

    @Data
    @NoArgsConstructor
    public static class Emp{
        private String firstName;
        private String surName;
        private char gender;
        private LocalDate dob;
        private String empId;
        private char jobType;
        private boolean shiftNeeded;
        private char empLevel;
    }
}

Output: Emp(firstName=JOE, surName=BLOGG, gender=M, dob=1988-01-01, empId=PX2018010199, jobType=P, shiftNeeded=false, empLevel=M)

My question is there a better solution for doing this from a performance point of view

\$\endgroup\$
  • \$\begingroup\$ From a pure performance point of view replace String.format(...) with a simple concatenation. Note that on compiling concatenated Strings in the form of String s = string1 + string2; will result in the compiler using a StringBuilder anyway \$\endgroup\$ – Roman Vottner Nov 27 '18 at 16:42
  • 2
    \$\begingroup\$ Do you have any performance issue with your solution? (You should never change a working solution for performance reasons unless you actually have a performance issue and proven by measurement that the code in question actually is the bottleneck and the alternative approach really solves the issue) \$\endgroup\$ – Timothy Truckle Nov 27 '18 at 21:00
  • 1
    \$\begingroup\$ The only think that I see is the dateMapper. Why do you create another dash separated date wile you can parse the original one with ` DateTimeFormatter.ofPattern("yyyyMMdd ")` ? \$\endgroup\$ – gervais.b Nov 28 '18 at 7:38
1
\$\begingroup\$

Why is everything static? It's not necessarily wrong, but it may make integration into a larger project more ugly.

EDIT: Some background to this: A static utility class makes other classes that depend on it difficult to use and test. It basically breaks the principal called Inversion of control.

Instead of having a second class hard-code the reference to such a static "employee factory" like this, it is given an instance of a "employee factory class" (e.g. as a constructor argument). That way the "employee factory" can easily be replaced without touching the second class.

This is especially interesting when you want to test that second class. In that case you don't need to worry about setting up the real "employee factory" and making sure that it works. Instead you can give the second class a so called mock class, that just simulates the workings of the "employee factory" and, for example, just returns hard-coded, predetermined employees.


The conditional operator (? :) in boolMapper is unnecessary:

public static boolean boolMapper(char value){
    return value == 'Y';
}

As @gervais.b says, formatting the date string to include the hyphens is unnecessary. You can parse the date directly with the pattern yyyyMMdd. Also you shouldn't create a new DateTimeFormatter instance in each method call. Instead use an instance stored in a static constant field:

private final static DateTimeFormatter EMPLOYEE_DATE_FORMATTER = DateTimeFormatter.ofPattern("yyyyMMdd");

public static LocalDate dateMapper(String value){
    return LocalDate.parse(value, EMPLOYEE_DATE_FORMATTER);
}

Don't unnecessarily abbreviate variable/class names. Use Employee instead of Emp, use dateOfBirth instead of dob, etc.


Considering you are converting dob and shiftNeeded to "proper" Java types, you should consider also consider converting gender, jobType and empLevel, which you currently store as a chars, into something more Java-esk, eg. enumerations.

\$\endgroup\$
  • 1
    \$\begingroup\$ Why would everything being static potentially cause problems when integrating into a larger project? \$\endgroup\$ – Katie.Sun Nov 29 '18 at 4:11
  • \$\begingroup\$ @Katie.Sun Good question. I've added an explanation. \$\endgroup\$ – RoToRa Nov 29 '18 at 11:50
  • \$\begingroup\$ Thanks for the explanation. Sorry to bug you again, but I'm just learning design patterns and wonder if you could give an example of a "employee factory class" (e.g. as a constructor argument). \$\endgroup\$ – Katie.Sun Nov 29 '18 at 12:43

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.