MultiFormStep - Code Metrics - Inspection of "MINOR: allowing to load submission from other step..." - silverstripe/silverstripe-multiform - Measure and Improve Code Quality continuously with Scrutinizer

Completed

Pull Request — master (#55)

by Peter

created 2016-01-05 23:52 UTC

MultiFormStep C

↳ Parent: Project

Complexity

Total Complexity

Size/Duplication

Total Lines	446
Duplicated Lines	4.04 %

Coupling/Cohesion

Components	2
Dependencies	7

Metric	Value
wmc	55
lcom	2
cbo	7
dl	18
loc	446
rs	6.8

27 Methods

Rating	Name	Duplication	Size	Complexity
A	getFields()	0	3	1
A	getExtraActions()	0	3	2
A	getValidator()	0	3	1
A	getTitle()	0	3	2
A	Link()	0	4	1
A	loadData()	0	3	3
A	saveData()	0	4	1
A	saveInto()	0	11	2
A	validateStep()	0	3	1
B	getNextStep()	0	17	6
B	getNextStepFromDatabase()	0	13	5
A	getNextSteps()	0	3	1
B	getPreviousStep()	0	14	5
A	getPreviousStepFromDatabase()	0	5	2
A	getPrevText()	0	3	1
A	getNextText()	0	3	1
A	getSubmitText()	0	3	1
A	setForm()	0	3	1
A	getForm()	0	3	1
A	canGoBack()	0	3	1
A	isFinalStep()	0	3	1
A	isCurrentStep()	0	3	2
A	addExtraClass()	9	9	2
A	removeExtraClass()	9	9	2
A	getExtraClasses()	0	3	1
B	getValueFromOtherStep()	0	14	6
A	copyValueFromOtherStep()	0	6	2

How to fix Duplicated Code Complexity

Duplicated Code

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:

If you have the same expression in different places: Extract expression to a method
If you have the same method in different sub-classes: Extract method, and pull up field to the parent class
If you have the same code in unrelated classes: Consider extracting the code to a new class, and injecting that class

Complex Class

Tip: Before tackling complexity, make sure that you eliminate any duplication first. This often can reduce the size of classes significantly.

Complex classes like MultiFormStep 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 MultiFormStep, and based on these observations, apply Extract Interface, too.

<?php

/**
 * MultiFormStep controls the behaviour of a single form step in the MultiForm
 * process. All form steps are required to be subclasses of this class, as it
 * encapsulates the functionality required for the step to be aware of itself
 * in the process by knowing what it's next step is, and if applicable, it's previous
 * step.
 *
 * @package multiform
 */
class MultiFormStep extends DataObject {
namespace YourVendor;

class YourClass { }

	private static $db = array(

		'Data' => 'Text' // stores serialized maps with all session information
	);

	private static $has_one = array(

		'Session' => 'MultiFormSession'
	);

	/**
	 * Centerpiece of the flow control for the form.
	 *
	 * If set to a string, you have a linear form flow
	 * If set to an array, you should use {@link getNextStep()}
	 * to enact flow control and branching to different form
	 * steps, most likely based on previously set session data
	 * (e.g. a checkbox field or a dropdown).
	 *
	 * @var array|string
	 */
	public static $next_steps;

	/**
	 * Each {@link MultiForm} subclass needs at least
	 * one step which is marked as the "final" one
	 * and triggers the {@link MultiForm->finish()}
	 * method that wraps up the whole submission.
	 *
	 * @var boolean
	 */
	public static $is_final_step = false;

	/**
	 * This variable determines whether a user can use
	 * the "back" action from this step.
	 *
	 * @TODO This does not check if the arbitrarily chosen step
	 * using the step indicator is actually a previous step, so
	 * unless you remove the link from the indicator template, or
	 * type in StepID=23 to the address bar you can still go back
	 * using the step indicator.
	 *
	 * @var boolean
	 */
	protected static $can_go_back = true;

	/**
	 * Title of this step.
	 *
	 * Used for the step indicator templates.
	 *
	 * @var string
	 */
	protected $title;

	/**
	 * Form class that this step is directly related to.
	 *
	 * @var MultiForm subclass
	 */
	protected $form;

	/**
	 * List of additional CSS classes for this step
	 *
	 * @var array $extraClasses
	 */
	protected $extraClasses = array();

	/**
	 * Temporary cache to increase the performance for repeated look ups.
	 *
	 * @var array $cache
	 */
	protected $cache = array();

	/**
	 * Form fields to be rendered with this step.
	 * (Form object is created in {@link MultiForm}.
	 *
	 * This function needs to be implemented on your
	 * subclasses of MultiFormStep.
	 *
	 * @return FieldList
	 */
	public function getFields() {
		user_error('Please implement getFields on your MultiFormStep subclass', E_USER_ERROR);
	}

	/**
	 * Additional form actions to be added to this step.
	 * (Form object is created in {@link MultiForm}.
	 *
	 * Note: This is optional, and is to be implemented
	 * on your subclasses of MultiFormStep.
	 *
	 * @return FieldList
	 */
	public function getExtraActions() {
		return (class_exists('FieldList')) ? new FieldList() : new FieldSet();

	}

	/**
	 * Get a validator specific to this form.
	 * The form is automatically validated in {@link Form->httpSubmission()}.
	 *
	 * @return Validator
	 */
	public function getValidator() {
		return false;
	}

	/**
	 * Accessor method for $this->title
	 *
	 * @return string Title of this step
	 */
	public function getTitle() {
		return $this->title ? $this->title : $this->class;
	}

	/**
	 * Gets a direct link to this step (only works
	 * if you're allowed to skip steps, or this step
	 * has already been saved to the database
	 * for the current {@link MultiFormSession}).
	 *
	 * @return string Relative URL to this step
	 */
	public function Link() {
		$form = $this->form;
		return Controller::join_links($form->getDisplayLink(), "?{$form->config()->get_var}={$this->Session()->Hash}");
class ParentClass {
    private $data = array();

    public function __call($method, array $args) {
        if (0 === strpos($method, 'get')) {
            return $this->data[strtolower(substr($method, 3))];
        }

        throw new \LogicException(sprintf('Unsupported method: %s', $method));
    }
}

/**
 * If this class knows which fields exist, you can specify the methods here:
 *
 * @method string getName()
 */
class SomeClass extends ParentClass { }
	}

	/**
	 * Unserialize stored session data and return it.
	 * This is used for loading data previously saved
	 * in session back into the form.
	 *
	 * You need to overload this method onto your own
	 * step if you require custom loading. An example
	 * would be selective loading specific fields, leaving
	 * others that are not required.
	 *
	 * @return array
	 */
	public function loadData() {
		return ($this->Data && is_string($this->Data)) ? unserialize($this->Data) : array();
<?php

/**
 * @property int $x
 * @property int $y
 * @property string $text
 */
class MyLabel
{
    private $properties;

    private $allowedProperties = array('x', 'y', 'text');

    public function __get($name)
    {
        if (isset($properties[$name]) && in_array($name, $this->allowedProperties)) {
            return $properties[$name];
        } else {
            return null;
        }
    }

    public function __set($name, $value)
    {
        if (in_array($name, $this->allowedProperties)) {
            $properties[$name] = $value;
        } else {
            throw new \LogicException("Property $name is not defined.");
        }
    }

}
	}

	/**
	 * Save the data for this step into session, serializing it first.
	 *
	 * To selectively save fields, instead of it all, this
	 * method would need to be overloaded on your step class.
	 *
	 * @param array $data The processed data from save() on {@link MultiForm}
	 */
	public function saveData($data) {
		$this->Data = serialize($data);
<?php

/**
 * @property int $x
 * @property int $y
 * @property string $text
 */
class MyLabel
{
    private $properties;

    private $allowedProperties = array('x', 'y', 'text');

    public function __get($name)
    {
        if (isset($properties[$name]) && in_array($name, $this->allowedProperties)) {
            return $properties[$name];
        } else {
            return null;
        }
    }

    public function __set($name, $value)
    {
        if (in_array($name, $this->allowedProperties)) {
            $properties[$name] = $value;
        } else {
            throw new \LogicException("Property $name is not defined.");
        }
    }

}
		$this->write();
	}

	/**
	 * Save the data on this step into an object,
	 * similiar to {@link Form->saveInto()} - by building
	 * a stub form from {@link getFields()}. This is necessary
	 * to trigger each {@link FormField->saveInto()} method
	 * individually, rather than assuming that all data
	 * serialized through {@link saveData()} can be saved
	 * as a simple value outside of the original FormField context.
	 *
	 * @param DataObject $obj
	 */
	public function saveInto($obj) {
		$form = new Form(
			Controller::curr(),
			'Form',
			$this->getFields(),
/** @return stdClass|null */
function mayReturnNull() { }

function doesNotAcceptNull(stdClass $x) { }

// With potential error.
function withoutCheck() {
    $x = mayReturnNull();
    doesNotAcceptNull($x); // Potential error here.
}

// Safe - Alternative 1
function withCheck1() {
    $x = mayReturnNull();
    if ( ! $x instanceof stdClass) {
        throw new \LogicException('$x must be defined.');
    }
    doesNotAcceptNull($x);
}

// Safe - Alternative 2
function withCheck2() {
    $x = mayReturnNull();
    if ($x instanceof stdClass) {
        doesNotAcceptNull($x);
    }
}
			((class_exists('FieldList')) ? new FieldList() : new FieldSet())
		);
		$form->loadDataFrom($this->loadData());
		$form->saveInto($obj);
		return $obj;
	}

	/**
	 * Custom validation for a step. In most cases, it should be sufficient
	 * to have built-in validation through the {@link Validator} class
	 * on the {@link getValidator()} method.
	 *
	 * Use {@link Form->sessionMessage()} to feed back validation messages
	 * to the user. Please don't redirect from this method,
	 * this is taken care of in {@link next()}.
	 *
	 * @param array $data Request data
	 * @param Form $form
	 * @return boolean Validation success
	 */
	public function validateStep($data, $form) {

		return true;
	}

	/**
	 * Returns the first value of $next_step
	 *
	 * @return string Classname of a {@link MultiFormStep} subclass
	 */
	public function getNextStep() {
		$nextSteps = static::$next_steps;

		// Check if next_steps have been implemented properly if not the final step
		if(!$this->isFinalStep()) {
			if(!isset($nextSteps)) user_error('MultiFormStep->getNextStep(): Please define at least one $next_steps on ' . $this->class, E_USER_ERROR);
		}

		if(is_string($nextSteps)) {
			return $nextSteps;
		} elseif(is_array($nextSteps) && count($nextSteps)) {
			// custom flow control goes here
			return $nextSteps[0];
		} else {
			return false;
class Author {
    private $name;

    public function __construct($name) {
        $this->name = $name;
    }

    public function getName() {
        return $this->name;
    }
}

abstract class Post {
    public function getAuthor() {
        return 'Johannes';
    }
}

class BlogPost extends Post {
    public function getAuthor() {
        return new Author('Johannes');
    }
}

class ForumPost extends Post { /* ... */ }

function my_function(Post $post) {
    echo strtoupper($post->getAuthor());
}
		}
	}

	/**
	 * Returns the next step to the current step in the database.
	 *
	 * This will only return something if you've previously visited
	 * the step ahead of the current step, and then gone back a step.
	 *
	 * @return MultiFormStep|boolean
	 */
	public function getNextStepFromDatabase() {
		if($this->SessionID && is_numeric($this->SessionID)) {
<?php

/**
 * @property int $x
 * @property int $y
 * @property string $text
 */
class MyLabel
{
    private $properties;

    private $allowedProperties = array('x', 'y', 'text');

    public function __get($name)
    {
        if (isset($properties[$name]) && in_array($name, $this->allowedProperties)) {
            return $properties[$name];
        } else {
            return null;
        }
    }

    public function __set($name, $value)
    {
        if (in_array($name, $this->allowedProperties)) {
            $properties[$name] = $value;
        } else {
            throw new \LogicException("Property $name is not defined.");
        }
    }

}
			$nextSteps = static::$next_steps;

			if(is_string($nextSteps)) {
				return DataObject::get_one($nextSteps, "\"SessionID\" = {$this->SessionID}");
<?php

/**
 * @property int $x
 * @property int $y
 * @property string $text
 */
class MyLabel
{
    private $properties;

    private $allowedProperties = array('x', 'y', 'text');

    public function __get($name)
    {
        if (isset($properties[$name]) && in_array($name, $this->allowedProperties)) {
            return $properties[$name];
        } else {
            return null;
        }
    }

    public function __set($name, $value)
    {
        if (in_array($name, $this->allowedProperties)) {
            $properties[$name] = $value;
        } else {
            throw new \LogicException("Property $name is not defined.");
        }
    }

}
			} elseif(is_array($nextSteps)) {
				return DataObject::get_one($nextSteps[0], "\"SessionID\" = {$this->SessionID}");
<?php

/**
 * @property int $x
 * @property int $y
 * @property string $text
 */
class MyLabel
{
    private $properties;

    private $allowedProperties = array('x', 'y', 'text');

    public function __get($name)
    {
        if (isset($properties[$name]) && in_array($name, $this->allowedProperties)) {
            return $properties[$name];
        } else {
            return null;
        }
    }

    public function __set($name, $value)
    {
        if (in_array($name, $this->allowedProperties)) {
            $properties[$name] = $value;
        } else {
            throw new \LogicException("Property $name is not defined.");
        }
    }

}
			} else {
				return false;
			}
		}
	}

	/**
	 * Accessor method for self::$next_steps
	 *
	 * @return string|array
	 */
	public function getNextSteps() {
		return static::$next_steps;
	}

	/**
	 * Returns the previous step, if there is one.
	 *
	 * To determine if there is a previous step, we check the database to see if there's
	 * a previous step for this multi form session ID.
	 *
	 * @return string Classname of a {@link MultiFormStep} subclass
	 */
	public function getPreviousStep() {
		$steps = DataObject::get('MultiFormStep', "\"SessionID\" = {$this->SessionID}", '"LastEdited" DESC');
<?php

/**
 * @property int $x
 * @property int $y
 * @property string $text
 */
class MyLabel
{
    private $properties;

    private $allowedProperties = array('x', 'y', 'text');

    public function __get($name)
    {
        if (isset($properties[$name]) && in_array($name, $this->allowedProperties)) {
            return $properties[$name];
        } else {
            return null;
        }
    }

    public function __set($name, $value)
    {
        if (in_array($name, $this->allowedProperties)) {
            $properties[$name] = $value;
        } else {
            throw new \LogicException("Property $name is not defined.");
        }
    }

}
		if($steps) {
			foreach($steps as $step) {
				$step->setForm($this->form);

				if($step->getNextStep()) {
					if($step->getNextStep() == $this->class) {
						return $step->class;
					}
				}
			}
		}
	}

	/**
	 * Retrieves the previous step class record from the database.
	 *
	 * This will only return a record if you've previously been on the step.
	 *
	 * @return MultiFormStep subclass
	 */
	public function getPreviousStepFromDatabase() {
		if($prevStepClass = $this->getPreviousStep()) {
			return DataObject::get_one($prevStepClass, "\"SessionID\" = {$this->SessionID}");
<?php

/**
 * @property int $x
 * @property int $y
 * @property string $text
 */
class MyLabel
{
    private $properties;

    private $allowedProperties = array('x', 'y', 'text');

    public function __get($name)
    {
        if (isset($properties[$name]) && in_array($name, $this->allowedProperties)) {
            return $properties[$name];
        } else {
            return null;
        }
    }

    public function __set($name, $value)
    {
        if (in_array($name, $this->allowedProperties)) {
            $properties[$name] = $value;
        } else {
            throw new \LogicException("Property $name is not defined.");
        }
    }

}
		}
	}

	/**
	 * Get the text to the use on the button to the previous step.
	 * @return string
	 */
	public function getPrevText() {
		return _t('MultiForm.BACK', 'Back');
	}

	/**
	 * Get the text to use on the button to the next step.
	 * @return string
	 */
	public function getNextText() {
		return _t('MultiForm.NEXT', 'Next');
	}

	/**
	 * Get the text to use on the button to submit the form.
	 * @return string
	 */
	public function getSubmitText() {
		return _t('MultiForm.SUBMIT', 'Submit');
	}

	/**
	 * Sets the form that this step is directly related to.
	 *
	 * @param MultiForm subclass $form
	 */
	public function setForm($form) {
		$this->form = $form;
	}

	/**
	 * @return Form
	 */
	public function getForm() {
		return $this->form;
	}

	// ##################### Utility ####################

	/**
	 * Determines whether the user is able to go back using the "action_back"
	 * Determines whether the user is able to go back using the "action_back"
	 * Determines whether the user is able to go back using the "action_back"
	 * form action, based on the boolean value of $can_go_back.
	 *
	 * @return boolean
	 */
	public function canGoBack() {
		return static::$can_go_back;
	}

	/**
	 * Determines whether this step is the final step in the multi-step process or not,
	 * based on the variable $is_final_step - which must be defined on at least one step.
	 *
	 * @return boolean
	 */
	public function isFinalStep() {
		return static::$is_final_step;
	}

	/**
	 * Determines whether the currently viewed step is the current step set in the session.
	 * This assumes you are checking isCurrentStep() against a data record of a MultiFormStep
	 * subclass, otherwise it doesn't work. An example of this is using a singleton instance - it won't
	 * work because there's no data.
	 *
	 * @return boolean
	 */
	public function isCurrentStep() {
		return ($this->class == $this->Session()->CurrentStep()->class) ? true : false;
class ParentClass {
    private $data = array();

    public function __call($method, array $args) {
        if (0 === strpos($method, 'get')) {
            return $this->data[strtolower(substr($method, 3))];
        }

        throw new \LogicException(sprintf('Unsupported method: %s', $method));
    }
}

/**
 * If this class knows which fields exist, you can specify the methods here:
 *
 * @method string getName()
 */
class SomeClass extends ParentClass { }
	}

	/**
	 * Add a CSS-class to the step. If needed, multiple classes can be added by delimiting a string with spaces.
	 *
	 * @param string $class A string containing a classname or several class names delimited by a space.
	 * @return MultiFormStep
	 */
	public function addExtraClass($class) {

		// split at white space
		$classes = preg_split('/\s+/', $class);
		foreach($classes as $class) {
			// add classes one by one
			$this->extraClasses[$class] = $class;
		}
		return $this;
	}

	/**
	 * Remove a CSS-class from the step. Multiple classes names can be passed through as a space delimited string.
	 *
	 * @param string $class
	 * @return MultiFormStep
	 */
	public function removeExtraClass($class) {

		// split at white space
		$classes = preg_split('/\s+/', $class);
		foreach ($classes as $class) {
			// unset one by one
			unset($this->extraClasses[$class]);
		}
		return $this;
	}

	/**
	 * @return string
	 */
	public function getExtraClasses() {
		return join(' ', array_keys($this->extraClasses));
	}

	/**
	 * Returns the submitted value, if any, of any steps.
	 *
	 * @param string $fromStep (classname)
	 * @param string $key
	 * @return mixed
	 */
	public function getValueFromOtherStep($fromStep, $key) {
		// load the steps in the cache, if this one doesn't exist
		if (!array_key_exists('steps_' . $fromStep, $this->cache)) {
			$steps = MultiFormStep::get()->filter('SessionID', $this->form->session->ID);
<?php

/**
 * @property int $x
 * @property int $y
 * @property string $text
 */
class MyLabel
{
    private $properties;

    private $allowedProperties = array('x', 'y', 'text');

    public function __get($name)
    {
        if (isset($properties[$name]) && in_array($name, $this->allowedProperties)) {
            return $properties[$name];
        } else {
            return null;
        }
    }

    public function __set($name, $value)
    {
        if (in_array($name, $this->allowedProperties)) {
            $properties[$name] = $value;
        } else {
            throw new \LogicException("Property $name is not defined.");
        }
    }

}

			if($steps) foreach($steps as $step) {
				$this->cache['steps_' . $step->ClassName] = $step->loadData();
			}
		}

		// check both as PHP isn't recursive
		return (isset($this->cache['steps_' . $fromStep]) && isset($this->cache['steps_' . $fromStep][$key])) ?
			$this->cache['steps_' . $fromStep][$key] : null;
	}

	/**
	 * allows to get a value from another step copied over
	 *
	 * @param FieldList $fields
	 * @param string    $formStep
	 * @param string    $fieldName
	 * @param string    $fieldNameTarget (optional)
	 */
	public function copyValueFromOtherStep(FieldList $fields, $formStep, $fieldName, $fieldNameTarget = null) {
		// if a target field isn't defined use the same fieldname
		if (!$fieldNameTarget) $fieldNameTarget = $fieldName;
''   == false // true
''   == null  // true
'ab' == false // false
'ab' == null  // false

// It is often better to use strict comparison
'' === false // false
'' === null  // false

		$fields->fieldByName($fieldNameTarget)->setValue($this->getValueFromOtherStep($formStep, $fieldName));
	}
}


silverstripe / silverstripe-multiform