4
\$\begingroup\$

Overview:

Here's the look of my form.

In this example, I have a form that allows dynamically adding fields in each category. Like, I can add multiple attractions like 5 attractions, same goes for Featured artists, so basically I can add an infinite set of data per category in this form.

Screenshot

So after submitting, all those fields (dynamic fields) are sent in array format. It checks if the category have at least one file uploaded. The function here is already correct, it gives the output.

if (count(array_filter($_FILES['attraction']['name'])) > 0) {

    $attraction_status = true;

    $attraction = $_FILES['attraction'];
    $attraction['tmp_name']['image'] = true;
    $attraction_field = Field::incrementFieldComparison($attraction['tmp_name'], $request->post('attraction_name'), $request->post('attraction_description'));

    $attraction_name        = implode('|', $attraction_field->field_1);
    $attraction_description = implode('|', $attraction_field->field_2);
    $attraction_image       = implode('|', $attraction_field->reference);
}

The problem is, I have like 7 categories in this form. So basically, I have this same chunk of code for seven times with different type of category.

if (count(array_filter($_FILES['featured_artist']['name'])) > 0) {

    $featured_artist_status= true;

    $featured_artist= $_FILES['featured_artist'];
    $featured_artist['tmp_name']['image'] = true;
    $featured_artist_field= Field::incrementFieldComparison($attraction['tmp_name'], $request->post('featured_artist_name'), $request->post('featured_artist_description'));

    $featured_artist_name = implode('|', $featured_artist_field->field_1);
    $featured_artist_description = implode('|', $featured_artist_field->field_2);
    $featured_artist_image = implode('|', $featured_artist_field->reference);
}

Is this fine, as long as I'm getting the right output? I'm concerned about the repetition of code. Is there any way to improve this code if this is a bad practice of coding?

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

Answering your questions:

Is this fine, as long as I'm getting the right output?

Not really, by your own admission, the possibility of more fields means you'll have blocks and blocks and blocks of the same code. See Structure below.

I'm concerned about the repetition of code. Is there any way to improve this code if this is a bad practice of coding?

See The code below.


Structure:

Although the code given could be greatly simplified, I think to a degree your methodology is at fault.

Currently, you would have seven variables for each dynamic field, as the number (currently five) increases, the amount of localised variables would be out of control!

Rather than having such a large number of these, consider using a dictionary.

For example:

$content = [];
$content['featured_artist'] = $variable;

Additionally, considering the DRY (don't repeat yourself) principles, the count(array_filter($_FILES[ statements should be turned into a function, and returned to a variable, which can be inserted into the theoretical $content described above.


The code:

Using the following code as a base, with some re-structuring, and maybe some good old fashioned wizardry, the code can be reborn!

if (count(array_filter($_FILES['featured_artist']['name'])) > 0) {

    $featured_artist_status= true;

    $featured_artist= $_FILES['featured_artist'];
    $featured_artist['tmp_name']['image'] = true;
    $featured_artist_field= Field::incrementFieldComparison($attraction['tmp_name'], $request->post('featured_artist_name'), $request->post('featured_artist_description'));

    $featured_artist_name = implode('|', $featured_artist_field->field_1);
    $featured_artist_description = implode('|', $featured_artist_field->field_2);
    $featured_artist_image = implode('|', $featured_artist_field->reference);
}

Such wizardry may look like this:

$completeFieldDict = [];
foreach ($_FILES as $key => $value){
    if (count(array_filter($value["name"])) > 0){
        $completeFieldDict[$key] = buildField($key);
    }
}
function buildField($name){

    $fieldDictionary = [];
    $fieldDictionary['status'] = true;

    $fieldDictionary['content'] = $_FILES[$name];
    $fieldDictionary['content']['tmp_name']['image'] = true;
    $fieldDictionary['field'] = Field::incrementFieldComparison($fieldDictionary['content']['tmp_name'], $request->post($key . '_name'), $request->post($key . '_description'));

    $fieldDictionary['name']        = implode('|', $fieldDictionary['field']->field_1);
    $fieldDictionary['description'] = implode('|', $fieldDictionary['field']->field_2);
    $fieldDictionary['image']       = implode('|', $fieldDictionary['field']->reference);

    return $fieldDictionary;
}

Although, the limitations of this are notable, in form of such a large dictionary.
But, as previously mentioned this could be greatly improved upon refactoring the structure.

Personally, I'd suggest having a database transaction embedded within buildField(), and not saving variables locally.

\$\endgroup\$

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.