Refactor : mykadDOB

2025-08-07

Today, i encounterd a TS function. It looked like this.

// Before
export function mykadDOB(mykad: string): string {
  if (!mykadValidator(mykad)) {
    return 'MYKAD INVALID';
  }
  const year = parseInt(mykad.substring(0, 2));
  const month =
    parseInt(mykad.substring(2, 4)) && parseInt(mykad.substring(2, 4)) < 10
      ? '0' + parseInt(mykad.substring(2, 4))
      : parseInt(mykad.substring(2, 4));
  //if month is more than 12, it is invalid
  if (parseInt(mykad.substring(2, 4)) > 12) {
    return 'MYKAD INVALID';
  }
  const day =
    parseInt(mykad.substring(4, 6)) && parseInt(mykad.substring(4, 6)) < 10
      ? '0' + parseInt(mykad.substring(4, 6))
      : parseInt(mykad.substring(4, 6));
  //if day is more than 31, it is invalid
  if (parseInt(mykad.substring(4, 6)) > 31) {
    return 'MYKAD INVALID';
  }
  //if month is 2 and day is more than 29, it is invalid
  if (
    parseInt(mykad.substring(2, 4)) === 2 &&
    parseInt(mykad.substring(4, 6)) > 29
  ) {
    return 'MYKAD INVALID';
  }

  //if month is january, march, may, july, august, october, december and day is more than 31, it is invalid
  if (
    (parseInt(mykad.substring(2, 4)) === 1 ||
      parseInt(mykad.substring(2, 4)) === 3 ||
      parseInt(mykad.substring(2, 4)) === 5 ||
      parseInt(mykad.substring(2, 4)) === 7 ||
      parseInt(mykad.substring(2, 4)) === 8 ||
      parseInt(mykad.substring(2, 4)) === 10 ||
      parseInt(mykad.substring(2, 4)) === 12) &&
    parseInt(mykad.substring(4, 6)) > 31
  ) {
    return 'MYKAD INVALID';
  }

  //if month is april, june, september, november and day is more than 30, it is invalid
  if (
    (parseInt(mykad.substring(2, 4)) === 4 ||
      parseInt(mykad.substring(2, 4)) === 6 ||
      parseInt(mykad.substring(2, 4)) === 9 ||
      parseInt(mykad.substring(2, 4)) === 11) &&
    parseInt(mykad.substring(4, 6)) > 30
  ) {
    return 'MYKAD INVALID';
  }

  const currentYear = new Date().getFullYear() % 100;

  if (year > currentYear) {
    return `19${year}-${month}-${day}`;
  } else {
    return `20${year}-${month}-${day}`;
  }
}

After some tinkering, it became this, and i love it:

// After
export function mykadDOB(mykad: string): string {
  if (!mykadValidator(mykad)) {
    return 'MYKAD INVALID';
  }

  function getYear(mykad: string) {
    return parseInt(mykad.substring(0, 2));
  }

  function getMonth(mykad: string): string {
    return mykad.substring(2, 4).padStart(2, '0');
  }

  function getDay(mykad: string): string {
    return mykad.substring(4, 6).padStart(2, "0");
  }

  function isValidDayAndMonth(month: string, day: string): boolean {
    const MaxDayOfMonth = {
      '01': 31,
      '02': 29,
      '03': 31,
      '04': 30,
      '05': 31,
      '06': 30,
      '07': 31,
      '08': 31,
      '09': 30,
      '10': 31,
      '11': 30,
      '12': 31
    }

    const isValidDay = (1 <= parseInt(day) && parseInt(day) <= 31)
    const isValidMonth = (1 <= parseInt(month) && parseInt(month) <= 12)
    const isDayCorrectInMonth = day <= MaxDayOfMonth[month];
    //  Actually, lack of LeapYear check
    //  e.g. 2025-02-29  INVALID
    return [isValidDay, isValidMonth, isDayCorrectInMonth].every(condition => condition === true);
  }

  const year = getYear(mykad)   // e.g. 84
  const month = getMonth(mykad) // e.g '08'
  const day = getDay(mykad)     // e.g. 30'

  // invalid
  if (!isValidDayAndMonth(month, day)) return "MYKAD INVALID";

  // valid day & month
  const currentYear = new Date().getFullYear() % 100;
  const adjustedYear = year > currentYear ? year + 1900 : year + 2000;  // assume everyone < 100 years old

  return `${adjustedYear}-${month}-${day}`;
}

p/s : source code available at @razmans/mykad, https://github.com/razmans/mykad

Because i enjoyed these 2 books:

• 99 Bottles of OOP 2nd Edition - by Sandi Metz

• Refactoring - Improving the Design of Existing Code by Martin Fowler

As a practitioner of writing maintenable clean code. , I think i would be fun to jott down my thought process along this refactoring. And there you go.

Part 1: Code Smell

My first glance ?

This function supposed to return DateOfBirth, with input string of mykad.

P/S: mykad represents the national-id-number of Malaysian, e.g 901030-11-1234. It would tell this person was born in 1990, Oct, 30th.

This gave me a STRONG code smell — something is not right here.

• repetition of code

• return statement among if-else, which are kinda hard to follow, at least for me

if-else logic for day/month check seems lengthy and bulky

So, how’s the quality of the original’s?

“Any fool can write code that a computer can understand. Good programmers write code that humans can understand.” ― Martin Fowler

Can a first-year-student understand this quickly and know where to fix a bug ?

Although, there are some helpful comment to ease the reading, i am not confident enough to say i can 100% follow the code.

If I can't do it, a first-year-cs-student definitely can't.

So, i can’t say it satisfy me.

Part 2: Where to begin ?

What i learnt from Martin Fowler’s book is that, for a long and complicated function, try to Group and Split the lines, into a dedicated function.

In the original’s, the variables of Day, Month and Year seems to be very important to extract it out from input string and used for further processing.

Why not group the day/month/year extraction logic?

export function mykadDOB(mykad: string): string {
  if (!mykadValidator(mykad)) {
    return 'MYKAD INVALID';
  }

  function getYear(mykad) {
    return parseInt(mykad.substring(0, 2));
  }

  function getMonth(mykad) {
    const month =
      parseInt(mykad.substring(2, 4)) && parseInt(mykad.substring(2, 4)) < 10
        ? '0' + parseInt(mykad.substring(2, 4))
        : parseInt(mykad.substring(2, 4));
    //if month is more than 12, it is invalid
    if (parseInt(mykad.substring(2, 4)) > 12) {
      return 'MYKAD INVALID';
    }
  }

  function getDay(mykad) {
    const day =
      parseInt(mykad.substring(4, 6)) && parseInt(mykad.substring(4, 6)) < 10
        ? '0' + parseInt(mykad.substring(4, 6))
        : parseInt(mykad.substring(4, 6));
    //if day is more than 31, it is invalid
    if (parseInt(mykad.substring(4, 6)) > 31) {
      return 'MYKAD INVALID';
    }
  }

  //if month is 2 and day is more than 29, it is invalid
  if (
    parseInt(mykad.substring(2, 4)) === 2 &&
    parseInt(mykad.substring(4, 6)) > 29
  ) {
    return 'MYKAD INVALID';
  }

  //if month is january, march, may, july, august, october, december and day is more than 31, it is invalid
  if (
    (parseInt(mykad.substring(2, 4)) === 1 ||
      parseInt(mykad.substring(2, 4)) === 3 ||
      parseInt(mykad.substring(2, 4)) === 5 ||
      parseInt(mykad.substring(2, 4)) === 7 ||
      parseInt(mykad.substring(2, 4)) === 8 ||
      parseInt(mykad.substring(2, 4)) === 10 ||
      parseInt(mykad.substring(2, 4)) === 12) &&
    parseInt(mykad.substring(4, 6)) > 31
  ) {
    return 'MYKAD INVALID';
  }

  //if month is april, june, september, november and day is more than 30, it is invalid
  if (
    (parseInt(mykad.substring(2, 4)) === 4 ||
      parseInt(mykad.substring(2, 4)) === 6 ||
      parseInt(mykad.substring(2, 4)) === 9 ||
      parseInt(mykad.substring(2, 4)) === 11) &&
    parseInt(mykad.substring(4, 6)) > 30
  ) {
    return 'MYKAD INVALID';
  }

  const year = getYear(mykad);
  const month = getMonth(mykad);
  const day = getDay(mykad);

  const currentYear = new Date().getFullYear() % 100;

  if (year > currentYear) {
    return `19${year}-${month}-${day}`;
  } else {
    return `20${year}-${month}-${day}`;
  }
}

Okay, now we added 3 function:

• getYear()

• getDay()

• getMonth()

So that we can tackle these problems, only 1 function at each time.

getYear() function looks good to me, gonna pass on that.

Lets look at getMonth()

// before
function getMonth(mykad) {
  const month =
    parseInt(mykad.substring(2, 4)) && parseInt(mykad.substring(2, 4)) < 10
      ? '0' + parseInt(mykad.substring(2, 4))
      : parseInt(mykad.substring(2, 4));
  //if month is more than 12, it is invalid
  if (parseInt(mykad.substring(2, 4)) > 12) {
    return 'MYKAD INVALID';
  }
}

Based on my sense, looks like it is returning “01”, “02”, “03”, … “12” to represent month.

If its month of 13, then invalid.

With some better JS sugar syntax, it can be rewritten as :

// after
function getMonth(mykad) {
  const month = mykad.substring(2, 4).padStart(2, "0");
  if (parseInt(mykad.substring(2, 4)) > 12) {
    return 'MYKAD INVALID';
  }
}

The ternary operator is omitted in this case, i loved it.

Then i can re-use the month variable as so, and then to return it:

// after
function getMonth(mykad) {
  const month = mykad.substring(2, 4).padStart(2, "0");
  if (parseInt(month) > 12) {
    return 'MYKAD INVALID';
  }
  return month;
}

As for now, this function returns String, either “01” ~ “12” or “MYKAD INVALID”. It is still not perfect, which we would handle later. As for now, we move on to getDay function.

// before
function getDay(mykad) {
  const day =
    parseInt(mykad.substring(4, 6)) && parseInt(mykad.substring(4, 6)) < 10
      ? '0' + parseInt(mykad.substring(4, 6))
      : parseInt(mykad.substring(4, 6));
  //if day is more than 31, it is invalid
  if (parseInt(mykad.substring(4, 6)) > 31) {
    return 'MYKAD INVALID';
  }
}

Same case like getMonth, the function try to extract the day digits and check its not more than 31.

// after
function getDay(mykad) {
  const day = mykad.substring(4, 6).padStart(2, "0");
  if (parseInt(day) > 31) {
    return 'MYKAD INVALID';
  }
  return day;
}

Similarly, it returns either “01” ~ “31” or “MYKAD INVALID”

Oh no, we forgot the initial logic is to immediately return “MYKAD INVALID” to its caller. Then we added it back:

if (month === 'MYKAD INVALID' || day === 'MYKAD INVALID') return 'MYKAD INVALID';

At this moment, we have:

export function mykadDOB(mykad: string): string {
  if (!mykadValidator(mykad)) {
    return 'MYKAD INVALID';
  }

  function getYear(mykad) {
    return parseInt(mykad.substring(0, 2));
  }

  function getMonth(mykad) {
    const month = mykad.substring(2, 4).padStart(2, "0");
    if (parseInt(month) > 12) {
      return 'MYKAD INVALID';
    }
    return month;
  }

  function getDay(mykad) {
    const day = mykad.substring(4, 6).padStart(2, "0");
    if (parseInt(day) > 31) {
      return 'MYKAD INVALID';
    }
    return day;
  }

  //if month is 2 and day is more than 29, it is invalid
  if (
    parseInt(mykad.substring(2, 4)) === 2 &&
    parseInt(mykad.substring(4, 6)) > 29
  ) {
    return 'MYKAD INVALID';
  }

  //if month is january, march, may, july, august, october, december and day is more than 31, it is invalid
  if (
    (parseInt(mykad.substring(2, 4)) === 1 ||
      parseInt(mykad.substring(2, 4)) === 3 ||
      parseInt(mykad.substring(2, 4)) === 5 ||
      parseInt(mykad.substring(2, 4)) === 7 ||
      parseInt(mykad.substring(2, 4)) === 8 ||
      parseInt(mykad.substring(2, 4)) === 10 ||
      parseInt(mykad.substring(2, 4)) === 12) &&
    parseInt(mykad.substring(4, 6)) > 31
  ) {
    return 'MYKAD INVALID';
  }

  //if month is april, june, september, november and day is more than 30, it is invalid
  if (
    (parseInt(mykad.substring(2, 4)) === 4 ||
      parseInt(mykad.substring(2, 4)) === 6 ||
      parseInt(mykad.substring(2, 4)) === 9 ||
      parseInt(mykad.substring(2, 4)) === 11) &&
    parseInt(mykad.substring(4, 6)) > 30
  ) {
    return 'MYKAD INVALID';
  }

  const year = getYear(mykad);
  const month = getMonth(mykad);
  const day = getDay(mykad);

  if (month === 'MYKAD INVALID' || day === 'MYKAD INVALID') return 'MYKAD INVALID';

  const currentYear = new Date().getFullYear() % 100;

  if (year > currentYear) {
    return `19${year}-${month}-${day}`;
  } else {
    return `20${year}-${month}-${day}`;
  }
}

It was “a-bit” nicer than the original's.

Am i satisfied? Definitely not.

Thats it for today, thanks for reading.

Part 3: How many times we need “MYKAD INVALID” ?

Previously, we left off at :

export function mykadDOB(mykad: string): string {
  if (!mykadValidator(mykad)) {
    return 'MYKAD INVALID';
  }

  function getYear(mykad) {
    return parseInt(mykad.substring(0, 2));
  }

  function getMonth(mykad) {
    const month = mykad.substring(2, 4).padStart(2, "0");
    if (parseInt(month) > 12) {
      return 'MYKAD INVALID';
    }
    return month;
  }

  function getDay(mykad) {
    const day = mykad.substring(4, 6).padStart(2, "0");
    if (parseInt(day) > 31) {
      return 'MYKAD INVALID';
    }
    return day;
  }

  //if month is 2 and day is more than 29, it is invalid
  if (
    parseInt(mykad.substring(2, 4)) === 2 &&
    parseInt(mykad.substring(4, 6)) > 29
  ) {
    return 'MYKAD INVALID';
  }

  //if month is january, march, may, july, august, october, december and day is more than 31, it is invalid
  if (
    (parseInt(mykad.substring(2, 4)) === 1 ||
      parseInt(mykad.substring(2, 4)) === 3 ||
      parseInt(mykad.substring(2, 4)) === 5 ||
      parseInt(mykad.substring(2, 4)) === 7 ||
      parseInt(mykad.substring(2, 4)) === 8 ||
      parseInt(mykad.substring(2, 4)) === 10 ||
      parseInt(mykad.substring(2, 4)) === 12) &&
    parseInt(mykad.substring(4, 6)) > 31
  ) {
    return 'MYKAD INVALID';
  }

  //if month is april, june, september, november and day is more than 30, it is invalid
  if (
    (parseInt(mykad.substring(2, 4)) === 4 ||
      parseInt(mykad.substring(2, 4)) === 6 ||
      parseInt(mykad.substring(2, 4)) === 9 ||
      parseInt(mykad.substring(2, 4)) === 11) &&
    parseInt(mykad.substring(4, 6)) > 30
  ) {
    return 'MYKAD INVALID';
  }

  const year = getYear(mykad);
  const month = getMonth(mykad);
  const day = getDay(mykad);

  if (month === 'MYKAD INVALID' || day === 'MYKAD INVALID') return 'MYKAD INVALID';

  const currentYear = new Date().getFullYear() % 100;

  if (year > currentYear) {
    return `19${year}-${month}-${day}`;
  } else {
    return `20${year}-${month}-${day}`;
  }
}

Where do we refactor next?

One would argue that we already have the variables of year/month/day, then we could replace those repetitive mykad.substring().

That’s not wrong at all.

But, the next code smell to me is

• tight coupling

What items are coupled here?

• string extraction for day/month/year

• return statement for invalid case

This strong coupling is actually against with S of SOLID principle.

• S: Single Responsibility Principle

Unlike getYear function, our getMonth and getDay have “extra” weight carried to verify the data.

Why not we just let the getMonth, getDay function only to do “single reponsibility”, aka extract the data only.

So, to demonstrate it, i will temporarily comment them out.

function getYear(mykad) {
  return parseInt(mykad.substring(0, 2));
}

function getMonth(mykad) {
  const month = mykad.substring(2, 4).padStart(2, "0");
  // if (parseInt(month) > 12) {
  //   return 'MYKAD INVALID';
  // }
  return month;
}

function getDay(mykad) {
  const day = mykad.substring(4, 6).padStart(2, "0");
  // if (parseInt(day) > 31) {
  //   return 'MYKAD INVALID';
  // }
  return day;
}

If there is a unit-test, this would definitely fail.

And normally, before refactoring, there should be a test-file written. Luckily, or maybe unluckily, there is none. Nonetheless, we would revert to test file in next part.

Then, apply technique used by Sandi — squint eye and look for similarity

Following part, looks alike to me, 3 of them :

• has if else

• return “MYKAD INVALID”

//if month is 2 and day is more than 29, it is invalid
if (
  parseInt(mykad.substring(2, 4)) === 2 &&
  parseInt(mykad.substring(4, 6)) > 29
) {
  return 'MYKAD INVALID';
}

//if month is january, march, may, july, august, october, december and day is more than 31, it is invalid
if (
  (parseInt(mykad.substring(2, 4)) === 1 ||
    parseInt(mykad.substring(2, 4)) === 3 ||
    parseInt(mykad.substring(2, 4)) === 5 ||
    parseInt(mykad.substring(2, 4)) === 7 ||
    parseInt(mykad.substring(2, 4)) === 8 ||
    parseInt(mykad.substring(2, 4)) === 10 ||
    parseInt(mykad.substring(2, 4)) === 12) &&
  parseInt(mykad.substring(4, 6)) > 31
) {
  return 'MYKAD INVALID';
}

//if month is april, june, september, november and day is more than 30, it is invalid
if (
  (parseInt(mykad.substring(2, 4)) === 4 ||
    parseInt(mykad.substring(2, 4)) === 6 ||
    parseInt(mykad.substring(2, 4)) === 9 ||
    parseInt(mykad.substring(2, 4)) === 11) &&
  parseInt(mykad.substring(4, 6)) > 30
) {
  return 'MYKAD INVALID';
}

What are they? Take a guess?

Yes, they are the month/day validation logics. Assume one can read the comment.

At this point, one would argue, why bother these when we already have a guard clause at the top of function.

if (!mykadValidator(mykad)) {
  return 'MYKAD INVALID';
}

Unfortunately, it’s not a full validator, just some simple text pattern matching.

// FYI
export function mykadValidator(mykad: string): boolean {
  const regex = /^[0-9]{6}-[0-9]{2}-[0-9]{4}$/;
  return regex.test(mykad);
}

It didnt have the logic as implemented in the original's. Therefore, we still need them.

Back to topic, instead of letting the return statements scatter all around inside function, why not dedicate a function for it.

Lets regroup them and name it isValidDayAndMonth()

function isValidDayAndMonth(){
  // return true if all valid
  // return false if one of our criteria fail
}

Then, do you notice the logic can be summarized as below:

• Jan/Mar/May/Jul/Aug/Oct/Dec - 31 days at max

• Feb - 29 days at max

• Apr/Jun/Sep/Nov - 30 days at max

Option 1

At this point, one could create a month Instance, which is created from a “month-factory” which eventually return a Jan/Feb/Mar instance, which are polymorism of month, e.g:

class Month{
  static factory(month){
    switch(month){
      case '01':
        return new Jan();
      case '02':
        return new Feb();
      // ...
      case '12':
        return new Dec();
    }
  }	

  maxDays(){
    return 31;
  }
}

class Jan extends Month{}

class Feb extends Month{
  maxDays(){
    return 29;
  }
}

class Mar extends Month{}

class Apr extends Month{
  maxDays(){
    return 30;
  }
}

// ...

class Dec extends Month{}

And use it like:

function isValidDayAndMonth(month, day){
  const monthInstance = Month.factory(month);
  if(day > monthInstance.maxDays()) return false
  return true
}

Technically, it is doable, and perhaps is the ultimate form of using polymorphism for refactoring

• reason : No more if-else logic, it is handled by factory function where the branches are taken care of.

Maybe it’s too complicated ? therefore i suggest the alternative — option 2.

Option 2

In this case, there is actually a less-OOP way to refactor it.

Since we only need to get the maximum allowed days from each month. We could make use of hash map data structure.

Why ?

• just some personal preference, i found it it easy to modify, more intuitive, and less OOP thus less code

function isValidDayAndMonth(month, day) {

  const MaxDayOfMonth = {
    '01': 31,
    '02': 29,
    '03': 31,
    '04': 30,
    '05': 31,
    '06': 30,
    '07': 31,
    '08': 31,
    '09': 30,
    '10': 31,
    '11': 30,
    '12': 31
  }
  const isDayCorrectInMonth = day <= MaxDayOfMonth[month];
  return isDayCorrectInMonth;
}

Im pretending we all love less code, thus picked option 2 and move on.

Still remember the validation logics we commented out before inside getDay and getMonth functions? Now it's good time we placed them in.

function isValidDayAndMonth(month, day) {
  const MaxDayOfMonth = {
    '01': 31,
    '02': 29,
    '03': 31,
    '04': 30,
    '05': 31,
    '06': 30,
    '07': 31,
    '08': 31,
    '09': 30,
    '10': 31,
    '11': 30,
    '12': 31
  }
  const isDayCorrectInMonth = day <= MaxDayOfMonth[month];

  if (parseInt(month) > 12) {
    return 'MYKAD INVALID';
  }
  if (parseInt(day) > 31) {
    return 'MYKAD INVALID';
  }

  return isDayCorrectInMonth;
}

But, this is not great, why let it return string instead of Boolean. Aren’t we had “agreed” on its convention naming — isValidDayAndMonth.

Thus:

function isValidDayAndMonth(month, day) {
  const MaxDayOfMonth = {
    '01': 31,
    '02': 29,
    '03': 31,
    '04': 30,
    '05': 31,
    '06': 30,
    '07': 31,
    '08': 31,
    '09': 30,
    '10': 31,
    '11': 30,
    '12': 31
  }
  const isDayCorrectInMonth = day <= MaxDayOfMonth[month];
  if (parseInt(month) > 12) return false;
  if (parseInt(day) > 31) return false;
  return isDayCorrectInMonth;
}

Here, i slightly tighten the checking and give them a meaningful variable.

const isValidDay = (1 <= parseInt(day) && parseInt(day) <= 31)
const isValidMonth = (1 <= parseInt(month) && parseInt(month) <= 12)
const isDayCorrectInMonth = day <= MaxDayOfMonth[month];   

Next, since this is a “ALL” conditions, im using JS sugar syntax to return the final result.

Another reason i prefer Array.every() here, is to hint code-maintainer that it is a ALL criteria to meet.

return [isValidDay, isValidMonth, isDayCorrectInMonth].every(condition => condition === true);


// alternatively
//  return isValidDay && isValidMonth && isDayCorrectInMonth
//  return [isValidDay, isValidMonth, isDayCorrectInMonth].every(Boolean);

Then, our isValidDayAndMonth function is finally done.

We then could elegantly return “Invalid MYKAD”，and remove those bulky if-else.

if (!isValidDayAndMonth(month, day)) return "MYKAD INVALID";
// if (month === 'MYKAD INVALID' || day === 'MYKAD INVALID') return 'MYKAD INVALID';

Lastly, i’m not a fans of if-else return, thus i make it ternary operation

const currentYear = new Date().getFullYear() % 100;

if (year > currentYear) {
  return `19${year}-${month}-${day}`;
} else {
  return `20${year}-${month}-${day}`;
}

would then become:

// valid day & month
const currentYear = new Date().getFullYear() % 100;
const adjustedYear = year > currentYear ? year + 1900 : year + 2000;  // assume everyone < 100 years old

return `${adjustedYear}-${month}-${day}`;

Finally, we made it this far:

export function mykadDOB(mykad: string): string {
  if (!mykadValidator(mykad)) {
    return 'MYKAD INVALID';
  }

  function getYear(mykad) {
    return parseInt(mykad.substring(0, 2));
  }

  function getMonth(mykad) {
    const month = mykad.substring(2, 4).padStart(2, "0");
    return month;
  }

  function getDay(mykad) {
    const day = mykad.substring(4, 6).padStart(2, "0");
    return day;
  }

  function isValidDayAndMonth(day, month) {
    const MaxDayOfMonth = {
      '01': 31,
      '02': 29,
      '03': 31,
      '04': 30,
      '05': 31,
      '06': 30,
      '07': 31,
      '08': 31,
      '09': 30,
      '10': 31,
      '11': 30,
      '12': 31
    }
    const isValidDay = (1 <= parseInt(day) && parseInt(day) <= 31)
    const isValidMonth = (1 <= parseInt(month) && parseInt(month) <= 12)
    const isDayCorrectInMonth = day <= MaxDayOfMonth[month];
    return [isValidDay, isValidMonth, isDayCorrectInMonth].every(condition => condition === true);
  }

  const year = getYear(mykad);
  const month = getMonth(mykad);
  const day = getDay(mykad);

  if (!isValidDayAndMonth(day, month)) return "MYKAD INVALID";

  // valid day & month
  const currentYear = new Date().getFullYear() % 100;
  const adjustedYear = year > currentYear ? year + 1900 : year + 2000;  // assume everyone < 100 years old

  return `${adjustedYear}-${month}-${day}`;
}

To answer the beginning question:

• Can a first-year-student understand this quickly and know where to fix a bug ?

I’m confident to answer that now.

Part 4: Test?

According to Sandi & Martin, the sequence of refactoring are:

• modify a bit

• test

• if a test fails, check again

• if all tests have passed, repeats

Sadly, its no the case here.

When i started out refactoring mykadDOB function, there is no test file in the repo.

So, the moment i started first line of unit test, i have no clue what is the outcome of this function, due to the initial crazy return statement scattered all around.

Thus, a unit-test is very important — as a reminder to myself and reader of this post:

• it tells the reader the “expectation” from a function

No matter how, at this point, we have decoupled some complex logic and modularize it into separate function, where naming of function should’ve telling users usage of each function.

At least, when i re-read it, it’s smooth, and less panicking.

Lastly, bit by bit, we can add each test case, and create a file to:

• check month within 1-12

• check day within 29/30/31 days according to month

• check year's return “fit” to case with assumption of <100 years old

// index.test.ts
// Node.js v18+
// usage : npx tsx --test index.test.ts
import test from 'node:test';
import assert from 'node:assert/strict';

import { mykadDOB } from "./index.ts";

test('mykadDOB return correct DOB', () => {
    assert.equal(mykadDOB("901030-55-1011"), '1990-10-30');
    assert.equal(mykadDOB("011030-55-1011"), '2001-10-30');
});

test('mykadDOB reject INVALID month and day', () => {
    assert.equal(mykadDOB("900015-55-1011"), 'MYKAD INVALID');
    assert.equal(mykadDOB("901315-55-1011"), 'MYKAD INVALID');

    assert.equal(mykadDOB("900132-55-1011"), 'MYKAD INVALID');
    assert.equal(mykadDOB("900230-55-1011"), 'MYKAD INVALID');
    assert.equal(mykadDOB("900332-55-1011"), 'MYKAD INVALID');
    assert.equal(mykadDOB("900431-55-1011"), 'MYKAD INVALID');
    assert.equal(mykadDOB("900532-55-1011"), 'MYKAD INVALID');
    assert.equal(mykadDOB("900631-55-1011"), 'MYKAD INVALID');
    assert.equal(mykadDOB("900732-55-1011"), 'MYKAD INVALID');
    assert.equal(mykadDOB("900832-55-1011"), 'MYKAD INVALID');
    assert.equal(mykadDOB("900931-55-1011"), 'MYKAD INVALID');
    assert.equal(mykadDOB("901032-55-1011"), 'MYKAD INVALID');
    assert.equal(mykadDOB("901131-55-1011"), 'MYKAD INVALID');
    assert.equal(mykadDOB("901232-55-1011"), 'MYKAD INVALID');
});

Finally. we are done.

It was fun along the refactoring journey.

Feel free to use this test case, and go thru the refactoring process again, no harm :)

Part 5 : Some-thoughts

In case you notice, the month/day validation ignores the LeapYear case.

I didn’t add any for those, since refactoring is not about creating more “feature” nor solving “new-bug”.

We could leave it behind, for another PR.

One more thing:

Since SOLID-principle is mentioned.

We could apply

• O: Open/Closed Principle

in our isValidDayAndMonth function like this:

// Open/Closed Principle
function isValidDayAndMonth(day, month, rules) {
  return rules.every(ruleFn => ruleFn(day, month));
}

// array of checking-fn
const rules = [
  (day, month) => 1 <= day && day <= 30,
  (day, month) => 1 <= month && month <= 12,
  // ...
  // leap year checking fn, and so on
]

Here, we basically “closed” the function for modification, and allow the “rules” to be passed in — “opened”. Coder can implements different rule including the leap-year check.

These are just summary of my thought process.

Again, thanks for reading.

Wish you another bugless day. :)

p/s: Pull Request of this refactoring is now merged

• https://github.com/razmans/mykad/pull/4#issuecomment-3157205359

Also published at linkedin article:

• https://www.linkedin.com/pulse/refactor-mykaddob-yeng-chien-wong-bojac
• https://www.linkedin.com/pulse/refactor-mykaddob-part-3-4-yeng-chien-wong-t7buc