4
\$\begingroup\$

How can I refactor it to elegant way? This code is about parse birthday from Rodne Cislo string. I don't know how to do it clever and add more clean code style details

public static void extractBirthdayFromRodneCislo(String rodneCislo) {
    rodneCislo = rodneCislo.replace("/", "=");

    int year;
    int month;
    int day;

    if (rodneCislo.length() < 10) {
        year = Integer.parseInt("19" + rodneCislo.charAt(0) + rodneCislo.charAt(1));
    } else {
        String century = Integer.parseInt(String.valueOf(rodneCislo.charAt(0)) + String.valueOf(rodneCislo.charAt(1))) < 53 ? "20" : "19";
        year = Integer.parseInt(century + rodneCislo.charAt(0) + rodneCislo.charAt(1));
    }

    month = Integer.parseInt(String.valueOf(rodneCislo.charAt(2)) + String.valueOf(rodneCislo.charAt(3)));

    if (month > 12) {
        month -= 50;
    }
    day = Integer.parseInt(String.valueOf(rodneCislo.charAt(4)) + rodneCislo.charAt(5));

    System.out.println(year + " " + month + " " + day);
}

The form is YYXXDD/SSSC, where XX=MM (month of birth) for male (numbers 01-12) and XX=MM+50 for female (numbers 51-62), SSS is a serial number separating persons born on the same date and C is a check digit, but for people born before 1 January 1954 the form is without the check digit - YYXXDD/SSS. This enables the system to work until the year 2054. The whole number must be divisible by 11.

\$\endgroup\$
  • 1
    \$\begingroup\$ Please clarify what format rodneCislo has. \$\endgroup\$ – Anatolii Jan 14 at 20:16
  • \$\begingroup\$ Who is Rodne Cislo ? \$\endgroup\$ – GaVaRaVa Jan 14 at 21:22
  • \$\begingroup\$ I edited it llook my question \$\endgroup\$ – High hopes Jan 14 at 21:34
  • \$\begingroup\$ We sometimes write birth numbers without the slash. You might want to remove/ignore that character instead of replacing it with equal sign. Why you do that btw? I dont see you using the equal sign anywhere later on... \$\endgroup\$ – slepic Jan 15 at 6:42
  • \$\begingroup\$ @GaVaRaVa Rodné číslo = birth number. It uniquely encodes person's gender And day of birth. (Uniquely Meaning each person with samé gender And samé day of birth have a differrent birth number) \$\endgroup\$ – slepic Jan 15 at 7:03
4
\$\begingroup\$

In addition to Doi9t's answer, which gives good advice, I'll add that your function name, extractBirthdayFromRodneCislo, is wrong, as it merely prints the birthday instead extracting it.

The easy fix is to rename the function extractBirthdayFromRodneCislo, which may fit you case, but doesn't allow you to do anything more fancy than that.

The better way to do things would be to return the date, allowing you to do anything with it after calling the method.

I'm completely new to Java, so I might miss something here, but it seems that the java.time.LocalDate datatype is a good fit for the job.

So your function becomes something like this:

public static LocalDate extractBirthdayFromRodneCislo(String rodneCislo) {

    // your logic here

    return LocalDate.of(year, month, day);
}

This approach has the added advantage that, should there be a mistake in the input string leading to an invalid date, it will throw an exception instead of silently accepting an invalid date – if you pass 622754/1234 to your function or Doi9t's, it will not care that 1962 -23 54 is not an acceptable output and print it just like any other date.

This illustrates to another problem: input validation. Your code doens't care if the input is indeed a Rodne Cislo. It will take any string as input. It may fail if Integer.parseInt encounter non-digit characters, but will accept strings that are too long, too short, has a properly placed / character or not (comments pointed out that Czech people don't always include the /, any other character in this place, etc., leading to a big risk of improperly parsing the string.

One possible tool for validating strings are regular expressions (regex). A lot of different patterns can be used to validate the input string, a simple one would be:

^[0-9]{6}\/[0-9]{3}[0-9]?$

If you are unfamiliar with regex, here is a breakdown of this expression:

^[0-9]{6}\/?[0-9]{3}[0-9]?$

^                                 # start with (no leading characater)
 [0-9]{6}                         # 6 digits
         \/?                      # may or may not include a "/" character
            [0-9]{3}              # 3 additional digits
                    [0-9]?        # 1 optional additional digit
                          $       # ends here (no trailing character)

Checking if the input string matches this pattern will call out a lot of invalid input strings, although it doesn't check if the month and day numbers are within valid ranges – it is definitely possible, but will get nasty quite quickly, and the LocalDate.of() method already takes care of it.

Another nice thing about regex is that you can extract specific ranges within the expression easily with capture groups. A little modifications on the previous expression gives (with explanation):

^([0-9]{2})([0-9]{2})([0-9]{2})\/?[0-9]{3}[0-9]?$

^                                                     # starts with (no leading characater)
 ([0-9]{2})                                           # 2 digits, capture them in group 1
           ([0-9]{2})                                 # 2 digits, capture them in group 2
                     ([0-9]{2})                       # 2 digits, capture them in group 3
                               \/?                    # an optional "/" character
                                  [0-9]{3}            # 3 digits
                                          ([0-9]?)    # 1 optional digit, capture it in group 4
                                                  $   # ends here (no trailing character)

You can then used the captured strings to parse the numbers (groups 1 to 3), or identify if the birth year is before or after 1954 (group 4).

My final take on the problem is this code:

import java.time.LocalDate;
import java.util.regex.Pattern;
import java.util.regex.Matcher;

class Main {
    public static void main(String[] args) {
        System.out.println(extractBirthdayFromRodneCislo("535223/1234"))
        // > 2053-02-23
        System.out.println(extractBirthdayFromRodneCislo("530223/123"))
        // > 1953-02-23
        System.out.println(extractBirthdayFromRodneCislo("7503121234"))
        // > 1975-03-12
        System.out.println(extractBirthdayFromRodneCislo("753312/1234"))
        // > Throws an exception (invalid month)
        System.out.println(extractBirthdayFromRodneCislo("753312/12345"))
        // > Throws an exception (doesn't match pattern)
    }

    public static LocalDate extractBirthdayFromRodneCislo(String rodneCislo){
        Pattern p = Pattern.compile("^([0-9]{2})([0-9]{2})([0-9]{2})\\/?[0-9]{3}[0-9]?$");

        Matcher m = p.matcher(rodneCislo);

        if (!m.matches()){
            // Input doesn't match the pattern
            // throw a relevant exception
            // although I can't seem to do that propely with Java
        }

        int year = Integer.parseInt(m.group(1));
        if (year < 54 && !m.group(4).isEmpty()){
          year += 2000;
        }else{
          year += 1900;
        }

        int month = Integer.parseInt(m.group(2));
        if (month > 12){month -= 50;}

        int day = Integer.parseInt(m.group(3));

        return LocalDate.of(year, month, day); // will throw an exception if month or day ends up invalid
    }
}
|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ There's no need to escape the / in a regular expression in Java. It would only be necessary in JavaScript or PHP. \$\endgroup\$ – Roland Illig Jan 15 at 17:17
  • 1
    \$\begingroup\$ What about throw new IllegalArgumentException(rodneCislo)? \$\endgroup\$ – Roland Illig Jan 15 at 17:19
3
\$\begingroup\$

Others have improved the parsing, but the code surrounding it is lacking. You asked for help for parsing the date but if the code should be made more "elegant", then instead of defining a single purpose static method for accessing the birth date, you should define a class to represent the Rodne Cislo number. Provide a constructor that performs validation and getters for accessing different fields in the number:

public class RodneCislo {
    private final LocalDate birthate;
    private final int serialNumber;

    public RodneCislo(final String rodneCislo) throws ValidationException {
        // Check format for correctness with regex.
        // Extract date and serial number.
        // Verify that checksum is correct.
    }

    public LocalDate getBirthDate() {
        ...
    }

    public int getSerialNumber() {
        ...
    }

    public Optional<Character> getChecksum() {
        ...
    }
}

Now you have one reusable component that contains all the responsibilities involved in reading Rodne Cislo numbers. Define unit tests in a separate RodneCisloTest class.

The point of representing the Rodne Cislo number as a dedicated class instead of a string is to provide code level guarantee to whoever processes the numbers that it is actually a valid number and not just a random piece of text (and thus input validation needs to be performed only once).

|improve this answer|||||
\$\endgroup\$
  • 1
    \$\begingroup\$ Many try to use a typed constructor RodneCislo(year, month, day) with a static factory method valueOf(String):RodneCislo throwing ValidationException or (subclass of) IllegalArgumentException. The "valueOf" or "fromString" is a kind of stanard used by many frameworks to automagically convert a string to a class. \$\endgroup\$ – gervais.b Jan 15 at 14:30
  • \$\begingroup\$ Great addition! Had I been even a little bit familiar with Java, I would have worked up to something very close to this. \$\endgroup\$ – gazoh Jan 15 at 15:15
  • \$\begingroup\$ @gervais.b Not sure this is the best approach here, as there is no way to infer the SSS digits from the birthdate, the month value also encodes gender, the optional presence of a checksum disambiguates the value of year... Having a constructor take a string as argument seems to me as a better alternative for working with this kind of object. \$\endgroup\$ – gazoh Jan 15 at 15:25
  • \$\begingroup\$ Factory methods are good if you want to create a type that may be a subclass of the containing class or return an optional etc. In this case it doesn't bring any advantages, so constructors are ok. \$\endgroup\$ – TorbenPutkonen Jan 15 at 20:13
  • \$\begingroup\$ @TorbenPutkonen, except that creating rodné číslo from a string can be done in multiple ways (ie the dash separátor May be required, optional or unexpected). Having distinct named static factory for each of those ways May not be bad idea. Or maybe if there Is just one that Takes a string And some discriminator of the separátor presence strategy... \$\endgroup\$ – slepic Jan 18 at 8:38
1
\$\begingroup\$

I have some suggestions for your code.

1) I suggest that you extract the different parts of the date in methods; it will make the code more readable and will be easier to refactor.

public static void extractBirthdayFromRodneCislo(String rodneCislo) {
  rodneCislo = rodneCislo.replace("/", "=");

  int year = getYear(rodneCislo);
  int month = getMonth(rodneCislo);
  int day = getDay(rodneCislo);

  System.out.println(year + " " + month + " " + day);
}

private static int getDay(String rodneCislo) {
   return Integer.parseInt(String.valueOf(rodneCislo.charAt(4)) + rodneCislo.charAt(5));
}

private static int getMonth(String rodneCislo) {
   int month = Integer.parseInt(String.valueOf(rodneCislo.charAt(2)) + String.valueOf(rodneCislo.charAt(3)));

   if (month > 12) {
      month -= 50;
   }

   return month;
}

private static int getYear(String rodneCislo) {
   int year;

   if (rodneCislo.length() < 10) {
      year = Integer.parseInt("19" + rodneCislo.charAt(0) + rodneCislo.charAt(1));
   } else {
      String century = Integer.parseInt(String.valueOf(rodneCislo.charAt(0)) + String.valueOf(rodneCislo.charAt(1))) < 53 ? "20" : "19";
      year = Integer.parseInt(century + rodneCislo.charAt(0) + rodneCislo.charAt(1));
   }

   return year;
}

2) getYear method, you can use multiple return to remove the variable.

private static int getYear(String rodneCislo) {
   if (rodneCislo.length() < 10) {
      return Integer.parseInt("19" + rodneCislo.charAt(0) + rodneCislo.charAt(1));
   } else {
      String century = Integer.parseInt(String.valueOf(rodneCislo.charAt(0)) + String.valueOf(rodneCislo.charAt(1))) < 53 ? "20" : "19";
      return Integer.parseInt(century + rodneCislo.charAt(0) + rodneCislo.charAt(1));
   }
}

3) Instead of using java.lang.String#charAt, I suggest that you use java.lang.String#substring

private static int getDay(String rodneCislo) {
   return Integer.parseInt(rodneCislo.substring(4, 6));
}

private static int getMonth(String rodneCislo) {
   int month = Integer.parseInt(rodneCislo.substring(2, 4));

   if(month > 12) {
      month -= 50;
   }

   return month;
}

private static int getYear(String rodneCislo) {
   String currentRange = rodneCislo.substring(0, 2);

   if (rodneCislo.length() < 10) {
      return Integer.parseInt("19" + currentRange);
   } else {
      String century = Integer.parseInt(currentRange) < 53 ? "20" : "19";
      return Integer.parseInt(century + currentRange);
   }
}

Refactored code

    public static void main(String[] args) {
        extractBirthdayFromRodneCislo("8001015009087");
    }

    public static void extractBirthdayFromRodneCislo(String rodneCislo) {
        rodneCislo = rodneCislo.replace("/", "=");

        int year = getYear(rodneCislo);
        int month = getMonth(rodneCislo);
        int day = getDay(rodneCislo);

        System.out.println(year + " " + month + " " + day);
    }

    private static int getDay(String rodneCislo) {
        return Integer.parseInt(rodneCislo.substring(4, 6));
    }

    private static int getMonth(String rodneCislo) {
        int month = Integer.parseInt(rodneCislo.substring(2, 4));

        if (month > 12) {
            month -= 50;
        }

        return month;
    }

    private static int getYear(String rodneCislo) {
        String currentRange = rodneCislo.substring(0, 2);

        if (rodneCislo.length() < 10) {
            return Integer.parseInt("19" + currentRange);
        } else {
            String century = Integer.parseInt(currentRange) < 53 ? "20" : "19";
            return Integer.parseInt(century + currentRange);
        }
    }
```
|improve this answer|||||
\$\endgroup\$
  • 1
    \$\begingroup\$ For getYear I suggest to define int yy = Integer.parseInt(rodneCislo.substring(0, 2));. After that, you don't need Integer.parseInt anymore since you can work on the numbers directly. \$\endgroup\$ – Roland Illig Jan 15 at 1:16
  • 1
    \$\begingroup\$ It might be better to define a factory method parseBirthNumber() that would return an object with methods like getCentury, getYear, getMonth, getDay, getGender, getSerial, getCheckDigit instead of having a separate parser for every component. Such a factory could make sure the check Digit Is ok And the number Is divisible by 11. And decoupling IT from systém.out. but that would ofc extend the capabilities beyond those of OPs code... \$\endgroup\$ – slepic Jan 15 at 6:09
  • \$\begingroup\$ Btw 8001015009087 Is not a valid czech birth number. Not even close :) \$\endgroup\$ – slepic Jan 15 at 6:48
0
\$\begingroup\$

All suggestions posted before my answer are valid, I'm writing some personal thoughts about your code:

public static void extractBirthdayFromRodneCislo(String rodneCislo) {
   //omitted
   System.out.println(year + " " + month + " " + day);
}

Instead of printing values inside this method you can return the String object as a result and after print it like the code below:

public static void extractBirthdayFromRodneCislo(String rodneCislo) {
    //omitted
    return String.format("%d %d %d", yy, mm, dd);
}

I used the String.format that permits to create string composed by values in a more readable way instead of composing the final String using the + operator.

The purpose of your code is basically extract some characters from your string and use them as int digits from your use of Integer.parseInt method like your code below:

if (rodneCislo.length() < 10) {
        year = Integer.parseInt("19" + rodneCislo.charAt(0) + rodneCislo.charAt(1));
    } else {
        String century = Integer.parseInt(String.valueOf(rodneCislo.charAt(0)) + String.valueOf(rodneCislo.charAt(1))) < 53 ? "20" : "19";
        year = Integer.parseInt(century + rodneCislo.charAt(0) + rodneCislo.charAt(1));
    }
}

You can refactor your code and rewrite your method like below:

public static String extractBirthdayFromRodneCislo1(String rodneCislo) {

    int sum = Integer.parseInt(rodneCislo.substring(0, 2));
    int yy = (rodneCislo.length() < 10 || sum > 53) ? 1900 + sum : 2000 + sum;

    int mm = Integer.parseInt(rodneCislo.substring(2, 4));
    if (mm > 12) { mm -= 50; }

    int dd = Integer.parseInt(rodneCislo.substring(4, 6));

    return String.format("%d %d %d", yy, mm, dd);
}

As already suggested your code should be integrated with a validation method that ensures that characters used in the method extractBirthdayFromRodneCislo are digits otherwise the method will fail.

|improve this answer|||||
\$\endgroup\$

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.