current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 1 down vote favorite

I'm trying to read certain values from an object. What would be the best practise to do so?

$jira_object = json_decode(trim($jira_value->data));
$this->_displayName = $jira_object->user->displayName;
$this->_issueID = $jira_object->issue->id;
$this->_issueKey = $jira_object->issue->key;
$this->_issueFieldSummary = $jira_object->issue->fields->summary;
$this->_issueFieldProgress = $jira_object->issue->fields->progress;
$this->_issueFieldType = $jira_object->issue->fields->issuetype;
$this->_issueFieldUpdated = $jira_object->issue->fields->updated;
$this->_issueFieldCreated = $jira_object->issue->fields->created;
$this->_issueFieldPriority = $jira_object->issue->fields->priority;
$this->_issueFieldStatus = $jira_object->issue->fields->status;
$this->_issueFieldAssignee = $jira_object->issue->fields->assignee;
$this->_issueFieldProject = $jira_object->issue->fields->project;

How could I make this shorter?

share|improve this question
    
@KidDiamond Hmm then I'll stick to this, it only looks so ugly. –  Stijn Bernards Sep 10 '14 at 8:15
    
@KidDiamond Hmm I'll try thanks! –  Stijn Bernards Sep 10 '14 at 8:23
    
You are accessing $jira_object->issue->fields many times, so you could put that in a variable to reduce the code a bit. –  Guffa Sep 10 '14 at 8:45

2 Answers 2

up vote 1 down vote accepted

I don't think this can be much shorter as it is, probably also due to the lack of code context. The only thing the code you are showing does is assigning values to variables. There isn't any quicker way then using the assignment operator =.

Based on the code you posted, it would be best to tackle this issue on a higher level in your application. Maybe you need to rethink your class design in order to improve it's readability / usability.

You might already have all those values in some type of list, in that case you might be able to use a foreach() loop. Or you can try giving your variables better names, and perhaps make sure all the assignment operators are nicely aligned vertically to improve the readability.

Is $jira_object returned from json_decode(trim($jira_value->data)) really an object? Or is it just an array in object form? Since you are trimming the source data, I assume it's an array so I would keep it as one (unless you have any particular reasoning why you would want to access it like an object). Also the underscore _ prefix in your private variables is an old convention and is redundant.

The best I could come up with is the following while maintaining the readability:

$jira = json_decode(trim($jira_value->data), true);

$this->displayName        = $jira['user']['displayName'];
$this->issueID            = $jira['issue']['id'];
$this->issueKey           = $jira['issue']['key'];
$this->issueFieldSummary  = $jira['issue']['fields']['summary'];
$this->issueFieldProgress = $jira['issue']['fields']['progress'];
$this->issueFieldType     = $jira['issue']['fields']['issuetype'];
$this->issueFieldUpdated  = $jira['issue']['fields']['updated'];
$this->issueFieldCreated  = $jira['issue']['fields']['created'];
$this->issueFieldPriority = $jira['issue']['fields']['priority'];
$this->issueFieldStatus   = $jira['issue']['fields']['status'];
$this->issueFieldAssignee = $jira['issue']['fields']['assignee'];
$this->issueFieldProject  = $jira['issue']['fields']['project'];

You could also put $jira['issue']['fields'] into a variable, but it wouldn't be that of an improvement. You will have extra lines of code without any significant benefits and it would fall under premature optimization, which is the root of all evil.

share|improve this answer
    
Thanks, this looks way nicer! I always wan't my code to look good. Don't know why.. I guess it's a good thing thanks! –  Stijn Bernards Sep 10 '14 at 8:53
up vote 1 down vote

This is shorter, but not necessarily better or more readable.

$jira = json_decode(trim($jira_value->data), true);

$this->displayName = $jira['user']['displayName'];
$this->issueID = $jira['issue']['id'];
$this->issueKey = $jira['issue']['key'];

foreach ($jira['issue']['fields'] as $key => $val) {
   $key = ucfirst($key);
   $this->{"issueField$key"} = $val;
}
share|improve this answer
    
Hmm I'll have a look into that. –  Stijn Bernards Sep 10 '14 at 9:02
    
Wouldn't you have to apply ucfirst() on the keys? –  Kid Diamond Sep 10 '14 at 9:46
    
I think you mean ucfirst(), but yes you are correct –  bumperbox Sep 10 '14 at 9:48

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.