edbennett / format_multiple_errors

MIT License
1 stars 0 forks source link

Long-ish function might need shortening #11

Closed chillenzer closed 1 year ago

chillenzer commented 1 year ago

The main function is not overly complicated but still rather long. You might want to have one further abstraction layer in between. For example, the if-else block could be a concept of its own, etc.?

edbennett commented 1 year ago

31 sloc (19 if you ignore line length limits) doesn't seem that long…

But maybe that's because other (not necessarily good) code where a single function call might be 10–15 lines has skewed my perspective.

Any suggestions for a name for the inner function?

chillenzer commented 1 year ago

The if-else block is def format_numbers(...) I'd say, so that in the main function you have

formatted_numbers = format_numbers(...)
edbennett commented 1 year ago

I think the issue I have with that approach is that I then have to pass almost every single local variable in to the inner function, including some that are technically redundant to each other (e.g. first_digit_index, significant_figures, and decimal_places).

The latter point could be avoided by taking lines 279–303 rather than just the if-else block, but then format_numbers ends up almost as long as the current function.

chillenzer commented 1 year ago

How about:

if _we_dont_need_decimals(first_digit_index, significant_figures, exponential):
    formatted_numbers = _map_errors(
        lambda value: str(int(round(value, decimal_places))),
        [value] + list(errors),
    )
else:
    formatted_numbers = [f"{value:.0{decimal_places}f}"] + _format_errors(
        errors, decimal_places, abbreviate
    )

with appropriate functions _we_dont_need_decimals and _format_errors. all_values is also made redundant. In total, that makes an 8 line reduction and improves readability in that it hides technical detail behind intelligible names that can be unwrapped upon request.

PS: And at this point you could even use a ternary (although I doubt that this improves readability):

formatted_numbers = (
    _map_errors(
        lambda value: str(int(round(value, decimal_places))),
        [value] + list(errors),
    )
    if we_dont_need_decimals(first_digit_index, significant_figures, exponential)
    else [f"{value:.0{decimal_places}f}"]
    + format_errors(errors, decimal_places, abbreviate)
)