Duplicate code is one of the most pungent code smells. A rule that is often used is to re-structure code once it is duplicated in three or more places.
Common duplication problems, and corresponding solutions are:
Complex classes like UserDefinedForm_Controller often do a lot of different things. To break such a class down, we need to identify a cohesive component within that class. A common approach to find such a component is to look for fields/methods that share the same prefixes, or suffixes. You can also have a look at the cohesion graph to spot any un-connected, or weakly-connected components.
Once you have determined the fields that belong together, you can apply the Extract Class refactoring. If the component makes sense as a sub-class, Extract Subclass is also a candidate, and is often faster.
While breaking up the class, it is a good idea to analyze how other classes use UserDefinedForm_Controller, and based on these observations, apply Extract Interface, too.
| 1 | <?php |
||
| 356 | class UserDefinedForm_Controller extends Page_Controller |
||
| 357 | { |
||
| 358 | |||
| 359 | private static $finished_anchor = '#uff'; |
||
| 360 | |||
| 361 | private static $allowed_actions = array( |
||
| 362 | 'index', |
||
| 363 | 'ping', |
||
| 364 | 'Form', |
||
| 365 | 'finished' |
||
| 366 | ); |
||
| 367 | |||
| 368 | 6 | public function init() |
|
| 369 | { |
||
| 370 | 5 | parent::init(); |
|
| 371 | |||
| 372 | // load the jquery |
||
| 373 | 5 | $lang = i18n::get_lang_from_locale(i18n::get_locale()); |
|
| 374 | 5 | Requirements::css(USERFORMS_DIR . '/css/UserForm.css'); |
|
| 375 | 5 | Requirements::javascript(FRAMEWORK_DIR .'/thirdparty/jquery/jquery.js'); |
|
| 376 | 5 | Requirements::javascript(USERFORMS_DIR . '/thirdparty/jquery-validate/jquery.validate.min.js'); |
|
| 377 | 5 | Requirements::add_i18n_javascript(USERFORMS_DIR . '/javascript/lang'); |
|
| 378 | 5 | Requirements::javascript(USERFORMS_DIR . '/javascript/UserForm.js'); |
|
| 379 | |||
| 380 | 5 | Requirements::javascript( |
|
| 381 | 5 | USERFORMS_DIR . "/thirdparty/jquery-validate/localization/messages_{$lang}.min.js" |
|
| 382 | 5 | ); |
|
| 383 | 5 | Requirements::javascript( |
|
| 384 | 6 | USERFORMS_DIR . "/thirdparty/jquery-validate/localization/methods_{$lang}.min.js" |
|
| 385 | 6 | ); |
|
| 386 | 5 | if ($this->HideFieldLabels) { |
|
| 387 | Requirements::javascript(USERFORMS_DIR . '/thirdparty/Placeholders.js/Placeholders.min.js'); |
||
| 388 | } |
||
| 389 | |||
| 390 | // Bind a confirmation message when navigating away from a partially completed form. |
||
| 391 | 5 | $page = $this->data(); |
|
| 392 | 5 | if ($page::config()->enable_are_you_sure) { |
|
| 393 | 5 | Requirements::javascript(USERFORMS_DIR . '/thirdparty/jquery.are-you-sure/jquery.are-you-sure.js'); |
|
| 394 | 5 | } |
|
| 395 | 5 | } |
|
| 396 | |||
| 397 | /** |
||
| 398 | * Using $UserDefinedForm in the Content area of the page shows |
||
| 399 | * where the form should be rendered into. If it does not exist |
||
| 400 | * then default back to $Form. |
||
| 401 | * |
||
| 402 | * @return array |
||
| 403 | */ |
||
| 404 | 6 | public function index() |
|
| 405 | { |
||
| 406 | 6 | if ($this->Content && $form = $this->Form()) { |
|
| 407 | 6 | $hasLocation = stristr($this->Content, '$UserDefinedForm'); |
|
| 408 | 6 | if ($hasLocation) { |
|
| 409 | 5 | $content = preg_replace('/(<p[^>]*>)?\\$UserDefinedForm(<\\/p>)?/i', $form->forTemplate(), $this->Content); |
|
| 410 | return array( |
||
| 411 | 5 | 'Content' => DBField::create_field('HTMLText', $content), |
|
| 412 | 'Form' => "" |
||
| 413 | 5 | ); |
|
| 414 | } |
||
| 415 | 1 | } |
|
| 416 | |||
| 417 | return array( |
||
| 418 | 1 | 'Content' => DBField::create_field('HTMLText', $this->Content), |
|
| 419 | 1 | 'Form' => $this->Form() |
|
| 420 | 2 | ); |
|
| 421 | } |
||
| 422 | |||
| 423 | /** |
||
| 424 | * Keep the session alive for the user. |
||
| 425 | * |
||
| 426 | * @return int |
||
| 427 | */ |
||
| 428 | public function ping() |
||
| 429 | { |
||
| 430 | return 1; |
||
| 431 | } |
||
| 432 | |||
| 433 | /** |
||
| 434 | * Get the form for the page. Form can be modified by calling {@link updateForm()} |
||
| 435 | * on a UserDefinedForm extension. |
||
| 436 | * |
||
| 437 | * @return Forms |
||
| 438 | */ |
||
| 439 | 9 | public function Form() |
|
| 445 | |||
| 446 | /** |
||
| 447 | * Generate the javascript for the conditional field show / hiding logic. |
||
| 448 | * |
||
| 449 | * @return void |
||
| 450 | */ |
||
| 451 | 9 | public function generateConditionalJavascript() |
|
| 452 | { |
||
| 453 | 9 | $default = ""; |
|
| 454 | 9 | $rules = ""; |
|
| 455 | |||
| 456 | 9 | $watch = array(); |
|
| 457 | 9 | $watchLoad = array(); |
|
| 458 | |||
| 459 | 9 | if ($this->Fields()) { |
|
| 460 | 9 | foreach ($this->Fields() as $field) { |
|
| 461 | 8 | $holderSelector = $field->getSelectorHolder(); |
|
| 462 | |||
| 463 | // Is this Field Show by Default |
||
| 464 | 8 | if (!$field->ShowOnLoad) { |
|
| 465 | $default .= "{$holderSelector}.hide().trigger('userform.field.hide');\n"; |
||
| 466 | } |
||
| 467 | |||
| 468 | // Check for field dependencies / default |
||
| 469 | 8 | foreach ($field->EffectiveDisplayRules() as $rule) { |
|
| 470 | |||
| 471 | // Get the field which is effected |
||
| 472 | $formFieldWatch = EditableFormField::get()->byId($rule->ConditionFieldID); |
||
| 473 | |||
| 474 | // Skip deleted fields |
||
| 475 | if (!$formFieldWatch) { |
||
| 476 | continue; |
||
| 477 | } |
||
| 478 | |||
| 479 | $fieldToWatch = $formFieldWatch->getSelectorField($rule); |
||
| 480 | $fieldToWatchOnLoad = $formFieldWatch->getSelectorField($rule, true); |
||
| 481 | |||
| 482 | // show or hide? |
||
| 483 | $view = ($rule->Display == 'Hide') ? 'hide' : 'show'; |
||
| 484 | $opposite = ($view == "show") ? "hide" : "show"; |
||
| 485 | |||
| 486 | // what action do we need to keep track of. Something nicer here maybe? |
||
| 487 | // @todo encapulsation |
||
| 488 | $action = "change"; |
||
| 489 | |||
| 490 | if ($formFieldWatch instanceof EditableTextField) { |
||
| 491 | $action = "keyup"; |
||
| 492 | } |
||
| 493 | |||
| 494 | // is this field a special option field |
||
| 495 | $checkboxField = false; |
||
| 496 | $radioField = false; |
||
| 497 | |||
| 498 | if (in_array($formFieldWatch->ClassName, array('EditableCheckboxGroupField', 'EditableCheckbox'))) { |
||
| 499 | $action = "click"; |
||
| 500 | $checkboxField = true; |
||
| 501 | } elseif ($formFieldWatch->ClassName == "EditableRadioField") { |
||
| 502 | $radioField = true; |
||
| 503 | } |
||
| 504 | |||
| 505 | // and what should we evaluate |
||
| 506 | switch ($rule->ConditionOption) { |
||
| 507 | 1 | case 'IsNotBlank': |
|
| 508 | $expression = ($checkboxField || $radioField) ? '$(this).is(":checked")' :'$(this).val() != ""'; |
||
| 509 | |||
| 510 | break; |
||
| 511 | case 'IsBlank': |
||
| 512 | $expression = ($checkboxField || $radioField) ? '!($(this).is(":checked"))' : '$(this).val() == ""'; |
||
| 513 | |||
| 514 | break; |
||
| 515 | 1 | View Code Duplication | case 'HasValue': |
| 516 | if ($checkboxField) { |
||
| 517 | $expression = '$(this).prop("checked")'; |
||
| 518 | } elseif ($radioField) { |
||
| 519 | // We cannot simply get the value of the radio group, we need to find the checked option first. |
||
| 520 | $expression = '$(this).parents(".field, .control-group").find("input:checked").val()=="'. $rule->FieldValue .'"'; |
||
| 521 | } else { |
||
| 522 | $expression = '$(this).val() == "'. $rule->FieldValue .'"'; |
||
| 523 | 1 | } |
|
| 524 | |||
| 525 | break; |
||
| 526 | case 'ValueLessThan': |
||
| 527 | $expression = '$(this).val() < parseFloat("'. $rule->FieldValue .'")'; |
||
| 528 | |||
| 529 | break; |
||
| 530 | case 'ValueLessThanEqual': |
||
| 531 | $expression = '$(this).val() <= parseFloat("'. $rule->FieldValue .'")'; |
||
| 532 | |||
| 533 | break; |
||
| 534 | case 'ValueGreaterThan': |
||
| 535 | $expression = '$(this).val() > parseFloat("'. $rule->FieldValue .'")'; |
||
| 536 | |||
| 537 | break; |
||
| 538 | case 'ValueGreaterThanEqual': |
||
| 539 | $expression = '$(this).val() >= parseFloat("'. $rule->FieldValue .'")'; |
||
| 540 | |||
| 541 | break; |
||
| 542 | View Code Duplication | default: // ==HasNotValue |
|
| 543 | if ($checkboxField) { |
||
| 544 | $expression = '!$(this).prop("checked")'; |
||
| 545 | } elseif ($radioField) { |
||
| 546 | // We cannot simply get the value of the radio group, we need to find the checked option first. |
||
| 547 | $expression = '$(this).parents(".field, .control-group").find("input:checked").val()!="'. $rule->FieldValue .'"'; |
||
| 548 | } else { |
||
| 549 | $expression = '$(this).val() != "'. $rule->FieldValue .'"'; |
||
| 550 | } |
||
| 551 | |||
| 552 | break; |
||
| 553 | } |
||
| 554 | |||
| 555 | if (!isset($watch[$fieldToWatch])) { |
||
| 556 | $watch[$fieldToWatch] = array(); |
||
| 557 | } |
||
| 558 | |||
| 559 | $watch[$fieldToWatch][] = array( |
||
| 560 | 1 | 'expression' => $expression, |
|
| 561 | 'holder_selector' => $holderSelector, |
||
| 562 | 'view' => $view, |
||
| 563 | 'opposite' => $opposite, |
||
| 564 | 'action' => $action |
||
| 565 | ); |
||
| 566 | |||
| 567 | $watchLoad[$fieldToWatchOnLoad] = true; |
||
| 568 | 8 | } |
|
| 569 | 9 | } |
|
| 570 | 9 | } |
|
| 571 | |||
| 572 | 9 | if ($watch) { |
|
| 573 | foreach ($watch as $key => $values) { |
||
| 574 | $logic = array(); |
||
| 575 | 1 | $actions = array(); |
|
| 576 | |||
| 577 | foreach ($values as $rule) { |
||
| 578 | // Assign action |
||
| 579 | $actions[$rule['action']] = $rule['action']; |
||
| 580 | |||
| 581 | // Assign behaviour |
||
| 582 | $expression = $rule['expression']; |
||
| 583 | 1 | $holder = $rule['holder_selector']; |
|
| 584 | $view = $rule['view']; // hide or show |
||
| 585 | $opposite = $rule['opposite']; |
||
| 586 | // Generated javascript for triggering visibility |
||
| 587 | $logic[] = <<<"EOS" |
||
| 588 | if({$expression}) { |
||
| 589 | {$holder} |
||
| 590 | .{$view}() |
||
| 591 | 1 | .trigger('userform.field.{$view}'); |
|
| 592 | } else { |
||
| 593 | {$holder} |
||
| 594 | .{$opposite}() |
||
| 595 | .trigger('userform.field.{$opposite}'); |
||
| 596 | } |
||
| 597 | EOS; |
||
| 598 | } |
||
| 599 | |||
| 600 | $logic = implode("\n", $logic); |
||
| 601 | $rules .= $key.".each(function() {\n |
||
| 602 | $(this).data('userformConditions', function() {\n |
||
| 603 | $logic\n |
||
| 604 | }); \n |
||
| 605 | });\n"; |
||
| 606 | foreach ($actions as $action) { |
||
| 607 | $rules .= $key.".$action(function() { |
||
| 608 | $(this).data('userformConditions').call(this);\n |
||
| 609 | });\n"; |
||
| 610 | } |
||
| 611 | } |
||
| 612 | } |
||
| 613 | |||
| 614 | 9 | if ($watchLoad) { |
|
| 615 | foreach ($watchLoad as $key => $value) { |
||
| 616 | $rules .= $key.".each(function() { |
||
| 617 | $(this).data('userformConditions').call(this);\n |
||
| 618 | });\n"; |
||
| 619 | } |
||
| 620 | } |
||
| 621 | |||
| 622 | // Only add customScript if $default or $rules is defined |
||
| 623 | 9 | if ($default || $rules) { |
|
| 624 | Requirements::customScript(<<<JS |
||
| 625 | (function($) { |
||
| 626 | 3 | $(document).ready(function() { |
|
| 627 | $default |
||
| 628 | |||
| 629 | $rules |
||
| 630 | }) |
||
| 631 | })(jQuery); |
||
| 632 | JS |
||
| 633 | , 'UserFormsConditional'); |
||
| 634 | } |
||
| 635 | 9 | } |
|
| 636 | |||
| 637 | /** |
||
| 638 | * Process the form that is submitted through the site |
||
| 639 | * |
||
| 640 | * {@see UserForm::validate()} for validation step prior to processing |
||
| 641 | * |
||
| 642 | * @param array $data |
||
| 643 | * @param Form $form |
||
| 644 | * |
||
| 645 | * @return Redirection |
||
| 646 | */ |
||
| 647 | 3 | public function process($data, $form) |
|
| 834 | |||
| 835 | /** |
||
| 836 | * Allows the use of field values in email body. |
||
| 837 | * |
||
| 838 | * @param ArrayList fields |
||
| 839 | * @return ArrayData |
||
| 840 | */ |
||
| 841 | 1 | private function getMergeFieldsMap($fields = array()) |
|
| 851 | |||
| 852 | /** |
||
| 853 | * This action handles rendering the "finished" message, which is |
||
| 854 | * customizable by editing the ReceivedFormSubmission template. |
||
| 855 | * |
||
| 856 | * @return ViewableData |
||
| 857 | */ |
||
| 858 | 9 | public function finished() |
|
| 896 | } |
||
| 897 |
You can fix this by adding a namespace to your class:
When choosing a vendor namespace, try to pick something that is not too generic to avoid conflicts with other libraries.