Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CF function CF_CFDP_IsSender(transaction_t *ti) is odd one out because it uses ti #30

Closed
jphickey opened this issue Nov 23, 2021 · 3 comments · Fixed by #326
Closed

Comments

@jphickey
Copy link
Contributor

This issue was imported from the GSFC issue tracking system

Imported from: [GSFCCFS-1728] CF function CF_CFDP_IsSender(transaction_t *ti) is odd one out because it uses ti
Originally submitted by: Gibson, Alan S. (GSFC-5870) on Tue Sep 14 10:58:59 2021

Original Description:
Every other function in cfdp.c that uses a transaction_t* as an argument names it 't', but CF_CFDP_IsSender uses ti.

@astrogeco
Copy link
Contributor

CF/fsw/src/cf_cfdp.c

Lines 142 to 161 in 96cdf30

static inline CF_CFDP_Class_t CF_CFDP_GetClass(const CF_Transaction_t *ti)
{
CF_Assert(ti->flags.com.q_index != CF_QueueIdx_FREE);
return !!((ti->state == CF_TxnState_S2) || (ti->state == CF_TxnState_R2));
}
/*----------------------------------------------------------------
*
* Function: CF_CFDP_IsSender
*
* Internal helper routine only, not part of API.
*
*-----------------------------------------------------------------*/
static inline int CF_CFDP_IsSender(CF_Transaction_t *ti)
{
CF_Assert(ti->flags.com.q_index != CF_QueueIdx_FREE);
/* the state could actually be CF_TxnState_IDLE, which is still not a sender. This would
* be an unused transaction in the RX (CF_CFDP_ReceiveMessage) path. */
return !!((ti->state == CF_TxnState_S1) || (ti->state == CF_TxnState_S2));
}

@skliper
Copy link
Contributor

skliper commented Apr 25, 2022

While I do appreciate not wasting characters... neither ti or t are all that descriptive/helpful in terms of variable names. Maybe transaction or txn or similar? I'd consider either of these options more "readable".

@thnkslprpt
Copy link
Contributor

While I do appreciate not wasting characters... neither ti or t are all that descriptive/helpful in terms of variable names. Maybe transaction or txn or similar? I'd consider either of these options more "readable".

I would tend to agree with you Jake.
However, there are several hundred uses of all 3 options (t, txn & transaction) in the code base. So for now, it might be best to just correct this particular instance.
For CF_Transaction_t objects specifically, they practically all use ‘t’, so we can align these 2 aberrations to the majority, like Alan Gibson suggested.
Perhaps future work could focus on improving the argument names in general, to make them more readable.

thnkslprpt added a commit to thnkslprpt/CF that referenced this issue Sep 20, 2022
dzbaker added a commit that referenced this issue Oct 3, 2022
Fix #30, Correct 2 aberrant instances of CF_Transaction_t argument name
@dmknutsen dmknutsen added this to the Draco milestone Feb 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment