current community

your communities

more stack exchange communities

Stack Exchange Inbox Reputation and Badges
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 4 down vote favorite

I need to change some array values in a 2-dimensional array for some cases. Sometimes the all values from an child array and sometimes only some values. In this cases I should only change the variable $stamm which $stamm2.

This code is the statement of an if condition, where all relevant verbs for this changings is mentioned.

$verb could be one over 25.000 french verbs. For this example you could use 'acheter'.

  $array = array( 
  array('e', 'es', 'e', 'ons', 'ez', 'ent'), array('ais', 'ais', 'ait', 'ions', 'iez', 'aient'), array('ai', 'as', 'a', 'âmes', 'âtes', 'èrent'), array('erai', 'eras', 'era', 'erons', 'erez', 'eront'), array('e', 'es', 'e', 'ions', 'iez', 'ent'), array('asse', 'asses', 'ât', 'assions', 'assiez', 'assent'), array('erais', 'erais', 'erait', 'erions', 'eriez', 'eraient'), array('e', 'ons', 'ez'), array('XXX'), array('ant'));

My short old code:

$stamm = substr("$verb", 0, - 2);    
$stamm2 = substr_replace($stamm, '<u>è</u>', -2, 1);

$array[0][0] = $array[0][2] = $array[4][0] = $array[4][2] = $array[7][0] = $stamm2.'e';
$array[0][1] = $array[4][1] = $stamm2.'es';
$array[0][5] = $array[4][5] = $stamm2.'ent';

My new improved code:

$stamm = substr("$verb", 0, - 2);    
$stamm2 = substr_replace($stamm, '<u>è</u>', -2, 1);

$zeiten = array(0, 3, 4, 6, 7);        
foreach($zeiten as $zeit) { 
    foreach($array[$zeit] as $person => $value) {               
        if ((($zeit == 0  || $zeit == 4) && !in_array($person,array(3,4))) || ($zeit == 3 || $zeit == 6) || ($zeit == 7 && $person==0)) 
        $array[$zeit][$person] = str_replace($stamm, $stamm2, $value);      
        }   
    }

Is this the the new code the best solution? Is it possible to improve the if condition?

share|improve this question
1  
Please provide a more context on the problem you are trying to solve and what values are in $array, $stamm and $stamm2. –  Nobody Jun 10 at 8:53
    
@Nobody I have edited my question. –  Grischa Jun 10 at 9:06
1  
It is still missing the variable $array. To make it more clear: Preferably your code should be runnable as is. As far as I can see you should provide example values for $array and $verb and would be fine. –  Nobody Jun 10 at 9:18
    
@Nobody Their is also an array_walk function to combine $array and $stamm. The code is workung for both versions. –  Grischa Jun 10 at 9:25
1  
You are still missing an example value for $verb and the overall description of what you are trying to do. My assumption is: You are trying to build a function that gives all possible inflections of a given verb according to french language. Is that right? –  Nobody Jun 10 at 9:31

1 Answer 1

up vote 6 down vote

Let me first review this from an readability point of view, then answer your question and an proposed solution.

Naming

Your variable names could be more descriptive.

  • $array -> (maybe) $conjugations (german: $konjugationen)
  • $stamm -> $word_stem (german: $wort_stamm)
  • $zeiten -> $tenses (german: $zeitformen)?

Structure

The most intriguing part for me is $array's structure. As I understand it, it is a 2D table of conjugations of the $verb where rows have the same tense and columns the same person. This should be somehow reflected in the data structure because it is not obvious to the viewer. Maybe you should even implement a class for that.

Comparison

Maybe it has to do with my lack of domain knowledge (I don't speak french) but I find both solutions unreadable. This is enforced by the naming problems.

The second one looks slightly more general with regard to reuse but the if portion is not very flexible.

Improvement

As I said, I have no idea about french but I suppose this holds true:

Each verb can be conjugated for person and tense. There is a set of rules that defines how this happens for each person and tense depending on the verb's form and a set of exemptions from those rules.

The solution to the problem consists of:

  1. Classifying the verb as to which rule/exemption to apply
  2. applying the rule

To me this sounds like 1. could be solved by a factory that creates strategy instances for 2.

$conjugator = get_conjugator($verb, $tense, $person);
$conjugated_verb = $conjugator->conjugate($verb);

Maybe the exceptions/rules are even partial per tense or person. This would mean you would have to make this distinction as well and produce a separate conjugator for each tense, person, verb triple.

Now the code you have posted is pretty much the part that goes into the implementation of a conjugator. And seeing that there could be many of them it would be nice to make implementing them as simple as possible.

Their interface would probably consist of a function that detects whether they are applicable (the if condition in your case) and a function that does the actual flexion of the input (the if's then code block).

share|improve this answer
1  
I would even go as far to suggest that OP implement a Trie data structure. It is perfect for this. –  Evan Bechtol Jun 10 at 12:13
1  
@EvanBechtol: I am not sure if it is a pure string matching problem. It probably is more complex. –  Nobody Jun 10 at 12:20
    
This would however be a really nice usecase for a Trie. Good review! –  Pinoniq Jun 17 at 20:24

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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