[Candidate] Add getAge function

[zaliqarosli]

Brief summary of changes

Calculates the candidate's age today using php's DateTime obj diff() function in:

• years, months, days
• years
• months
• days

If you're not a huge fan of doing it this way, look at commit 2 which uses \Utility::calculateAge function (not as accurate for calculating number of days)

I have added changes to timepoint_list to show an example of how to use these functions and for testing. These can be removed before merging. Travis will fail until these changes to timepoint_list are removed.

Testing instructions (if applicable)

1. Edit line 109 of timepoint_list.class.inc to use all the getAge..() functions.
2. Got to Access Profile, click on a candidate, view the Age in the table at the top of the candidate page. Test this for years, months, and days.

[johnsaigle]

Looks good!

It might be helpful to have an example of where it would be used. If not that, maybe a unit test demonstrating that it returns what we expect it to return.

Not worth blocking though

[ridz1208]

@zaliqarosli I dont have a problem with it but I'm not sure why you are truncating the months and days. You are making it unusable for any autism study, they would only get 0 all the time.

[johnsaigle]

Hmm yeah it may be good to change the name of the function to getAgeInYears().

Maybe an even cleaner approach would be to move calculateAge from Utility to Candidate. Then you could add wrapper functions in the Candidate class that call calculateAge, e.g.

public function getAgeInYears(): int {
    return $this->calculateAge(
         $this->candidateInfo['DoB'], 
         (new \DateTime()->format('Y-m-d'))
    )['year'];
}

And do another one for getAgeInMonths and getAgeInDays. Then it would work for many types of studies.

[zaliqarosli]

@johnsaigle @ridz1208 ya i am down to make separate functions for in years, in months, and in days. I don't agree with moving calculateAge to the candidate class however. It is a function that's used a lot in NDB_BVL_Instrument and I think makes sense to be a generic 'Utility' function rather than candidate specific

[ridz1208]

Well... this is not technically giving you an age IN years. it's giving you the number of full years in the age.

ie a 3 month old baby should not be 0 but rather 0.25 years

[johnsaigle]

@ridz1208 Yeah it's int years not float years. I am speculating but I imagine it's more typical for researchers to want the age of a child in months rather than decimal years anyway. 5 months is more useful than 0.4166666666666666 years
@zaliqarosli Sounds good. You're right it doesn't need to be moved in this PR. Probably can be done another time.

[zaliqarosli]

@ridz1208 i know what you mean, but in that case, the project can use getAgeInMonths() instead of getAgeInYears(). I don't think a Parkinson's project for example would find use in seeing a candidate's age as 85.83 years as opposed to just 85 years

[zaliqarosli]

but now I'm thinking do we also want a function called getAgeInYears&Months????

[thomasbeaudry]

Why not take an input variable that specifies years, months, or days instead of having 3 functions?

[zaliqarosli]

@thomasbeaudry because the calculations are all different and grouping them into one function that takes as input the different cases wouldn't necessarily make the code or functions any cleaner/succinct

[zaliqarosli]

@johnsaigle @ridz1208 okay checkout the calculations using php's DateTime diff() functionality in my last commit. I made commit 2 incase commit 3 causes controversy xD

[johnsaigle]

I'm a big big fan of using DateTime objects for this sort of thing so I'm happy you're doing that. 😄

I have a couple of concerns:

1. Utility::calculateAge() will give different results from the getAge*() functions.

That's not a bad thing necessarily but we'll have to keep this in mind and should document it somewhere clearly.
It's possible that this could cause an issue in analysis. Someone with project experience could better speak to this.
Maybe a point in a LORIS meeting would be good?
I'm gonna add the Needs Documentation and Add to Release Notes labels. We can remove them if this turns out not to be a big deal.

2. I think the names of the function should be consistent.

Either use the word "In" for all of them or remove it from all of them.

3. Could you add unit tests?

4. Since getAge() doesn't use calculateAge anymore, it should probably be changed to just return a DateTime. It doesn't need to conform to the format used by calculateAge(). It might be confusing if it does actually.

This would also let you call the main function getAge() from your other getAge*() functions so they could simplify to e.g.

public function getAgeInMonths(): int
    {
        $this->getAge();
        return (int)$age->format('m') + 12 * $age->format('y');
    }

[zaliqarosli]

@johnsaigle

1. So regarding the issue in analysis, that has probably been brought up before hence the warning in the doc comments of the Utility::calculateAge() function itself. This is a caveat for behavioural data and is not a new issue. however, i'm no longer using this function, so why is this still a concern for this PR?

2. I don't really care to fight for this one but this is something that @ridz1208 and I agreed on in order to provide nuance

3. Yes, if I can learn how to make them

4. On the contrary, I think it is useful to return the year, month and day as an array so that an implementation of that function can choose to use all or only one of the values. For example, it allows a use to either use 'years months days' or just 'year months' if they so choose. I also think the function doc comments can help with any confusion. I'm also not sure about returning a DateTime when calling for an age, as the result wouldn't really make sense without already formatting it.

ps. the diff() function returns a DateInterval type

[johnsaigle]

1. It might be weird to have two different places in the code that give different results for somebody's age.

2. Agree to disagree. Won't block the PR over it.

3. Cool, let me know if you want help

4. it allows a use to either use 'years months days' or just 'year months' if they so choose

the result wouldn't really make sense without already formatting it

Don't these sentences contradict each other? In the first you're saying that you want the users to format on their own and then you're saying you want to format it for them.

DateTime is unformatted which I think is ideal. If you return a DateTime then the calling code function could just do $age->format('Y-m-d') or $age->format('Y-m) to get 'years months days' or 'years months'. Also, what if they want seconds? (Yes, they probably won't for age, I know.) We don't have that option with a custom array.

It would also allow us to move forward with using DateTimes everywhere for internal code instead of flip-flopping between arrays, strings, and DateTimes as we do now.

To me it seems DateTimes give us more flexibility and consistency and allow us to take advantage of types.
With arrays, we have a fixed format, limited precision, inconsistency with other places in the code (except for Utility::calculateAge -- which we're deliberately trying to distinguish this code from because they behave differently), and make it so that a person has to read the function comment to know what's going on.

You're right about the "->days"/DateInterval part. I don't have a problem there.

[johnsaigle]

OK hold on I realize how I was being confusing/was confused.

I think getAge could return a DateInterval instead. DateTime doesn't make sense.

[zaliqarosli]

@johnsaigle i don't see the contradiction. i think you're misunderstanding me. what the functions are formatting is not a DateTime object, its a DateInterval object, as that is what is returned by diff(). Returning a DateInterval object without formatting it doesn't seem useful to me. Users can format the age in whatever way they want. And I think providing the DateInterval data in a way that makes this possible can be useful. For example, having getAge() return a string "%y Years, %m Months, %d days" is not as useful as returning an array where its values %y, %m, %d can then be used.

You're talking about DateTime but that is not the result type that we're dealing with here. How would you yourself deal with the DateInterval returned by diff before you return it in the getAge() function?

[zaliqarosli]

@johnsaigle getAge could just return a DateInterval but then we'd need more functions to then return meaningful data from the candidate class

[johnsaigle]

i think you're misunderstanding me.

I definitely was. Sorry. I think what I said still holds if getAge returns a DateInterval though.

I was wrong everywhere I said DateTime. This should all be DateIntervals.

How would you yourself deal with the DateInterval returned by diff before you return it in the getAge() function?
For example, having getAge() return a string "%y Years, %m Months, %d days" is not as useful as returning an array where its values %y, %m, %d can then be used.

DateInterval does that on its own though. You're creating an array around DateInterval that has fewer options.
I would use it like this:

$ageInterval = $candidate->getAge();
// print time since birth.
echo $ageInterval->format("%y years, %m months, %d days, and %h hours");

getAge could just return a DateInterval but then we'd need more functions to then return meaningful data from the candidate class

I think you'd just need to modify the ones you have here

function getAgeInMonths(): int {
    $ageInterval = $this->getAge();
    return (int) $ageInternval->format("%y") * 12 + (int) $ageInterval->format("%m");
}

[ridz1208]

@zaliqarosli I like how this is looking

[johnsaigle]

Refactor looks good! 😄 Sorry again for the initial confusion.

Let me know if you want assistance with the unit tests.

[johnsaigle]

As far as I can tell, the integration tests in modules/timepoint_list/test/timepoint_listTest.php are failing because that file creates a couple of Candidates and inserts them into the database. However they are not provided with a DoB value which is messing up the tests.

If you add DoBs for the candidates by modifying the arrays in this file, I think that should fix it.

Loris/modules/timepoint_list/test/timepoint_listTest.php

Lines 111 to 149 in 73ba888

	$this->DB->insert(
	"candidate",
	array(
	'CandID' => '900001',
	'PSCID' => 'TST0002',
	'RegistrationCenterID' => 1,
	'RegistrationProjectID' => 1,
	'Active' => 'Y',
	'UserID' => 1,
	'Entity_type' => 'Human',
	)
	);
	// Session for candidate TST0002: should not be listed on the page
	$this->DB->insert(
	'session',
	array(
	'ID' => '999998',
	'CandID' => '900001',
	'Visit_label' => 'Test',
	'CenterID' => 1,
	'ProjectID' => 1,
	)
	);
	// Inactive session for candidate TST0001: should not appear
	// on the page
	$this->DB->insert(
	'session',
	array(
	'ID' => '999997',
	'CandID' => self::$_TST0001_CANDID,
	'Visit_label' => 'Test2',
	'CenterID' => 1,
	'ProjectID' => 1,
	'Active' => 'N',
	)
	);
	$this->safeGet(

[driusan]

@zaliqarosli is this ready? It seems to be passing Travis

[zaliqarosli]

@driusan i believe so!