Skip to content

restrutured get_feature_names_out for readability - #8

Merged
dodoarg merged 3 commits into
dodoarg:issue397from
feature-engine:dtfromindex
Apr 4, 2022
Merged

restrutured get_feature_names_out for readability#8
dodoarg merged 3 commits into
dodoarg:issue397from
feature-engine:dtfromindex

Conversation

@solegalli

@solegalli solegalli commented Apr 4, 2022

Copy link
Copy Markdown

hi @dodoarg

I made some changes to docstrings and restructured the method to get the variable names to try and improve readability.

I'd like to know your opinion on having multiple returns per method.

I've read that having a single exit point in complex functions helps maintain the code in the long run, that is why I try to avoid breaking the flow with a return in the middle of a method. But I'd be keen to know your opinion or if you found resources that suggest otherwise.

Finally, we would need to add an example in the user_guide on how to extract variables from the dataframe index. Would you be able to add it?

We have a toy dataframe that you could use in this file: https://github.com/feature-engine/feature_engine/blob/main/docs/user_guide/timeseries/forecasting/LagFeatures.rst

I went ahead an edited the user guide. If you 'd be so kind to merge this PR, then we are done :)

If there is something you think is not right, please let me know.

Thanks a lot of your help!

@dodoarg

dodoarg commented Apr 4, 2022

Copy link
Copy Markdown
Owner

Seems like theres plenty of resources/debates about the "guard statement" vs "single exit point" paradigm for functions
e.g. https://www.anthonysteele.co.uk/TheSingleReturnLaw.html - this article seems quite thorough.
I think the take home message is there isn't a definitely bad practice in that sense, just pick what makes it for better readibility. Our transformer is a case of if we landed on a special case then do this otherwise do that which would probably warrant an early return had it been a side-effect or exception kind of special case; but in our case, it is not. In fact, it does pretty much exactly the same thing, just with a different syntax (.index instead of ["variable"]). Note that this is why I initially tried to not split the index case to begin with, keep it consistent with the rest, thus extend the functionality of the likes of _find_or_check_datetime_variables, _check_nans etc and add the if special case conditions there. I understand though that you didn't want those functions affected by this PR, so we're left with this little dilemma.

Since removing the early return here only causes the following code to be indented once, it's still quite readible. In more complex cases, potential multiple indentations would take away from readibility/maintainability rather than help with it I think.
To be honest I don't really know much about this topic as I am no CS myself but hope you got something from this little discourse anyway haha

@solegalli

Copy link
Copy Markdown
Author

Thank you @dodoarg that was very helpful :)

@dodoarg

dodoarg commented Apr 4, 2022

Copy link
Copy Markdown
Owner

oh I didn't notice you added the example yourself. Gj @solegalli

@dodoarg dodoarg closed this Apr 4, 2022
@dodoarg dodoarg reopened this Apr 4, 2022
@dodoarg
dodoarg merged commit 7184923 into dodoarg:issue397 Apr 4, 2022
@solegalli
solegalli deleted the dtfromindex branch April 6, 2022 08:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants