3
\$\begingroup\$

I had written with very few PHP skills a conjugator script. It was not good, so I decided to start it from scratch with a help from a friend. I'm using the code formatter from eclipse.

Now I need your reviews and improvement tips.

I know all my long arrays with the all verbs and many more long arrays aren't a good way,but I have no clue how to put them all in a database and how then to use them.

Now a specific question:

word_stem.php In this file, the script gets the correct word_stem to building in conjugate.php function in the line:

Compiling a conjugated verb

$conjugated_verb = word_stem($infinitiveVerb, $person, $tense, $mood) . ending($person, $tense, $mood, $endingwith, $exceptionmodel, $infinitiveVerb);

They are many exception rules for many verb groups. That's why I had to writte somethings long and complicated if conditions to find the needed changed mood-tense-person combinations, which should use the correct word_stem.

Excerpt 1 from word_stem()

if (in_array($exceptionmodel->getValue(), [
    ExceptionModel::Eler_Ele,
    ExceptionModel::Eter_Ete
]) && (($mood->getValue() === Mood::Indicatif && $tense->getValue() === Tense::Present && in_array($person->getValue(), [
    Person::FirstPersonSingular,
    Person::SecondPersonSingular,
    Person::ThirdPersonSingular,
    Person::ThirdPersonPlural
]) || $tense->getValue() === Tense::Futur) || ($mood->getValue() === Mood::Subjonctif && $tense->getValue() === Tense::Present && in_array($person->getValue(), [
    Person::FirstPersonSingular,
    Person::SecondPersonSingular,
    Person::ThirdPersonSingular,
    Person::ThirdPersonPlural
])) || ($mood->getValue() === Mood::Conditionnel && $tense->getValue() === Tense::Present) || ($mood->getValue() === Mood::Imperatif && $tense->getValue() === Tense::Present && $person->getValue() === Person::SecondPersonSingular))) {
    $word_stem = substr_replace($word_stem, 'è', - 2, 1);
}

Excerpt 2

if ($exceptionmodel->getValue() === ExceptionModel::VALOIR && (($mood->getValue() === Mood::Indicatif && $tense->getValue() === Tense::Present && in_array($person->getValue(), [
    Person::FirstPersonSingular,
    Person::SecondPersonSingular,
    Person::ThirdPersonSingular
]) || $tense->getValue() === Tense::Futur) || ($mood->getValue() === Mood::Conditionnel && $tense->getValue() === Tense::Present) || (($mood->getValue() === Mood::Imperatif && $tense->getValue() === Tense::Present && $person->getValue() === Person::SecondPersonSingular)))) {
    $word_stem = word_stem_length($infinitiveVerb, 4) . 'u';
}
if ($exceptionmodel->getValue() === ExceptionModel::VALOIR && ($mood->getValue() === Mood::Subjonctif && $tense->getValue() === Tense::Present && in_array($person->getValue(), [
    Person::FirstPersonSingular,
    Person::SecondPersonSingular,
    Person::ThirdPersonSingular,
    Person::ThirdPersonPlural
]))) {
    $word_stem = word_stem_length($infinitiveVerb, 4) . 'ill';
}

Sometimes I need in one if condition all persons from a tense in a mood and sometimes only some parts froma tense in a mood.

So looks for example a exception array for one Person in one time and mood.

$endings[Mood::Indicatif][Tense::Present][Person::FirstPersonSingular]

Feel free, to have a look at the other files and give me please tips. You can also become a constributor in this project.

I will use the script for my german speaking learning website.

\$\endgroup\$
0

1 Answer 1

3
\$\begingroup\$

Wrap conditions to be visible

The first order of business when having long if conditions is to wrap lines at each and every && or ||, and in general to wrap the lines so that you are able to see the conditions without scrolling. If you have to scroll, you'll loose context straight away. This will render your code into the following:

if ($exceptionmodel->getValue() === ExceptionModel::VALOIR
    && (  ($mood->getValue() === Mood::Indicatif 
           && $tense->getValue() === Tense::Present
           && in_array($person->getValue(), 
                     [ Person::FirstPersonSingular,
                       Person::SecondPersonSingular,
                       Person::ThirdPersonSingular
                     ]))
        || $tense->getValue() === Tense::Futur
        || ($mood->getValue() === Mood::Conditionnel
            && $tense->getValue() === Tense::Present) 
        || ($mood->getValue() === Mood::Imperatif
            && $tense->getValue() === Tense::Present
            && $person->getValue() === Person::SecondPersonSingular
           )
       )
   )
{
    $word_stem = word_stem_length($infinitiveVerb, 4) . 'u';
}

if ($exceptionmodel->getValue() === ExceptionModel::VALOIR 
   && ($mood->getValue() === Mood::Subjonctif 
       && $tense->getValue() === Tense::Present
       && in_array($person->getValue(), 
                   [ 
                     Person::FirstPersonSingular,
                     Person::SecondPersonSingular,
                     Person::ThirdPersonSingular,
                     Person::ThirdPersonPlural
                   ])
      )
   ) 
{
    $word_stem = word_stem_length($infinitiveVerb, 4) . 'ill';
}

In addition to making them all visible, I've indented the different blocks as best I can. Possible bug: Do you have an parenthesis issue in the first if block after the in_array() where there seem to be missing an parenthesis? I've changed it here, but you need to reiterate it, to verify its correctness. In general if you wrap and get alternating && vs || you need to take care and verify correctness regarding ordering and grouping of conditionals.

Also note how I moved the starting brace, {, to the next line to keep it aligned with both the if and the ending brace, }. This also gives a much needed vertical space to break between conditions and the statement to be executed.

I've put the ending parenthesis, ), on a separate line as well, this comes somewhat down to personal taste, so do that if that pleases you. Or pack them together on the previous line. Just be consistent, and when dealing with long lines like in this block, try avoid having more than one condition on any line.

Precompute condition parts

Another good option can be to precompute the parts used in the condition. This has at least the following advantages:

  • Shorten the condition statement in general
  • Make it read more easily, as you clearly indicate the test condition
  • Avoid computing values more than once, i.e. $tense->getValue()
  • Simplify grouping and ordering of multiple conditions
  • Reuse of condition part in following if statements

Doing this you can end up with the following:

// Precompute condition parts
$exceptionIsValoir = $exceptionmodel->getValue() === ExceptionModel::VALOIR;

$moodVal =  $mood->getValue();
$moodIsIndicatif = $moodVal == Mood::Indicatif;
$moodIsConditionnel = $moodVal === Mood::Conditionnel; 
$moodIsSubjonctif =  $moodVal === Mood::Subjonctif;

$tenseVal = $tense->getValue();
$tenseIsPresent = $tenseVal=== Tense::Present;
$tenseIsFutur = $tenseVal === Tense::Futur;

$personVal = $person->getValue();
$personIs_2S = $personVal == Person::SecondPersonSingular;
$personIs_1S_2S_3S = in_array($personVal, [
    Person::FirstPersonSingular,
    Person::SecondPersonSingular,
    Person::ThirdPersonSingular
  ]);
$personIs_1S_2S_3S_3P = in_array($personVal, [
    Person::FirstPersonSingular,
    Person::SecondPersonSingular,
    Person::ThirdPersonSingular,
    Person::ThirdPersonPlural
  ]);


// The actual validations  
if ($exceptionIsValoir
    && (   ($moodIsIndicatif && $tenseIsPresent && $personIs_1S_2S_3S)
        || $tenseIsFutur
        || ($moodIsConditionnel && $tenseIsPresent)
        || ($moodIsImperatif && $tenseIsPresent && $personIs_2S)))
{
    $word_stem = word_stem_length($infinitiveVerb, 4) . 'u';
}

if ($exceptionIsValoir
    && ($moodIsSubjonctif && $tenseIsPresent && $personIs_1S_2S_3S_3P))
{
    $word_stem = word_stem_length($infinitiveVerb, 4) . 'ill';
}

Early returns to reduce nesting levels

The concept of early returns to reduce nesting levels, is that if you have a structure like the following with no else block on the first few levels:

function something() {
    if first_level_condition {
        if second_level_condition {
            if third_level_condition and something {
                do something useful
            } else {
                do something else
            }
        }  // Note missing else block on second level
     }  // Note missing else block on first level
}

Then you can simplify this to:

function something() {
    if not first_level_condition
       or not second_level_condition {
        return
    }

    if third_level_condition and something {
        do something useful
    } else {
        do something else
    } 
}

Now you've moved your code of interest, that is level three, out two indentation levels, which gives you some more room to write code in.

At first glance of your example, it seemed like you could do this in your code as the two original cases both had the common element of $exceptionmodel->getValue() === ExceptionModel::VALOIR, which could have been negated, and caused an early return. However in your added excerpt, and in the original code many more variants exists making this not so useful in your context.

It is still a useful pattern to be aware of, when doing multiple if statements and nesting of these.

(Optionally) Build condition groups

Another option which I've used sparingly is to actually precompute the entire groups of condition statements, using constructions like the following:

// Precompute condition parts
... as before ...

// Build condition group
$c = ($moodIsIndicatif && $tenseIsPresent && $personIs_1P_2P_3P)
$c = $c || $tenseIsFutur
$c = $c || ($moodIsConditionnel && $tenseIsPresent)
$c = $c || ($moodIsImperatif && $tenseIsPresent && $personIs_2S)

if ($exceptionIsValoir && $c)
{
   $word_stem = word_stem_length($infinitiveVerb, 4) . 'u';
}
...

This construct can be useful in some circumstances, but in your case I believe I would stop at precomputing the condition part using good condensed variable names, and do an early return if possible.

Addendum 1: Automating generation of condition variables

You ask if one can automate stuff like $exceptionIsValoir or $tenseIsPresent, and as I'm a bit rusty I php I posted my version as a question here: Dynamic variables in Php from enum. But it is possible to make dynamic variables in PHP, and one way of doing it is:

$exceptionVal = $exceptionmodel->getValue();
foreach(ExceptionModel::getConstants() as $constName => $constValue) {
    ${'exceptionIs' . $constName} = $exceptionVal === $constValue;
}

$tenseVal = $tense->getValue();
foreach (Tense::getConstants() as $constName => $constValue) {
    ${'tenseIs' . $constName} = $tenseVal === $constValue;
}

This would allow you to use $exceptionIsALLOIR or $tenseIsPresent. You could optionally change the first one to use $exception_is_ALLOIR by changing the constant string prefix.

With regards to the use of in_array() within the if conditions, I'm no personal fan of it. This is partly because it is lengthy and kind of confusing as you prefix the enums with full class name. I would rather precompute these manually, like in $personIs_1S_2S_3S, as this can shorten the text compared to the rather lengthy version of $personIsFirstPersonSingular || $personIsSecondPersonSingular || $personIsThirdPersonSingular.

Regarding precomputing combinations of ExceptionModel, I would do it on a case by case basis. Possibly I there would use the or'ed combination as I don't see a neat way of shortening the names as is easily done for the Person enum.

Addendum 2: Reconsider general concept of word_stem()

When following your links to the original code I see that word_stem() is a rather long function consisting of above 40 disconnected if statements similar to the two above (or four in modified post), spanning over 300 lines. That is a heavy function, and raise some concerns:

  • Can it be simplified by using other functions?
  • Can it be simplified by connecting together the if's, so that you don't need to execute all of them each and every time?
  • How do you test such a massive beast if it is correct at all?
  • If you match on one of the if's, does that exclude all others? In other words, can you do return $word_stem directly instead of at end?

Without looking into the functions too much, I would consider if you should make functions like word_stem_Exer_Exe and word_stem_xER and word_stem_ENVOYER, and let the code in word_stem() be:

if ($exceptionIs_ELER_ELE_or_ETER_ETE) {
    $word_stem =  word_stem_Exer_Exe($person, $tense, $mood);
}
if ($exceptionIsCER or $exceptionIsGER or $exceptionIsE_Akut_CER
    or $exceptionIsE_Akut_GER)
{
    $word_stem =  word_stem_xER($person, $tense, $mood);
}
if ($exceptionIsENVOYER) {
    $word_stem = word_stem_ENVOYER($person, $tense, $mood);
}

Or possibly use if ... else if ... else if which would stop evaluating the if sequences at first hit. In other words, this would require that only one of the 40++ if's matches. Using return directly would also eliminate evaluating further if's effectively.

Using functions like word_stem_xxxx() would also ease the testing some, as you could specific test that exception group. With these functions you would then move the person, tense and mood tests inside of the function, and then they could possibly be clearer in general even without that much shortening. You could then possibly only use the first tip of $tenseVal = $tense->getValue() to shorten them sufficiently.

Disclaimer: I don't know the entire context, and haven't thoroughly read the accompanying code, so consider this carefully before reimplementing into sub-functions or else-if's as that might have side effects not known to me. Then again, you might have a lot of side effects already in the massive beast that word_stem() is.

Addendum 3: Automagical is<ENUM_VALUE>() methods

An alternative approach to creating the temporary variables, is to use reflection and automatically define test function for equality to any given enum value.

If extending the base Enum class, or the ExtensionModel class with the following function:

function __call($func, $param) {
    $func_prefix = substr($func, 0, 2);
    $func_const = substr($func, 2);
    if ($func_prefix == "is") {
        $reflection = new ReflectionClass(get_class($this));
        return $this->getValue() === $reflection->getConstant($func_const);
    }
}

Then it is legal to do stuff like in the following test function:

function myFunction(ExceptionModel $exceptionModel, Tense $tense) {

    if ($exceptionModel->isALLER()
        && $tense->isPresent() ) {
        ... do something ...
    }

given that both ExceptionModel and Tense inherits from the Enum class. In other words, now you can do $enumobject->is<ENUM_VALUE>() for any enum value inheriting from Enum.

PS: After posting my original answer you added two more excerpts. I've not included those in my analysis, but the concept will be the same only with a few more lines added.

\$\endgroup\$
1

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.