Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

What do you think of my function. It is used to change a cyrillic word with a stress marker into the same cyrillic word but with the stress marker seperately represented as a number. As I want to return two values I pass two parameters by reference. I assume there is at most only one stress marker per word. Be critical. What could I improve here. I just want to see what people think.

function seperateStress($word,&$cleanWord,&$stressLetter){
    $stressLetter=-1;
    $cleanWord="";
    for($i=0;$i<strlen($word);$i++){

        if($word[$i]=="&" ){
            //echo $i."</br>";
            $stressLetter=$i;

            $i=$i+5;
        }else{
            $cleanWord .=$word[$i]; 
            //echo $word[$i].":".$i."</br>";
        }
    }


    //return array($cleansedWord,$stressedLetter);
}

//Sample Test run
$stressWord= "велосипе́ды";
$noStressWord="";
$stressLetter=-1;

seperateStress($stressWord,$noStressWord,$stressLetter);
echo $noStressWord."::".$stressLetter."</br>";
share|improve this question
It appears that I have found another solution to the problem. I stumbled across php's multibyte string functions and I am thus able to avoid using this implementation myself. Great :) – VaccinalBowl Sep 6 '12 at 8:34

1 Answer

up vote 2 down vote accepted

Normally I would look at the PHP manual and make sure you aren't recreating a function that already exists, but I have no experience with non ASCII characters so i don't feel comfortable with the terminology to look this up. I would suggest that you make sure of this though. This sounds like something that might already exist somewhere. As to your function, there are a few things that jumped out at me.

First are those references. They are a powerful tool, but I would be careful using them. They are relatively difficult to spot and not everyone knows what they do. Sometimes its better just to use plain return statements, but that is something you will have to decide based on the scope of this project. If its something that isn't likely to be used by others, then this is fine, otherwise you might think about changing it.

When incrementing a variable by more than one, a more elegant way of doing so is by using the following syntax. By the way, where did the five come from? Unless cyrillic always has three digit entities, then you run the risk of missing some of the escape sequence. For example nbsp is a four digit entity. I would use strpos() or something similar to find the position of the semicolon instead.

$i += 5;

$stressLetter and $cleanWord definitions are redundant. You set them to the same values before the function was called. The only reason this would be necessary is if you revert back to using a return value instead of references, and then you should remove those parameters from the parameter list and from the procedural code before the function call.

share|improve this answer
Thank you for the response. The reason I increment by five is because the string is something like врфвра&#134;вфыврю. On locating the ampersand I have found the stress marker and know I can skip the next few characters to get to the в. The reason the variables appear again is because I wanted to make sure they are null to start with as they are passed in as referecnes. I am using references because I wanted to return two values. Nobody else will look at this code I do not think. It's just for a pet project that I am doing in my own time. – VaccinalBowl Sep 4 '12 at 17:04
If you wan't to return two values then return an array as you have commented out. – vascowhite Sep 5 '12 at 11:28

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.