1
\$\begingroup\$

I am new to PHP development. So Kindly, review my code and guide me make it better/optimize. Currently, it works well with inputs that I have tried.

<?php
// Program to find the word in a sentence with maximum specific character count
// Example: "O Romeo, Romeo, wherefore art thou Romeo?”
// Solution: wherefore 
// Explanation: Because "e" came three times
$content = file_get_contents($argv[1]); // Reading content of file
$max = 0;
$arr = explode(" ", $content); // entire array of strings with file contents
for($x =0; $x<count($arr); $x++) // looping through entire array 
{
$array[$x] = str_split($arr[$x]); // converting each of the string into array
}
for($x = 0; $x < count($arr); $x++)
{
    $count = array_count_values($array[$x]);
    $curr_max = max($count);
    if($curr_max > $max)
    {
        $max = $curr_max;
        $word = $arr[$x];
    }
}
echo $word;
?>
\$\endgroup\$
1
\$\begingroup\$

You could improve readability by splitting the character counting out into a function:

function most_repeated_count($word) {
    $letters = str_split($word);
    return max(array_count_values($letters));
}

Then use it to build an associative array mapping words to their counts:

$content = file_get_contents($argv[1]);
$words = explode(' ', $content);
$word_counts = array_combine(
    $words,
    array_map('most_repeated_count', $words)
);

And finally, sort that array:

arsort($word_counts);
$max = key($word_counts);

All together, now!

\$\endgroup\$
  • \$\begingroup\$ Because sorting is costly, especially with long arrays, would it improve performance to replace the last two lines with $max = array_search(max($word_counts),$word_counts); \$\endgroup\$ – BeetleJuice Jul 3 '16 at 6:55
  • \$\begingroup\$ @BeetleJuice: Probably, but for performance, I'd avoid creating a bunch of big new arrays, too. So this answer's all about readability for now. \$\endgroup\$ – Ry- Jul 3 '16 at 7:55
2
\$\begingroup\$

arr and array

These arrays have too similar names. It's better to avoid using too similar names, to avoid confusion and mistakes.

These names are also poor, as they tell nothing about what's in the arrays. $words would have been a more natural name.

There's no need for both of these arrays at the same time. When you loop over the words to create another array of chars-of-words, you could go one step further, skip the array creation and do the character counting, and save the extra storage of another array.

Code organization

It's good to break your program to multiple small functions, each with a single responsibility. It will make it possible to test smaller and bigger logical units alike independently. For example, you could have functions like:

  • read a file and return an array of words
  • find the count of the most frequent character in a letter
  • find the word with the most frequent character in an array of words

Naming

$x is a bit unusual the counter variable in loops. $i is more common. Some of the improvements I suggested above will hopefully help you replace the counting for loop with a foreach loop. That way instead of a counter variable, you will be able to use something more natural like $word.

\$\endgroup\$
  • \$\begingroup\$ I had posted the question twice by mistake. Deleted it. Anyways thanks. \$\endgroup\$ – Unbreakable Jul 5 '16 at 6:45

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.