Re: Refactoring just to refactor?

sub index_menu {
    return [
        map  { anchor(textify($_) => {href => $_})  }
        sort { article_sort($a,$b)                  }
        grep { m/^[A-Z]./ && -f $_                  }
        file_list(shift() // '.')
    ];
}
[download]

Why I did it this way:

return [...];: Makes very obvious what is about to happen; we're returning an array ref.
Braces aligned both left and right: similar things should look similar. Why? Because if they suddenly look dissimilar it means I've made a mistake; missing closing brace, for example.
grep first operation to reduce the sort problem space. The problem space may be small today but it's just a good habit.
Call file_list() directly and pass its response into grep: Why store in an explicit intermediate variable if you're already chaining all the rest of the operations? This reduces the amount of state one has to be concerned with.
shift() // '.': This may be controversial; we're not unpacking args at the top of the sub. But unpacking is the first thing that happens, and it happens within the first statement. The advantage we get is that one doesn't have to remember yet another variable or be concerned with what it may have act upon it. The brevity reduces the amount of state one needs to keep in-head. But yes, this last one is probably controversial since it would probably violate a PBP rule.

Also m/^[A-Z]./ has the same effect here as m/^[A-Z].+/; either way you're matching any string that starts with A-Z and contains at least one more character. But the one without the quantifier doesn't have to read forward to the end of the string unnecessarily.

Finally, the abstraction may be wrong altogether. Here we're splitting hairs on how to format the code or what order sort and grep should come in, but overlooking the fact that anchor(textify(...), {href => ...}) looks a lot like HTML generation, and that you would probably be better served with a proper MVC framework and at very least with the use of a templating system. At minimum a template system to generate the HTML would be cleaner, and could likely be dropped into your code without other major refactoring.

Dave

Comment on Re: Refactoring just to refactor? Select or Download Code

Replies are listed 'Best First'.
Re^2: Refactoring just to refactor? by Lady_Aleena (Priest) on Jun 30, 2019 at 02:02 UTC
Wow! I didn't mean for this thread to rewrite that sub, it was just an example. 8) Doesn't putting `return` at the beginning of the subroutine make it harder to find? I always `return` or `print` at the end of subroutines. I will give the right curly brace alignment some thought. I am happy with the left curly brace alignment, but I am not sure about the right at this time. jwkrahn caught that I used `sort` before `grep` here, and I made the change already. At one time I called subroutines without assigning them to variables. For some reason, maybe readability, I stopped doing that and always assign subroutine output to their own variables where applicable. I have been writing for Perl 5.8.8 for so long now that I have not taken time to learn the goodies in later versions of Perl. I think `//` is in a later version, so I have not used it yet. Also, does it not lead to confusion if the parameter for the subroutine is not named? Will readers of the subroutine know where the parameter is being used? This is how I write subroutines. `sub name { my $parameter = shift; # or # my ($parameter1, parameter2) = @_; my $end_variable; # ... code to munge the parameter(s) and set end variable... return $end_variable; # or # print $end_variable; }` [download] I do not know why I added `.+` to the end of the regex. It doesn't even need the `.`, the `^[A-Z]` would do just as well. I may have been writing other regexen where I needed to search whole strings when I wrote that one. Many Monks have tried to get me to use various "template" modules. None of them have made any sense to me, even when I tried to use them, they have all lead me to pits of despair. I rolled my own modules for generating HTML for my site that make sense to me. The refactoring for my site for some template module would take months to rewrite. My home rolled template and HTML generation modules are loathed my most, if not all, Monks, but they are what I am comfortable using. I have used them both heavily. I am sure you have seen others banging their heads against that wall before. I try to not bring them up. If you look and then want to yell at me about them, you can open an issue on GitHub to keep it away from here. Too much drama surrounds them already. Thank you for the tips! *No matter how hysterical I get, my problems are not time sensitive. So, relax, have a cookie, and a very nice day!* Lady Aleena	[reply] [d/l] [select]
Re^3: Refactoring just to refactor? by davido (Cardinal) on Jun 30, 2019 at 03:35 UTC
I didn't intend to incite drama. :) My suggestions are just my own opinions, and I reserve the right to be outvoted by the prevailing trends, or vetoed by the owner of the code (you). I mentioned Perl Best Practices earlier. The book is great but not because it has 256 very good suggestions. No, it has 256 good morsels of food for thought. In the years since it was written some of the ideas took hold, others were shown to take us down a path toward madness, and some are hotly debated. I don't think my advice is any better. :) Dave	[reply]
Re^3: Refactoring just to refactor? by hippo (Bishop) on Jun 30, 2019 at 10:31 UTC
Doesn't putting return at the beginning of the subroutine make it harder to find? I always return or print at the end of subroutines. davido's subroutine has only one statement, which is the `return`. This means that the `return` is both at the beginning and at the end. Whichever it is perceived to be then becomes a purely philosophical matter for the reader. :-) I have been writing for Perl 5.8.8 for so long now that I have not taken time to learn the goodies in later versions of Perl. I think // is in a later version, so I have not used it yet. You are correct and the defined-or operator `//` was introduced in perl 5.10. It is a very useful shorthand and I use it all the time. From personal experience I would suggest that you do take time to learn the features introduced in newer perls. Maybe don't bother with anything marked experimental and IMHO stay well away from smartmatch and `given/when`. The rest is pretty good and if your only reason not to use them is lethargy/inertia then maybe this is the prod that will do it? Of course if you need to support 5.8.8 or earlier for whatever reason then that's fine and you just have to accept you are stuck with that for now. It is also worth pointing out that despite its truly excellent backwards compatibility, Perl (including the core modules) does sometimes remove obsolete features or introduce breaking changes, so it pays to keep an eye on those too. Also, does it not lead to confusion if the parameter for the subroutine is not named? Will readers of the subroutine know where the parameter is being used? This is why we have comments. If you think that another reader (or even yourself in 6 months) will have any confusion, simply comment the code to say "this takes one argument (see 'shift()') which is the directory of files to list and defaults to '.' if absent" and the job is done.	[reply] [d/l] [select]


Syntactic Confectionery Delight
	PerlMonks