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.
up vote 3 down vote favorite

Here is the code: I have 3 objects, and 3 model. The note, have many tags, and every tags must have one note. So, I have a note_tag_list to store the tags and notes relationship.

The program is create a note, with some tags.

First, I create a note object, and put it into the model. And I get back the object from the model. And I detect whether the tag is exist, just don't create, otherwise, create it. Then, I read back the tag object from the db. And use all the tags and note to create new note_tag_list object, and put this object to the model. Then, it finished.

But I think it seems very complex for my flow. Any ways to do it more elegant? Thank you. 

//load all the object
$this->load->library('obj_note'); 
$this->load->library('obj_tag');  
$this->load->library('obj_note_tag_list'); 

//load all the model
$this->load->model('note_model');  
$this->load->model('tag_model');  
$this->load->model('note_tag_list_model'); 

//create a note object
$aNewUserNote    =new obj_note();

//set the object information
$user_id         =get_current_user_id();
$title           =$this->input->post('ui_title_input');
$content_url     =$this->input->post('ui_url_input');
$content         =$this->input->post('ui_textarea');
$right           =RIGHT_NOTE_PUBLIC;
$status          =STATUS_NOTE_ACTIVE;   

$aNewUserNote->set_user_id($user_id);
$aNewUserNote->set_title($title);
$aNewUserNote->set_content_url($content_url);
$aNewUserNote->set_content($content);
$aNewUserNote->set_right($right);
$aNewUserNote->set_status($status);

$aNewlyCreateNoteId = 0; 

if(!$this->note_model->create_note_by_note($aNewUserNote, $aNewlyCreateNoteId))
{          
    //create success
    return;
}   

//get back the object from the model
$aNewlyCreateObj = $this->note_model->read_note_by_note_id($aNewlyCreateNoteId);


$aTagNameString = $this->input->post('ui_tag');
$tagsStringArray = $this->convert_tags_string_to_lower_case_string_array($aTagNameString);

$aTagArray = array();  
$createNewTagIsSuccess = FALSE;

//Create New Tag
for($i = 0; $i < count($tagsStringArray); $i++)
{   
    $aTagObj = $this->tag_model->read_tag_by_tag_name($tagsStringArray[$i]);
    if($aTagObj == NULL)
    {
        $aNewTag =new obj_tag();
        $aNewTag->set_create_user_id($user_id);
        $aNewTag->set_tag_name($tagsStringArray[$i]);

        if(!$this->tag_model->create_tag_by_tag($aNewTag)) //create tag fail
        {
          $createNewTagIsSuccess = FALSE;  
        }
    }
}  

//Read all the tag
for($j = 0; $j < count($tagsStringArray); $j++)
{ 
    $aTagObj = $this->tag_model->read_tag_by_tag_name($tagsStringArray[$j]);
    $aTagArray[count($aTagArray)] = $aTagObj;
}                                            


//Make the note, and tag have relationship
for($k = 0; $k < count($aTagArray); $k++)
{                                       
    $theTagObj = $aTagArray[$k];          


    $aNoteTagList = new obj_note_tag_list();
    $aNoteTagList->set_note_id($aNewlyCreateObj[0]->get_id());
    $aNoteTagList->set_tag_id($theTagObj[0]->get_id());

    $this->note_tag_list_model->create_note_tag_list_by_note_tag_list($aNoteTagList);
}
share|improve this question
Try a codereview site. This feels OT. – pst Aug 22 '11 at 11:32
I started to give it a try, but not knowing your objects and return types/values, its pretty much impossible. There are a few things I can spot straight away (like setting a var to another var before you use it, and using for where you should use foreach), but that's all I'm doing. – adlawson Aug 22 '11 at 12:21

migrated from stackoverflow.com Aug 22 '11 at 12:47

3 Answers

up vote 1 down vote

You could write a constructor for your object:

//create a note object
$aNewUserNote    =new obj_note(get_current_user_id(),
                               $this->input->post('ui_title_input'),
                               $this->input->post('ui_url_input'),
                               $this->input->post('ui_textarea'),
                               STATUS_NOTE_ACTIVE
                              );

Same thing for your tag:

    $aNewTag =new obj_tag($user_id, $tagsStringArray[$i]);
share|improve this answer
up vote 0 down vote

This:

//set the object information
$user_id         =get_current_user_id();
$title           =$this->input->post('ui_title_input');
$content_url     =$this->input->post('ui_url_input');
$content         =$this->input->post('ui_textarea');
$right           =RIGHT_NOTE_PUBLIC;
$status          =STATUS_NOTE_ACTIVE;   

$aNewUserNote->set_user_id($user_id);
$aNewUserNote->set_title($title);
$aNewUserNote->set_content_url($content_url);
$aNewUserNote->set_content($content);
$aNewUserNote->set_right($right);
$aNewUserNote->set_status($status);

Should be rewritten as this

// create and populate note object
$note = new obj_note();           
$note->set_user_id(get_current_user_id());
$note->set_title($this->input->post('ui_title_input'));
$note->set_content_url($this->input->post('ui_url_input'));
$note->set_content($this->input->post('ui_textarea'));
$note->set_right(RIGHT_NOTE_PUBLIC);
$note->set_status(STATUS_NOTE_ACTIVE);

Other than that, my comment on your question will explain the rest.

share|improve this answer
up vote 0 down vote

Just some idea to improve the code a little bit:

1, I wouldn't count the size of the array in every iteration. So I'd change

for($i = 0; $i < count($tagsStringArray); $i++) { ... }

to

$size = count($tagsStringArray)
for ($i = 0; $i < $size; $i++) { ... }

2, Use guard clauses:

//Create New Tag
for($i = 0; $i < count($tagsStringArray); $i++)
{   
    $aTagObj = $this->tag_model->read_tag_by_tag_name($tagsStringArray[$i]);
    if($aTagObj != NULL) {
        continue;
    }

    $aNewTag = new obj_tag();
    $aNewTag->set_create_user_id($user_id);
    $aNewTag->set_tag_name($tagsStringArray[$i]);

    if(!$this->tag_model->create_tag_by_tag($aNewTag)) //create tag fail
    {
      $createNewTagIsSuccess = FALSE;  
    }

}

3, The following comments could be function names:

//Create New Tag
//Read all the tag
//Make the note, and tag have relationship

For example:

function readAllTags($tagsStringArray) {
    $aTagArray = array();  
    for($j = 0; $j < count($tagsStringArray); $j++)
    { 
        $aTagObj = $this->tag_model->read_tag_by_tag_name($tagsStringArray[$j]);
        $aTagArray[count($aTagArray)] = $aTagObj;
    }  
    return $aTagArray;
}

It makes the code flatten and easier to read: http://www.codinghorror.com/blog/2006/01/flattening-arrow-code.html

4, Use the value of $createNewTagIsSuccess or omit the variable. The code set it to FALSE before the create new tag loop and the loop sometimes set it to FALSE again. It looks little bit weird. (Maybe a bug? No code uses this flag.)

$createNewTagIsSuccess = FALSE;

5, I agree with @Loki Astari about constructors. Furthermore, Consider static factory methods instead of constructors (See Martin Fowler: Effective Java Second Edition, Item 1.)

6, AFAIK the index in following is unnecessary:

$aTagArray[count($aTagArray)] = $aTagObj;

So use

$aTagArray[] = $aTagObj;
share|improve this answer

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.