2
\$\begingroup\$

I have 3 objects and 3 models. 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 creates 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. I create the tag if it doesn't exist. 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.

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);
}
\$\endgroup\$
  • \$\begingroup\$ 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. \$\endgroup\$ – adlawson Aug 22 '11 at 12:21
1
\$\begingroup\$

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]);
\$\endgroup\$
0
\$\begingroup\$

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.

\$\endgroup\$
0
\$\begingroup\$

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;
\$\endgroup\$

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.