0

This is my second php program. I'd like to ask about efficiency of my code.

Is it a good way, or can I make it better? Please give me an idea on how to store data better. This is for educational purpose only.

I need to store data then preform a search by ID, first or last name and the data file format is:

1, John, Smith

2, Mary, Gray

3, Tim, Cook

// php
    $fp = fopen("data.txt", 'r');
    $filestring ="";
    while(!feof($fp))
    {
        $temp = fgets ($fp);
        $temp = rtrim ($temp, "\r\n");
        $filestring= $filestring.$temp.",";
    }

    echo $filestring;
    echo "Convert string into array";
    $students = str_split ($filestring, strlen ($filestring));
    echo var_dump ($students);

    echo "Put each value into array cell";
    fclose ($fp);
    foreach ($students as $key=>$value){
        $students [$key] = explode (",", $value);

    }
Natasha Almazova
  • 7
  • 4
  • 4
    Maybe should be on [Code Review](http://codereview.stackexchange.com/)? – Sam Feb 14 '14 at 20:22
  • In most cases a database is better – Paul Dessert Feb 14 '14 at 20:23
  • I cannot use database here...just php script and file – Natasha Almazova Feb 14 '14 at 20:23
  • If you're limited to that, why are you asking? – Paul Dessert Feb 14 '14 at 20:24
  • If you can't use any kind of database, then you could probably run this more efficiently treating it as a csv file and using PHP's built-in [fgetcsv()](http://www.php.net/manual/en/function.fgetcsv.php) function – Mark Baker Feb 14 '14 at 20:27
  • I am asking just about the way I put data, if it good or not...the database is not an option...it just a beginner level class assignment for php – Natasha Almazova Feb 14 '14 at 20:27
  • what do you mean by -"You can just see if a line "starts with" the specific ID; no need to explode it until there is a match", I need to store all data first then search for value. – Natasha Almazova Feb 14 '14 at 20:33
  • yes, and what do you mean by -"You can just see if a line "starts with" the specific ID; no need to explode it until there is a match", I did not get that part.. – Natasha Almazova Feb 14 '14 at 20:34
  • See [this answer](http://stackoverflow.com/a/13246630/2864740) for how to read a file one line at a time. – user2864740 Feb 14 '14 at 20:40
  • @user2864740 oh I see now...this like a short cut in data storing...hm...I am not sure if the prof. will like it, haha...I'll ask him...it is a good idea!...I know...I cannot say "thanks here"...so here you go - spasiba (in Russian) LOL – Natasha Almazova Feb 14 '14 at 20:42
  • @user2864740 yes yes, in checking, I got it...The problem is I just remembered that I also need to search for first name starts with "first letter", so can I use strpos then? – Natasha Almazova Feb 14 '14 at 20:48
  • @NatashaAlmazova There are numerous issues that need to be addressed before continuing with any form of optimizing. – user2864740 Feb 14 '14 at 20:54
  • Stack Overflow is for specific questions about coding with definite answers. This is more appropriate for codereview.stackexchange.com. – Adi Inbar Feb 14 '14 at 23:02

1 Answers1

0

There are several issues with this code.

I've outlined some of them.

The following code with a string-concat operation after reading each line which is wasteful; if you are going to read in an entire file as a string, use file_get_contents which is much more efficient. It also changes all newlines to "," so at the end $filestring will contain "1, John, Smith,2, Mary, Gray,3, Tim, Cook" - uhg. (It also doesn't check for the result of fgets so it could also result in an error.)

$fp = fopen("data.txt", 'r');
$filestring ="";
while(!feof($fp))
{
    $temp = fgets ($fp);
    $temp = rtrim ($temp, "\r\n");
    $filestring= $filestring.$temp.",";
}

The following looks like it was meant to split the line into files, however the usage is very wrong. Refer to str_split for what it means (it splits on length, not character sequence) - basically, the $students array will be only one element long and contain "1, John, Smith,2, Mary, Gray,3, Tim, Cook", from above.

echo $filestring;
echo "Convert string into array";
$students = str_split ($filestring, strlen ($filestring));
echo var_dump ($students);

The following doesn't work due to errors in the previous code and it makes no sense to update the $students array which is "the [supposed] array of lines from the file". (Using appropriate variable names can help clarify code intent.)

echo "Put each value into array cell";
fclose ($fp);   # wut? this should be up top!
foreach ($students as $key=>$value){
    $students [$key] = explode (",", $value);
    # check here
}

Consider starting as so ..

$lines = array();  # Read each line into an array of lines.
                   # I'm leaving it as such because this is trivial to convert
                   # it to a streaming approach as per the final comment.
$fp = fopen("data.txt", 'r');
while(!feof($fp))
{
    $line = fgets($fp);
    if ($line !== false) {
        $lines[] = rtrim($line);
    }
}
fclose($fp);

# Then, for each line in $lines, extract the Student data and do something
# with it, such as printing a result if there is a match. If you need an array
# of students, create a $students array.

# Note that you don't need the $lines array at all! But could do something
# per-line inside the reading loop, where it uses the $line variable.
user2864740
  • 60,010
  • 15
  • 145
  • 220