Re: [PATCH v5 2/4] Add support for SAP protocol

On Thu, Mar 17, 2011 at 10:50 AM,  [off-list ref] wrote:

Hi Angus.H,

quoted

A lot of code could be refactored.
Quite a few repeatable (logically) blocks in every
"...._req()" and "..._rsp()" handlers.

Indeed, but those blocks are not equal in all cases. Thus, I prefere to split it up into seperate functions instead of having one big function with switch-case statement.
It's not like we can replace those blocks with one generic function without switch-case.  I consider this as slightly more readable. However, I'm not strongly opposed to change it one function.

Waldek

Hi
I just wanted to say that refactoring of typical req() here could be
seen as 3 or 4 internal prcedure calls,
[ refactoring here == extracting common/repeatable code to new routine ]

typical req example:
static void power_sim_off_req(struct sap_connection *conn)
 {
-       DBG("SAP_SIM_OFF_REQUEST");
+       DBG("conn %p state %d", conn, conn->state);
+
+       if (conn->state != SAP_STATE_CONNECTED)
+               goto error_rsp;
+
+       if (!is_power_sim_off_req_allowed(conn->processing_req))
+               goto error_rsp;
+
+       conn->processing_req = SAP_POWER_SIM_OFF_REQ;
+       sap_power_sim_off_req(conn);
+
+       return;
+
+error_rsp:
+       error("Processing error (state %d pr 0x%02x)", conn->state,
+                                               conn->processing_req);
+       sap_error_rsp(conn);
 }

could be represented as:
{
validate_req(conn, VALIDATION_RULE)
assign_req(conn,REQ)
send_req(conn, handlers[REQ])  ( where handlers is an array of req/rsp
handlers to sap driver )
error_handling:
}


it's just an example, more readable/maintainable.
/AH