WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 2 months ago

#44111 closed enhancement (wontfix)

Setting data to variable early.

Reported by: rnaby Owned by:
Milestone: Priority: normal
Severity: normal Version:
Component: Administration Keywords: has-patch close
Focuses: administration Cc:

Description

I think setting data to variable early.

Attachments (1)

44111.diff​ (1.1 KB) - added by rnaby 3 years ago.
The patch for the ticket.

Download all attachments as: .zip

Change History (6)

@rnaby
3 years ago

The patch for the ticket.

#1 @SergeyBiryukov
3 years ago

  • Keywords reporter-feedback close added; dev-feedback removed

Hi @rnaby, welcome to WordPress Trac!

The existing code looks fine to me as is. Is there a problem solved by this patch?

#2 @rnaby
3 years ago

@SergeyBiryukov Thanks for giving feedback. Actually, it's not solving problem, I proposed it for a cleaner approach. Like we are setting some data to a variable and if some condition has met only then we're changing it. Otherwise it's being what it was. I think it makes the code simple and a little less lengthy to read or understand.

#3 @pento
3 years ago

  • Version trunk deleted

#4 @hellofromTonya
10 months ago

  • Keywords reporter-feedback removed

Removing reporter-feedback as @rnaby provided reasoning behind the ticket and patch.

@SergeyBiryukov What do you think?

#5 @hellofromTonya
2 months ago

  • Keywords needs-testing removed
  • Milestone Awaiting Review deleted
  • Resolution set to wontfix
  • Status changed from new to closed

Thank you @rnaby for your suggestion, reasoning, and patch!

Circling back to review the patch and request.

Core is filled with different patterns for setting the default state:

  • Pattern 1: within the else
  • Pattern 2: within the : of a ternary
  • Pattern 3: before the if conditional (proposed approach)

All of these patterns are technically valid.

An argument against using the proposed pattern (i.e. Pattern 3) is: performance. Why? If the conditional expression is true, then the variable is set twice. The performance hit here is tiny.

What about readability? Readability would take precedence over this tiny tiny performance hit. However, IMO the readability of one over the others is subjective. For example, @rnaby prefers pattern 3 while some contributors find the else or ternary to be more clearly expressive and aligning to the concept of if this do that; else do this other thing.

IMO there's not a compelling reason to enforce one pattern over the others through the coding standards nor to change the codebase to comply.

Given that this ticket was marked as a close candidate 3 years ago with no progress, closing this ticket as wontfix. However if there's enough interest, a new ticket could be opened to discuss which pattern to adopt as the coding standard.

Note: See TracTickets for help on using tickets.