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\$
3
  • \$\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\$ Commented Nov 27, 2018 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\$ Commented Nov 27, 2018 at 21:00
  • 2
    \$\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
    Commented Nov 28, 2018 at 7:38

1 Answer 1

2
\$\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\$
4
  • 1
    \$\begingroup\$ Why would everything being static potentially cause problems when integrating into a larger project? \$\endgroup\$
    – Katie.Sun
    Commented Nov 29, 2018 at 4:11
  • \$\begingroup\$ @Katie.Sun Good question. I've added an explanation. \$\endgroup\$
    – RoToRa
    Commented Nov 29, 2018 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
    Commented Nov 29, 2018 at 12:43
  • \$\begingroup\$ in real code it's not 'static' i made it for my sample code \$\endgroup\$
    – edwin
    Commented Sep 16, 2020 at 8:28

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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