tags:

views:

121

answers:

2

A

if (  ( empty($infoA) || empty($infoB) ) && ( !empty($inputA) && !empty($inputB) )  ) 
{
  //add
}

B

if (  ( !empty($infoA) || !empty($infoB) ) && ( empty($inputA) && empty($inputB) )  )
{
  //remove
}

C

if (  ( !empty($infoA) || !empty($infoB) ) && ( ($inputA != $infoA) || ($inputB != $infoB) )  ) 
{
  //add
}

So, in order to not repeat the add, we can:

if (A || C) 
{
  //add
}
elseif(B) 
{
 //remove
}

Any better logic to be applied here on your option?

Context: I believe it's irrelevant what this should do. I mean, it's a logical question. :s Not sure what to write here... :(

This is for a form: some inputs will come from the database, others from input fields. We are doing some comparisons here.

A context: If the value that comes from the database is empty, and the input fields A and B are NOT empty, do add to database.

B context: If the value that comes from the database is NOT empty, and the input fields A and B are empty, do remove from the database.

C context: If the values that comes from the database are NOT empty, AND the input fieldA !equal infoA or input fieldB NOT equal database value infoB then, do add to the database.

Please advice. MEM

+1  A: 

Several questions:

  • Is C really Add, or is it update?
  • Do you really need to tie procesing of inputA and inputB with infoA and infoB?

In general, I would refactor this by:

  • Splitting the add/remove decision into a separate process_input function which handled just one pair of input/info objects.
  • Spliting add/remove activities into their own functions which would handle basic empty handling of the data.

The only simplification of the logic is that the decision for calling add() or remove() is only based on whether or not the input data is empty or not. That's all that's really needed. The add() and remove() functions do their own data validation to make sure the operation is valid for the supplied data.

This refactoring demonstrates two important programming techniques:

  • The DRY (don't-repeat-yourself) principle... we shouldn't be checking empty() on an object more than once.
  • SRP (single-responsibility-principle) - each operation should only do one thing.
    • The input handler is responsible for pairing the input data with the existing info data and then calling process_input.
    • The process_input function is only responsible for deciding whether we need to do an add() or remove() operation.
    • The add() and remove() functions are each responsible for their own distinct operation which can be tested in isolation of the rest of the code

(Excuse my php... it's a bit rusty)

function remove($data)
{
    if (empty($data))
        return;

    // Do remove operation
}

function add($data)
{
    if (empty($data))
        return;

    // Do add operation
}

function process_input($input, $info)
{
    if (empty($input))
        remove($info);
    else
        add($input);
}

process_input($inputA, $infoA);
process_input($inputB, $infoB);
Simon Gillbee
Why do you have return; and only later the operations code?
MEM
@MEM - I prefer to assert the function contract at the top of a function and exit (return) quickly if the functino cannot continue. In the case of "remove", I assert (test with "if") that $data is not empty. If it *is* empty ("if (empty($data))") then I exit the function quickly. If this were production code rather than demo code I would probably log an error message or handle the error condition in some other proper manner.
Simon Gillbee
@MEM - This style of programming reduces the ammount of if{} else { if { if {}}} nested blocks which severely reduces readability. Basically, I have a block of "if (test_for_error) return" at the top of the function and then after I've passed all these contract assertions, then I do the actual work. But there's not need for that work to be in an else{} block because if we got here we already know that all the above assertions have passed.
Simon Gillbee
Fixed a bug in my code example... add() needs to validate that $data is _not_ empty (just like remove()). You can't add or remove data that's empty ;) That's part of the method-contract for these functions... they only operate on data that's not empty.
Simon Gillbee
@Simon Gillbee - thanks a lot for your explanations. I will study them and later on, place some questions if they arrive. Thanks a lot.
MEM
+1  A: 

I think unrolling the conditions and using else or else-if statements helps a lot, for readability and understanding of code. While your proposed answer cuts down on lines of code, it is going to be hard to understand and debug, especially if you pass it on to someone else.

Here's my take

// check if to update in DB or not

$value = db_adapter::get_var($sql);
if (empty($value))
{
  if (!empty($field_A) && !empty($field_B))
    add($field_A, $field_B);  

} else 
// you can roll the following block up into a function
{


  if (empty($field_A) && empty($field_B))
    remove($field_A, $field_B);
   else {
      if ($field_A != $value)
       update($field_A);  
     else if ($field_B != $value)
       update($field_B);
   }

}

Each of the else can be rolled up into a function; I didn't because I can't think of anything logical to call the block of code in else block (process-maybe-delete-update? determine-update-or-delete?)

Extrakun
Thanks a lot for your answer. I have end up changing the logic a little bit and I still believe it's not easy to understand for someone else, so I will consider remaking this, by doing a good use of your advices. Thanks a lot.
MEM