Posted by Graham Leach on Thu, Oct 10, 2024 - 6:50am --- Updated: Oct 10 '24 - 7:40am

Q:  Is the following good programming practice according to the "Backdrop CMS" point of view? 

 

/**

 * Grabs the items in a shopping cart for a user.

 *

 * @param $cid

 *   (optional) The cart ID to load, or NULL to load the current user's cart.

 * @param $action

 *   (optional) Carts are statically cached by default. If set to "rebuild",

 *   the cache will be ignored and the cart reloaded from the database.

 *

 * @return array

 *   An array of cart items.

 */

function uc_cart_get_contents($cid = NULL, $action = NULL) {

  static $items = array();

  $cid = $cid ? $cid : uc_cart_get_id(FALSE);

 

I am just wondering whether or not the highlighted line of PHP code is representative of good programming practice?  

 

Syntax:

(Condition) ? (Statement1) : (Statement2);

·      Condition: It is the expression to be evaluated which returns a boolean value.

·      Statement 1: it is the statement to be executed if the condition results in a true state.

·      Statement 2: It is the statement to be executed if the condition results in a false state.

 

Personally I find it hard to understand how $cid can work as a condition, as this variable does not range across BOOLEAN values (TRUE | FALSE).

I also find it hard to believe that an unadorned $cid can also work as it is being used in the above ternary, and yet, it does.

According to my reading, the value of $cid ranges across:

  • NULL (if $cid is not passed in as an argument)...perhaps to accommodate anonymous shopping?  Who knows?  Alas, there are no comments to help explain such things.

 

  • The UID of the current user (which makes the most sense if you want to link the shopping cart to an authenticated user.

 

  • The result of md5(uniqid(rand(), TRUE)) as generated by uc_cart_get_id()  which, presumably will create a shopping cart for each anonymous shopper, as that $cid is also stored in their $_SESSION['uc_cart_id']. 

 

I am also (not) loving the return stanza from uc_cart_get_id(), which reads:

 

return isset($_SESSION['uc_cart_id']) ? $_SESSION['uc_cart_id'] : FALSE;

 

Can someone help to enlighten me regarding the above ternary statements please?  

Why are they being used everywhere and why in these various ways?

What's so bad with this code?


# Determine if a Cart ID ($cid) has been passed in as an argument

if (is_null($cid)) {

  # No $cid was passed in. 

  # Generate one with a function that either uses the User ID

  # Or, in the case of an anonymous shopper, a very random number

  $local_cart_id = uc_cart_get_id();

}

else {

  # A Cart ID was passed in as an argument.

  # Place it in a local variable.

  $local_cart_id = $cid

}
 


If someone uses the words "compact" or "elegant" in response I think they have something to learn about defensive programming, code smell, long-term code maintainability and responsibility regarding the accumulation of technical debt.

Comments

Thu, Oct 10, 2024 - 8:06am by yorkshirepudding

Interesting question.  I see that style of conditional logic regularly in Backdrop core and contrib.

I think the technical term for that kind of boolean is "truthy" or "falsy" ("All values are truthy unless they are defined as falsy"). - the value is not exactly TRUE or FALSE, but something that equates to it.

I have previously asked the question about whether we should or shouldn't use truthy or falsy but I think it was in a Dev meeting and I've no idea which one or what the answer was.  I do see it often and sometimes it will always be ok, but sometimes it is better to take a defensive programming approach.

There can be advantages to a shorter code base when you are maintaining it, and sometimes a line like that can help keep the code closer together so it is easier to see the big picture around what the code is doing; too many of the latter more verbose example can make the code harder to follow.

I think there is a place for both approaches.

"Code smell" is a new term to me - what does it mean?

Thu, Oct 10, 2024 - 7:11pm by Graham Leach

Hey @yorkshirepudding,



I personally hate ternary statements because I always need to waste brain cycles unraveling them rather than just trace out (to me) a much more brain-friendly (to me) if...then...else or switch construct, which is what I grew up with.
 

I was also brought up on uncomplicated (BASIC) and strongly typed languages (Pascal) and then developed further in a corporate-style programming environment (RPG, COBOL, INFORMIX) where the cost of maintenance was heavily front-loaded, so I have probably developed a certain bias in that direction

I met my first weakly typed languages in University, and my (pedantic) Professors discouraged constructs that were in any potential future booby traps (or maybe difficult to grade).   In my Graduate days Agile was all the rage and OOP was also just emerging, so I am carrying that legacy as well.

 

Just because a language is weakly typed doesn't mean the abuse of its constructs is a good idea.  We can choose to speak with English or French grammar in English, but one of them typically makes more sense, and the other demands that the listener perform unnecessary work and may also not receive the meaning payload all that quickly or easily - which is my point.



The construct

 

$cid = $cid ? $cid : uc_get_cart_id(FALSE);

 

In my view has a bunch of problems

 

Even according to the permissive nature of PHP shouldn't it have been?

 

$local_cid = (bool) ($cid) ? $cid :  uc_get_cart_id();

 

1.  Use different names for locally used variables and arguments

 

2.  Cast to correct datatype in CONDITION

 

3.  Call to function should return a value and not require an argument

 

Here's some information on "code smell".  I find Drupal code very smelly :)

 

https://en.wikipedia.org/wiki/Code_smell

 

One way to look at smells is with respect to principles and quality: "Smells are certain structures in the code that indicate violation of fundamental design principles and negatively impact design quality".  Code smells are usually not bugs; they are not technically incorrect and do not prevent the program from functioning. Instead, they indicate weaknesses in design that may slow down development or increase the risk of bugs or failures in the future. Bad code smells can be an indicator of factors that contribute to technical debt.  Robert C. Martin calls a list of code smells a "value system" for software craftsmanship.
 

Sat, Oct 12, 2024 - 5:45pm by hosef

The official PHP rules for casting variables to boolean are here: https://www.php.net/manual/en/language.types.boolean.php#language.types.boolean.casting

In regards to the example:

  1. I don't understand what the point of changing the name of variables is in this case.
  2. When you wrap an expression in parenthesis, it is automatically cast to a boolean. I would use (bool) $cid when I need to return the value or store it in a variable, but I would just use ($cid) when I am using the variable as a condition.
  3. Again, I am not sure what the point of this is. Is there some reason that the function should not have a parameter? 

 

Fri, Oct 11, 2024 - 12:25pm by kiamlaluno

Personally I find it hard to understand how $cid can work as a condition, as this variable does not range across BOOLEAN values (TRUE | FALSE).

PHP is not a strictly-typed programming language.
A condition can be also an expression that returns an integer, which is equivalent to TRUE when its value is different from 0, and FALSE when its value is 0. It could also be an expression returning a string; in that case, it is equivalent to FALSE when the string is empty, or it is "0".

Sun, Oct 13, 2024 - 12:58am by Graham Leach

I would be the first to admit that I am not some kind of "uber" programmer.  

While I do have a Computer Science degree, my commercial experience always leaned more heavily towards Business/Systems Analysis (my first job), Database Programming, Network Engineering, CTO, CIO and then CEO roles when I was working in industry, which was a long time ago (1995 - 2010).  

From 2010 to 2022 I was a University Professor (Computer Science, Engineering, Design).  I retired in 2022.  

I am probably horribly "old fashioned" to most people coming up today.

Here's my answers to your points:

  1. Readability (one of my points)
  2. If true, why then was it not "boolean" wrapped (another of my points)
  3. Why should a simple function call that returns a UID ever need a parameter?
  4. (bonus) Under what circumstances should a UID generator function return NULL?

Your comments around parentheses were interesting, but I didn't accept them at face value.  So I elected to refresh my understanding of them:

https://stackoverf101low.com/questions/52987991/what-do-parentheses-around-a-variable-do-in-php

I want to make my orientation clear.  My focus is lifecycle-oriented.  I recently saw a post claiming that 70% of the cost of having a codebase is maintaining it, not writing it.  This whitepaper also discusses that fact:

https://www.krugle.com/resources/downloads/Krugle_WP_Hidden_Costs_of_Code_Maintenance.pdf

As for understanding how boolean types are handled, here's what I am reading:

https://www.php.net/manual/en/language.types.boolean.php

I believe that Backdrop CMS may not realize that it has inherited a massive technical debt in the form of a low-quality (from a lifecycle perspective) codebase where 70% of the cost has yet to be realized.

The sooner this fact is internalized by the Backdrop CMS community and  an appropriate response designed into Backdrop CMS culture, the better.  The result will be higher-quality code that is easier to understand, adapt and adopt.

Here's a couple of analogies and a final statement for not just you, but everyone  through time who elects to visit this topic:

  1. It is possible to pass an exam without preparation, but this behavior, while possible, is not generally considered wise - even though it may be admired by the immature among us because someone "got away with it". 
  2. It is equally possible to eat all of the "complimentary" cookies in the staff room in the place of bringing your own lunch - and some delight in having done so for having "hacked the system".   

Both are examples of a certain type of systemic failure, where a lone actor sought to improve their specific situation at the cost of the system as a whole.  

In the first example, society as a whole pays the price because an unqualified person "passed" through a screening process (the exam).  Now there is a credentialed person who may not be competent enough to supply the service that the credential advertises.  In the second example, co-workers and guests collectively pay the price for one person having been "selfish".  Both examples are fairly common, yet deeply antisocial.  Perhaps the default mode for people, without adequate encouragement (or control) is to be antisocial?

(this is essentially the "natural philosophy" debate between Hobbes, Locke, Rousseau and Hume, by the way.  It is hardly my idea).

Backdrop CMS, along with its associated community, is another example of a "system as a whole", is it not?  How about we decide as individuals to behave that way, and actively encourage the group in that direction as a whole?  Under those circumstances, which thought patterns (and consequent actions) should be "smiled upon"?  Which "frowned upon"?  Which "tolerated"?  Which "forbidden"?

I would like the act of creating code for the Backdrop CMS system to be "super easy for the next person to understand and potentially modify to their needs" to be a formally "smiled upon" behavior, and for Backdrop CMS to incorporate the necessary enabling messaging for that behavior to be projected across the Backdrop CMS community and Backdrop CMS itself - the Drupal 7 intellectual property that the community has elected to parent.

 

Sun, Oct 13, 2024 - 9:02am by hosef

I am not an Ubercart expert, so I had to go read the module to answer some of these.

  1. I do not agree that having more variable names floating around my code makes it easier to read.
  2. I did not write that code. It's current form is 16 years old at least. That was before I ever wrote a single line of code.
  3. The function in question does not return a user ID, it returns a cart ID. The parameter changes what happens when there is not yet a cart available for the current session.
  4. There might not be a UID available, such as when the user has not logged in.

I think that we have internalized the fact that maintaining code bases is expensive. We also have a limited amount of time and money to spend rewriting all 52,000 contributed modules and who knows how many custom modules. As such, we don't spend time changing things that we don't have to.

I am not really sure what you want us to change, but it seems to me that you are asking us to take on an extremely heavy investment in rewriting code that works as is for no perceivable end user improvement.

Sun, Oct 13, 2024 - 8:18pm by Graham Leach

Hello @hosef,

First of all, thank you for engaging with me on this topic.  I appreciate it.

But, looks like we are going to "agree to disagree" on this one.  

Here's some more information for you to weigh:

1.

https://stackoverflow.com/questions/2753494/using-function-arguments-as-local-variables

2.

Your maturity (and attitude) in and towards coding is likely to evolve over time.  I say this because I know mine did, and so did the attitude of everyone in my cohort.  As you swim more and more through old code, I believe you'll start to appreciate my position, which is to recognize and face a problem with a long-term change in attitude and approach, not to ignore it.  One of the things that's happening is PHP is  progressively "tightening up" and Drupal/BCMS "breaks" quite frequently as a result due to its (to me) very low-quality code.

3., 4.

You may have missed this tidbit in uc_cart.module:

function uc_cart_get_id($create = TRUE) {

 global $user;

 if ($user->uid) {

   return $user->uid;

 }

 elseif (!isset($_SESSION['uc_cart_id']) && $create) {

   $_SESSION['uc_cart_id'] = md5(uniqid(rand(), TRUE));

 }

 return isset($_SESSION['uc_cart_id']) ? $_SESSION['uc_cart_id'] : FALSE;

}


Am I reading this code wrong?  Looks to me like the UID is returned for authenticated users and a randomized number for anonymous users - but who knows?  There's no explanation in comments (one of my biggest points).

BONUS QUESTION:  Why would a function named get_cart_id() feature a default behavior of create?  if you are going to go with a "weak OOP" model why not have  a roster of simple, single-purpose helper functions named:

get_cart_id();

set_cart_id();

new_cart_id()

I think it's pretty clear how I feel:  Very little of what I am seeing in code makes any sense from a "code parent" perspective.

Yes, it works - but isn't that the bare minimum ask of a talented, intelligent programmer?  The very first thing you do with someone who demonstrates talent and high-intelligence (rare stuff) is ask them to start leading others.  You ask them to start thinking and acting like a Technical Architect and then continue to push them onwards and upwards to increasingly high-level and abstract ways of thinking that expose more and more dimensions of challenge (CTO, CIO, Founder).  These are increasingly strategic roles.  Programming is mostly tactical, especially when a proper requirements analysis has been performed.  There's a step-change difference.

A lot of what I am seeing when I look at Drupal code is a false economy ("just get it working") where the true price for achieving that functionality is paid later, and over and over, in terms of wasted time ("where is everything?  why are things connected this way?  why are things being done this way?  why is nothing explained?").  Drupal seems kinda unique in this respect.  Many public Web Application Frameworks take pains to make their onboarding process easy, and encourage social behaviours that "help" the next person on the chain.  In the corporate space, due to the high cost of programming talent, this is usually the norm.  Who knows?  Perhaps it's an artifact of the era Drupal came into being, or the person that was its figurehead for many years (Dries).

Finally, regarding your last statements(s).  One of my biggest points is BCMS has already incurred the maintenance cost that Drupal left behind in the form of something called "technical debt".  It's unavoidable and being imposed on BCMS every day (resulting in a tiny community).  It's a large part of what makes Drupal "hard", which was not something that was commonly said of other PHP frameworks (CodeIgniter anyone?).

It shouldn't be a point of pride.  I don't know of many successful and lasting consumer-facing products that proudly say "We are a pain to work with".

I say there is a massive technical debt and BCMS has taken that debt on, for good or for ill.  If you agree with those two statements then I think the responsible reaction that leadership should take would be to propose a two-headed strategy that back-fills the incurred technical debt as quickly as possible (years, decades) and forward-fills from some point on with code that doesn't add to the problem.

Just in case I wasn't clear before, my meta-question was this:  Is BCMS doing anything to avoid repeating the obvious (to me) mistakes made by the Drupal community of the past?  What culture is being built around those principles?  What artifacts are on offer to guide those who want to participate with BCMS so they don't make the problem worse?

If something has been published by the BCMS project that addresses the above issues, I would love to see it.  It might the be foundation of a "BCMS way", which I would equally love to see.