I'm trying to read certain values from an object. What would be the best practice 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?

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

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.


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;
  • \$\begingroup\$ Hmm I'll have a look into that. \$\endgroup\$ – Stijn Bernards Sep 10 '14 at 9:02
  • \$\begingroup\$ Wouldn't you have to apply ucfirst() on the keys? \$\endgroup\$ – Kid Diamond Sep 10 '14 at 9:46
  • \$\begingroup\$ I think you mean ucfirst(), but yes you are correct \$\endgroup\$ – bumperbox Sep 10 '14 at 9:48

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.