4
\$\begingroup\$

Is there a cleaner way of doing this? I'm using filter_input to first check for valid input, then using it again to assign the variable. Also, my condition in if/else seems quite long.

$optionsMonth = array('options'=>array('min_range'=>1, 'max_range'=>12));
$optionsYear = array('options'=>array('min_range'=>1900, 'max_range'=>9999));

if ((filter_input(INPUT_GET, 'month', FILTER_VALIDATE_INT, $optionsMonth))&&(filter_input(INPUT_GET, 'year', FILTER_VALIDATE_INT, $optionsYear))){
    $month = filter_input(INPUT_GET, 'month', FILTER_VALIDATE_INT, $optionsMonth );
    $year = filter_input(INPUT_GET, 'year', FILTER_VALIDATE_INT, $optionsYear);
} else {
    $datetime = new Datetime('NOW');
    $month = $datetime->format('m');
    $year = $datetime->format('Y');
}
\$\endgroup\$
1
\$\begingroup\$

You may be over thinking it. It's actually very simple.

The only ambiguous thing is if the year is two digits. In most cases a two digit year can be fixed like this. I believe in your use the month and year will be all be future and nothing from the 1900s.

if (intval($optionsYear) < 100){$optionsYear  = '20' . $optionsYear;}

Then to finish up:

$date = strtotime("$optionsYear-$optionsMonth-13"); // yyyy-mm-dd
if ($date){  // test validity, will be false if invalid
  $month = date('m',$date);  // invalid, use today's month and year.
  $year = date('Y',$date); 
}
else{
  $month = date('m');  // valid numbers entered
  $year = date('Y'); 
}

Because the day of the month is no an issue I used 13. Not that it would but in dates the month and day can cause problems. By using a day of 13 it cannot be confused with a month. If a date were entered as 13/4/2015, it would not be difficult to understand 13 is a day and 4 is a month. Oct, Nov, Dec would still be ambiguous if entered as 10, 11, or 12.

\$\endgroup\$
0
\$\begingroup\$

Here my proposal:

  • I don't need to store in array the options rules.
  • I would build in both case the DateTime Object and then use always the ->format() method
  • I would use the short syntax for array (PHP 5.4+ only) [] inestad of array()

then:

$month = filter_input(INPUT_GET, 'month', FILTER_VALIDATE_INT, ['options'=>['min_range' => 1    , 'max_range' => 12]]);
$year  = filter_input(INPUT_GET, 'year' , FILTER_VALIDATE_INT, ['options'=>['min_range' => 1900 , 'max_range' => 9999]]);

$D = new DateTime();

if($month && $year) 
    $D->setDate($year,$month,1);

$month = $D->format("m");
$year  = $D->format("Y");

echo "$month-$year";

Unfortunately the DateTime object is more permissive on argument input, otherwise it could be sufficient a try ... catch without a filter_input. But it could give unexpected results. However if you want more flexibility on input, then you can think in this direction.

\$\endgroup\$
-2
\$\begingroup\$

Your code is actually fine may be a suggestion to break it into multiple evaluations and functions for:

  1. readability, clarity
  2. structure
  3. testability
  4. code reuse

For example,

if ((filter_input(INPUT_GET, 'month', FILTER_VALIDATE_INT, $optionsMonth)) 
    &&(filter_input(INPUT_GET, 'year', FILTER_VALIDATE_INT, $optionsYear)))

can be written as a function call:

/**
 * This function validates the input against defined valid inputs.
 * It internally calls filter_input().
 * @param {Array} $optionsMonth - valid options
 * @param {int} $month - a user supplied input
 *
 * @return {boolan} - true if valid, false otherwise.
 */ 
function is_valid_input($optionsMonth, $month);

Then the implementation of the function is hidden, and the output of function is clear as it does one thing, so you may use use it like this:

if(!is_valid_input($optionsMonth, $month) ||
    !is_valid_input($optionsYear, $year) {
   // handle defaults
}

Therefore the suggested approach hides the implementation of the function, allows a clear structure flow and also allows it to be testable (ie. unit-test). Further readings on advantages of writing in functions here

\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review! You're presented a alternate solution, but you haven't reviewed the code, nor have you explained why your suggestion may be better. \$\endgroup\$ – 200_success May 2 '15 at 22:11
  • \$\begingroup\$ Thank you, it made sense that it should be rewritten for code review purposes. Apologies, and I have updated the answer. Please let me know if there is any thing else to amend. Cheers. \$\endgroup\$ – daxeh May 3 '15 at 2:34

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.