[HN Gopher] Good refactoring vs. bad refactoring
___________________________________________________________________
Good refactoring vs. bad refactoring
Author : steve8708
Score : 155 points
Date : 2024-08-19 23:01 UTC (23 hours ago)
(HTM) web link (www.builder.io)
(TXT) w3m dump (www.builder.io)
| michaelteter wrote:
| The first example complained about the refactor appealing to
| functional thinkers (implying that it would be difficult to grok
| by the existing devs), but then the "improved" version is
| virtually the same save for the (unnecessary?) use of Ramda in
| the first.
|
| And while many devs are resistant to try functional ways, this
| first example reads so much better than the original code that I
| find it impossible to believe that some prefer the imperative
| loop/conditional nesting approach.
| happytoexplain wrote:
| Are you saying you find it hard to believe that some prefer the
| first "Before" example over the first "Bad refactor" example?
| p2501 wrote:
| Aesthetic aside, I am under the impression that people start
| programming, by and large, with imperative for/if style => so
| the imperative style is readable by more people. Even for more
| experienced programmers, reading imperative probably cost less
| energy, since it is more internalised?
|
| Futhermore, in JS, the functionnal style is less performant
| (nearly twice on my machine, i assume because it do less
| useless memory allocations)
|
| So, same functionnality, readable by more people, more
| performant? The imperative example seems like the better code.
| joshka wrote:
| I, a Datapoint of 1, find the functional style to generally
| express the ideas of what's happening to be significantly
| easier to grok. Particularly if intermediate variables and
| conversion constructors are introduced rather than relying on
| full chains. E.g.: function
| processUsers(users: User[]): FormattedUser[] { let
| adults = users.filter(user => user.age >= 18);
| return adults.map(user => FormattedUser.new_adult(user));
| }
|
| On the performance tip, what scale are we talking? Is it
| relevant to the target system? Obviously the example is
| synthetic, so we can't know that, but does it seem like this
| would have a runtime performance that is meaningful in some
| sort of reasonable use case?
| zahlman wrote:
| > I am under the impression that people start programming, by
| and large, with imperative for/if style => so the imperative
| style is readable by more people.
|
| IMO, this is a simple consequence of technology moving faster
| than society. There are still instructors out there who
| learned to program in an environment where the go-to options
| for imperative programming were C and FORTRAN; the go-to
| options for other paradigms (if you'd even heard of other
| paradigms) were things like Lisp, Haskell and Smalltalk; and
| CPU speeds were measured in MHz on machines that you had to
| share with other people. Of course you're going to get more
| experience with imperative programming; and familiarity
| breeds comprehension.
|
| But really, I believe strongly that the functional style -
| properly factored - is far more intuitive. The mechanics of
| initializing some output collection to a default state (and,
| perhaps, the realization that zero isn't a special case),
| keeping track of a position in an input collection, and
| repeatedly appending to an output, are just not that
| interesting. Sure, coming up with those steps could be a
| useful problem-solving exercise for brand-new programmers.
| But there are countless other options - and IMX, problem-
| solving is fiendishly hard to teach anyway. What ends up
| happening all the time is that you _think_ you 've taught a
| skill, but really the student has memorized a pattern and
| will slavishly attempt to apply it as much as possible going
| forward.
|
| > Futhermore, in JS, the functionnal style is less performant
| (nearly twice on my machine, i assume because it do less
| useless memory allocations)
|
| Sure. Meanwhile in Python: $ python -m
| timeit "x = []" "for i in 'example sequence':" "
| x.append(i)" 500000 loops, best of 5: 796 nsec per
| loop $ python -m timeit "x = [i for i in 'example
| sequence']" 500000 loops, best of 5: 529 nsec per
| loop
|
| ... But, of course: $ python -m timeit "x =
| list('example sequence')" 2000000 loops, best of 5:
| 198 nsec per loop
|
| Horses for courses.
| vasco wrote:
| People think imperatively though. If I think of visiting my
| friend, grabbing gas on the way back, the way I'll
| visualize the steps is not functional.
| The_Colonel wrote:
| That's quite debatable.
|
| In this case, you first declare the end-goal - visit a
| friend and have full gas tank, with the actual steps to
| achieve them being much less important and often left to
| be defined at a later point (e.g. which particular gas
| station, which particular pump etc.). This corresponds
| more to functional thinking.
|
| An imperative thinking would correspond more to "I will
| sit in the car, start the engine, ride on highway, stop
| at address X, converse with Y, leave 2 hours later, stop
| at gas station X" - in this case the imperative steps are
| the dominant pattern while the actual intent (visit a
| friend) is only implicit.
| vasco wrote:
| Your second part is how I think and how I think most
| people think. That's exactly what I meant.
| The_Colonel wrote:
| So when you arrange the visit with your friend two weeks
| in advance, you first think about sitting in the car,
| driving out of the garage, getting on the highway,
| turning on the radio, parking the car, ringing the bell
| and this other myriad of actions, and the actual talking
| with the friend is just one of the actions, with no
| prominence over the others?
|
| I certainly don't think like that. My main goal is to
| visit a friend. The transportation is subordinate, it's
| only a mean to the goal, an implementation detail which I
| don't care about much. I might even take a train instead
| of driving the car, or even ride a bike, if I feel like
| it and the weather is nice on the day of the visit.
|
| Now reflecting on this, I think such focus on the process
| (as opposed to focus on the goal), exact imperative
| order, not being able to alter the plan even if the
| change is meaningless in relation to the goal, is a sign
| of autism. But I don't believe most people think like
| that.
| vasco wrote:
| You're the one that added all those conditions about
| exactness, about needing to replay every step (even this
| is a problem because steps are fractal), or not being
| able to change the plan. I can think imperatively and
| still do those :)
| whilenot-dev wrote:
| I disagree. People are sometimes just forced to think
| imperatively when dealing with computers, but I usually
| think declarative when I design my programs.
|
| Say if I want to filter a sequence of users and omit
| users below the age of 18, I'll construct my predicate (a
| "what"), and want to apply that predicate to create a new
| sequence of user (another "what").
|
| I really don't want to tell a computer how to process a
| list every single time. I don't care about creating an
| index first and checking the length of my list in order
| to keep track that I process each user in my list
| sequentially, and don't forget that important "i++". All
| I want at that moment is to think in streams, and this
| stream processing can happen in parallel just as well for
| all I care.
|
| But I also do think Python, Haskell etc. are the most
| expressive here with list comprehensions. It can't get
| more concise than this IMHO: users_adult
| = [ user for user in users if
| user.age >= 18 ]
| yxhuvud wrote:
| While I think of things in a defined order, I also think
| in sets. If I grab a bunch of peanuts, I don't visualize
| grabbing every single peanut one by one, I visualize
| getting a bunch at the same time.
| vasco wrote:
| If you grab a bunch of peanuts you're probably thinking
| in "hand fulls", but the fact that the world and actions
| we take are fractal in the way we can analyze them
| doesn't prove one or the other.
| harperlee wrote:
| I dont know a lot about the brain but I do know that
| people that research modeling brains have used models in
| which there are two types of things to think: declarative
| things AND procedural things. See SOAR and ACT-R.
|
| The caveat here is to which extent computers have tinted
| their models of a brain, but they are professional
| cognitive researchers so I'd give them the benefit of the
| doubt :)
| kazinator wrote:
| [delayed]
| The_Colonel wrote:
| > Even for more experienced programmers, reading imperative
| probably cost less energy, since it is more internalised?
|
| I disagree. For-cycles are usually more difficult to reason
| about, because they're more general and powerful. If I see
| "for (...", I only know that the subsequent code will
| iterate, but the actual meaning has to be inferred from the
| content.
|
| Meanwhile, a .map() or .filter() already give me hints - the
| lambda will transform the values (map), will filter values
| (filter), these hints make it easier to understand the logic
| because you already understand what the lambda is meant to
| do.
|
| Other benefits stem from idiomatic usage of these constructs.
| It's normal to mix different things into one for-cycle - e.g.
| filtering, transformation, adding to the resulting collection
| are all in the same block of code. In the functional
| approach, different "stages" of the processing are isolated
| into smaller chunks which are easier to reason about.
|
| Another thing is that immutable data structures are quite
| natural with functional programming and they are a major
| simplification when thinking about the program state. A given
| variable has only one immutable state (in the current
| execution) as opposed to being changed 1000 times over the
| course of the for-loop.
| math_dandy wrote:
| No need to fear mutable local state. Shared state is where
| immutable data structures really shine.
| persnickety wrote:
| > If I see "for (...", I only know that the subsequent code
| will iterate
|
| And then someone slaps do {} while(0) in a macro.
| skybrian wrote:
| (Raises hand.) I prefer the for loop. Pushing items to an array
| is idiomatic Javascript for creating an array. An if statement
| is an idiomatic way to do it conditionally. It's also easier to
| debug.
|
| The map and filter methods are nice too, but they're for one-
| liners.
| nine_k wrote:
| Writing assembly was the idiomatic way of programming before
| Fortran and human-readable languages came.
|
| Writing with goto was the idiomatic way before Algol and
| structural programming came.
|
| Having only a handful of scalar types was the idiomatic way
| until structural data types came (and later objects).
|
| Writing programs as fragments of text that get glued together
| somehow at build time was the idiomatic way until module
| systems came. (C and partly C++ continue to live in 1970s
| though.)
|
| Callback hell was the idiomatic way to do async until Futures
| / Promises and appropriate language support came.
|
| Sometimes it's time to move on. Writing idiomatic ES5 may
| feel fun for some, but it may not be the best way to reach
| high productivity and correctness of the result.
| skybrian wrote:
| Making analogies like this doesn't prove anything, they're
| just suggestive. All I'm getting out of this is that you
| think for loops are old-fashioned.
| Jean-Papoulos wrote:
| That's because they are. Functional code is more
| readable. And if you look back, basically all advances in
| programming languages have been about "making stuff more
| readable". Thus, for loops (for this usage) are "old".
| skybrian wrote:
| You say it's more readable and I disagree with that!
|
| Is this just fashion? Is there a way to settle it other
| than "I like it better?"
| Joker_vD wrote:
| > Functional code is more readable.
|
| There is no way that name:
| R.pipe(R.prop('name'), R.toUpper), age:
| R.prop('age'), isAdult: R.always(true)
|
| is more readable than name:
| user.name.toUpperCase(), age: user.age,
| isAdult: true
| nine_k wrote:
| Certainly so! The first example is needlessly contrived.
|
| But instead of for (const i=0; i <
| data.length; i++) { new_data[i] =
| old_data[i].toUpperCase(); }
|
| you can write const new_data =
| old_data.map((x) => x.toUpperCase());
|
| I think it's both more clear and less error-prone.
| nuancebydefault wrote:
| The second code does not compile and introduces a new
| dependency.
| bobthepanda wrote:
| I will say in 2024 i feel like for/of or forEach would at
| least let you avoid the boilerplate of an index.
| skybrian wrote:
| Yes, I agree.
| asddubs wrote:
| I also prefer the loop, I would negate the condition and use
| continue, but otherwise leave it unchanged. I don't have a
| problem with the functional version but it doesn't scan as
| well to me.
| mabster wrote:
| I don't code in JS very often, so there's that.
|
| Both sets of code were fine, but I understood the loop
| variant instantly, while it took me a bit longer with the FP
| code.
|
| As a side note: The only real JS I did was optimising some
| performance critical code, and I did have to refactor a
| number of FP chains back to loops. This was because the FP
| way keeps constructing a new list for each step which was
| slow.
| diatone wrote:
| > I find it impossible to believe that some prefer the
| imperative loop/conditional nesting approach.
|
| Yeah, there's your problem. This is in fact possible!
| jamil7 wrote:
| I've haven't written JS in a long time - are engines like V8
| smart enough to roll the filter and map into a single loop?
| Otherwise wouldn't a reduce be more efficient there?
| nevon wrote:
| It's not a matter of being smart enough. Since JavaScript is
| interpreted, the optimization happens at runtime. If the code
| is executed once and the number of items in the array is
| small, then it will take more time for the compiler to
| optimize the code than to naively execute it. Most code falls
| into this category.
|
| As for whether or not it's possible at all to combine a map
| and filter into a single loop I guess depends on whether the
| first operation can have side effects that affect the second
| operation or the collection that is being iterated over. I
| don't know the answer, but I would be surprised if there
| wasn't some hard to detect corner case that prohibits this
| kind of optimization.
| ggm wrote:
| Good refactoring respects the idioms of the language and the
| culture of the organisation. Change to new methodology is
| thoughtful and probably slow, except when a revolution happens
| but then, its still respectful to the new culture.
|
| Bad refactoring is elitist, "you won't understand this" commented
| and the owner walks with nobody left behind who understands it.
|
| That the examples deprecated FP and preferred an idiom natural to
| Java(script) only speaks to the principle. I can imagine a quant-
| shop in a bank re-factoring to pure Haskell, out of somthing
| else, and being entirely happy that its FP respecting.
|
| So the surface "FP patterns are bad" is a bit light-on. The point
| was, nobody else in that specific group could really be expected
| to maintain them unless they were part of the culture.
|
| "If you unroll loops a la duff's device, you should explain why
| you're doing it" would be another example.
| boxed wrote:
| The dig against FP is weird since the "good refactor" also uses
| FP, just a built in one in JS. Which I agree is better, but
| mostly by being built in and idiomatic, it's still exactly as
| functional.
| ggm wrote:
| "I don't like this style of FP coding" ok: if you run the
| group and own the codebase you can enforce that. So good
| refactoring is style guide enforcement.
|
| I think I over-read his dislike of FP. really the complaint
| is "why did you introduce a new dependency" which I am
| totally fine with, as a complaint. Thats not cool.
|
| Many of his examples kind-of bury the lede. If he had tried
| to write an abstract up front, I think "dont code FP"
| wouldn't have been in it. "use the methods in the language
| like .filter and .map" might be.
| watwut wrote:
| I think that was the point - the library added nothing, the
| same thing could be done with pure javascript.
| boxed wrote:
| If that was the point, then that paragraph needs to be
| rewritten badly.
| nuancebydefault wrote:
| In fact the paragraph needs _good_ refactoring
| carlmr wrote:
| On point. Even then mentioning it used filter and map. But
| the bad refactor also uses filter and map. It's the exact
| same change of programming paradigm.
|
| Given the text, I would have expected some minor refactor
| with range-based for loops (are these a thing? My JS is
| rusty). Where you get the advantage of map (no off-by-one
| indexing errors) without changing the programming paradigm.
| bobthepanda wrote:
| you don't even need range loops.
|
| you have forEach: list.forEach((item,
| index) => doSomething());
|
| you also have for/of, though that doesn't have an index if
| you need that for (const item of list) {
| doSomething() };
|
| which just obviates the need for index incrementing in the
| most common case, where you are incrementing by one until
| you hit the end of the list.
| nailer wrote:
| Oh god the 'object oriented' refactor. I wish everyone who had OO
| thrust upon them in the early 2000s received some explicit
| communication that what they were taught is essentially a hoax
| and bears no resemblance to Alan Kay's original intention
| spc476 wrote:
| Some of that is on Alan Kay because it took him twenty years to
| realize people couldn't read his mind on the proper definition
| of "object oriented."
| sevnin wrote:
| To be fair OOP of today is much more similar to Simula then to
| Small Talk, reading the wikipedia I can see almost 1:1 mapping
| including the modeling philosophy and all that, people mostly
| yoinked the name from Alan Kay.
| vips7L wrote:
| That OO refactor isn't actual OO. The tell tale sign is that it
| is named by what it does rather than what it is (verb vs noun)
| and the -or ending in the name [0]. It's just a function
| masquerading as a class.
|
| The better refactor to introduce OO concepts would have been to
| introduce an isAdult function on the user class and maybe a
| formatted function. This + the functional refactor probably would
| have made for the best code. return
| users.filter(u => u.isAdult()) .map(u => format(u)); //
| maybe u.formatted()
|
| [0] https://www.yegor256.com/2015/03/09/objects-end-with-er.html
| dominicrose wrote:
| What about a pure function that can take anything that has an
| age as input? Well obviously that wouldn't work for a cat but
| it's just an example. It requires typescript and I'm not sure
| how to name the file it would go in, but I think it's
| interesting to consider this duck-typing style.
| function isAdult({age}: {age: int}) { return age >=
| 18 }
|
| ps: I replaced const by function because I don't like the IDE
| saying I can't use something before it is defined. It's not a
| bug it's an early feature of javascript to be able to use a
| function before it is defined. Code is just easier to read when
| putting the caller above the callee.
| vips7L wrote:
| That wouldn't be object oriented. In OO you tend to want to
| ask an object about itself, Yegor talks a bit about this in
| his book Elegant Objects.
|
| What you are proposing is just functions or data-oriented
| programming; which is fine if that's your thing, but I'd be
| weary because of the reasons you outline above. Can a book be
| an adult? What about a tv show? Or recipe from the 9th
| century? isAdult really only applies to users and really
| belongs on that object.
| Frieren wrote:
| > u.isAdult()
|
| Being adult is not a property of the user but of the
| jurisdiction that the user is in. In some places or some
| purposes it is 18 but it could be, e.g., 21 for other purposes.
|
| If you software is not going to just run on the USA it is not a
| good idea to implement isAdult in the user but in a separated
| entity that contains data about purpose and location.
| vips7L wrote:
| With proper OO you could still implement that on the user
| object. boolean isAdult() {
| return this.age >= this.location.ageOfAdulthood();
| // or this.location.isAdult(this.age); pick your poison!
| }
|
| ...anyway it's just an example of how to introduce OO
| concepts. As everything in programming _it depends_
| Terr_ wrote:
| Just having some fun with bikeshedding here: Yeah, that
| could work but IMO in a big/international system the
| responsibility should ideally live elsewhere, since:
|
| * You may need to determine adulthood for a _different_
| jurisdiction than where the person currently resides. Their
| citizenship may be elsewhere, or you may be running a
| report that expects "adulthood" to be by some other
| region's standards, etc.
|
| * Sometimes the underlying _kind_ of adult-need wanted is
| slightly different, like for consuming alcohol or voting.
|
| * There may be a weird country or province has laws that
| need additional factors, like some odd place where it's a
| different age-cutoff for men and women.
| vips7L wrote:
| Yes this is just extreme bike-shedding at this point. But
| none of this is impossible with more OO principles, like
| interfaces: class User { //
| Convenience function to check if the user is an adult in
| their current location boolean isAdult() {
| return this.location.isAdult(this); }
| boolean isOfDrinkingAge() { return
| this.location.isOfDrinkingAge(this); }
| } interface Location { boolean
| isAdult(User u); boolean isOfDrinkingAge(User
| u); } class WeirdLawsLocation
| implements Location { boolean isAdult(User u)
| { return switch (u.gender()) {
| case MALE -> u.age() >= 16; case
| FEMALE -> u.age() >= 18; }
| } boolean isOfDrinkingAge(User u) {
| return u.age() >= 21 } }
|
| In the hypothetical that you want to check somewhere the
| user is not currently: class
| SwedenLocation implements Location { boolean
| isAdult(User u) { return u.age() >= 18;
| } boolean isOfDrinkinAge(User u) {
| return u.age() >= 18; } } var
| sweden = new SwedenLocation();
| sweden.isOfDrinkingAge(user);
| vips7L wrote:
| On a side note, this discussion really made me realize
| how useful concise method bodies would be in Java:
| https://openjdk.org/jeps/8209434
| Terr_ wrote:
| That feels like unnecessary levels of indirection to
| provide a method that shouldn't be on the User anyway.
| j = Jurisdiction.fromUserLocation(user);
| j.isOfDrinkingAge(user);
| vips7L wrote:
| > that shouldn't be on the User anyway.
|
| That's just your opinion. It's ok to provide convenience
| functions. I see no difference between the amount of
| indirection in our implementations, except mine is in the
| more natural place and you don't have to know how to get
| a location or jurisdiction to answer the question: "is
| this user an adult?". Knowing that it uses a location or
| jurisdiction is an implementation detail that you
| shouldn't couple yourself to.
|
| Cheers mate, I think I'm done moving goal posts for this
| conversation :)
| piotrkaminski wrote:
| What I get out of this is that even for teams that prioritize
| trust and velocity and eschew the use of pre-commit code reviews,
| there's a strong argument to be made for putting all new hires on
| an approval-required list for the first few months!
| jv_be wrote:
| A good refactor does not change behaviour, I would like the
| author to start with that point. Take many more much smaller
| steps while doing so. Not touching a piece of code in the first 6
| to 9 months is something I don't really agree with. Breaking
| complex methods up by extracting variables and methods can really
| help learning the code, whilst not breaking it. If you are
| worried about consistency, just pair up of practice ensemble
| programming instead of asynchronous code reviews. Leaving a new
| dev alone with the code and give them feedback about the things
| they did wrong after they went through everything is just not a
| great way to treat people in your team.
| kolme wrote:
| A refactor does not change behavior, _period_. By definition.
|
| If something changed, it's not a refactor. It's a change.
|
| Like in the example where the caching was removed: NOT a
| refactor.
|
| Ore where the timeouts for the requests were changed: NOT a
| refactor.
|
| The definition of refactor: change the structure of the code
| without altering the behavior.
|
| It's like saying a crash is a bad landing.
| sevnin wrote:
| "By definition" proceeds to define the word in a way that
| most people won't agree with. Okay mate. Your definition is
| ass. The person that wrote the article doesn't agree with
| you, I don't agree with you. I will still use the word
| refactor when I talk about simplifying the application such
| that code becomes simpler through simplifying and improving
| the design.
| gcarvalho wrote:
| I think it's widely accepted that refactors should not
| change behavior. That's my experience, at least.
| sevnin wrote:
| Huh, I guess that's true, even on wikipedia. I will have
| to stop using the word then. Though I'm pretty sure most
| people use it in day-to-day with much more abandon.
| FooBarBizBazz wrote:
| The metaphor, I think, is that your code is a
| mathematical function, and, to be even more specific and
| "toy example" about it, let's say it's a polynomial. If
| the old code was
|
| x^2 + x z + y x + y z
|
| then you notice that you can express the same polynomial
| as:
|
| (x + y)*(x + z)
|
| It's still the same polynomial, but you "separated
| concerns", turning it into a product of two simpler
| factors.
|
| Similar ideas apply to sets of tuples. Perhaps you were
| given
|
| {(1, 1), (1, 4), (2, 1), (2, 4), (3, 1), (3, 4)}
|
| and you notice that this can be expressed more simply as
| the Cartesian product:
|
| {1, 2, 3} x {1, 4}
|
| Again, a literal factoring. You can imagine how
| variations of this idea would apply to database tables
| and data structures.
|
| That's where I think the word "refactor" comes from.
| sevnin wrote:
| My man, thanks for the explanation but I understand the
| concept I just didn't agree on definition.
| FooBarBizBazz wrote:
| Sure. Whenever you post on a forum it's half for anybody
| else reading, right? I just thought it was interesting to
| consider the underlying metaphor; maybe people don't
| think about it. Metaphors, connotations, etymologies -- I
| find these interesting.
| sevnin wrote:
| Sure, I agree. You could even respond to me fully
| selfishly - treat me as a stepping stone just to form
| your argument, nothing wrong with that.
| nuancebydefault wrote:
| If you change the polynomial, you change the order of
| calculations and hence change behaviour. You might get
| overflows in case of integers or different precision in
| case of floats.
|
| Nice to learn where the word originates from. Often the
| meaning of words change over time. E.g. today no horses
| need involved in bootstrapping.
| nxicvyvy wrote:
| That's the definition most people use.
|
| https://www.google.com/search?q=refactoring%20definition
| whilenot-dev wrote:
| > A refactor does not change behavior, period. By definition.
|
| Refactoring does not change the _external_ behavior. If you
| can 't change the internal behavior, then you can't reduce
| complexity.
|
| So with that in mind...
|
| - changing implicit caching => refactoring
|
| - changing implicit timeouts => refactoring
|
| - changing explicit caching => more than refactoring
|
| - changing explicit timeouts => more than refactoring
|
| Because the word _external_ implies conceptual boundaries, I
| would personally also distinguish refactoring by levels:
|
| - system design
|
| - service design
|
| - program design
|
| - component design
|
| - module design
|
| - function design
|
| ...where this blog post only talks about the latter two.
| nuancebydefault wrote:
| By _your_ definition. If you apply it, a lot of effort is
| needed for little gain. The best refactors i've seen improved
| behavior while making code more concise.
| azangru wrote:
| The first example of a good refactor is a meh refactor at best,
| and possibly a bad refactor. Array methods such as map or filter
| are not "more conventional" in javascript; they are "as
| conventional" as for-loops, and arguably less "conventional",
| given how for-loops have been around since the introduction of
| the language. They are also inevitably more expensive than for-
| loops (every cycle creates an anonymous function; a map followed
| by a filter means another iteration of the array). The original
| example was fine; there was no need to "refactor" it.
| knallfrosch wrote:
| Disagree on this. filter and map are much more readable and
| especially extensible than result-arrays. Plus it eliminates
| out-of-bonds indexing.
|
| See the variable name. It's forced to be 'result' so that it's
| consistent with the result-array style. Therefore it lacks a
| descriptive name.
|
| For the functional methods, you can easily assign the
| filter(age > 18) result to an intermediate variable like
| adultUsers to make the code even more descriptive. Useful when
| you have more steps. With the result-array approach, you'd have
| to repeat the looping code or bury the description deep in the
| loop itself and so you usually avoid that.
| asp_hornet wrote:
| > much more readable
|
| That's down to preference.
|
| Doesnt both filter and map copy the array increasing gc
| pressure?
| kolme wrote:
| > _every cycle creates an anonymous function_
|
| No, that's not how it works. The function is evaluated once
| before the call and passed as an argument, then internally
| reused.
|
| Also, you're microptimizing. Prioritizing _supposed_
| performance over readability.
|
| And yes, for-loops and mutable structures are more error prone
| than map-filter-reduce. The original is OK but could be better.
| azangru wrote:
| > No, that's not how it works. The function is evaluated once
| before the call and passed as an argument, then internally
| reused.
|
| Yes, sorry; you are right of course.
| mariopt wrote:
| Words can not express my hate for this kind of articles.
|
| Imagine working on a legacy codebase where the PM holds the dogma
| of refactoring being a bad thing and expecting you to do it
| wrong, even micro managing your PRs.
|
| Most often than not, I do see projects suffering and coders
| actually resigning due to a lack of internal discussing about
| best practices, having space/time to test potential solutions,
| having Lead devs who resemble dictators quite well.
|
| Let me guess, some PM wrote this article and they just want you
| to push the product asap by applying pressure and not allowing
| you ever to refactor. This is just a casual day in software
| development. I'm not surprised anymore when most web apps have
| silly bugs for years because it's gonna be a Jira ticket and a
| big discussion about..... one evil thing called refactor.
|
| Several years ago I rewrote a full SaaS in about 3 months, it
| took another team 12 months with 5 devs. Guess which version made
| the investors happy, mine.
|
| Bad refactoring is just a product of poor engineering culture.
| exe34 wrote:
| more people just means more time spent trying to coordinate and
| in the limit, you spend all the time talking and none coding.
| remus wrote:
| > Imagine working on a legacy codebase where the PM holds the
| dogma of refactoring being a bad thing and expecting you to do
| it wrong, even micro managing your PRs.
|
| I don't think the article said that anywhere? It was just a
| list of some common things that can go wrong when refactoring,
| along with some examples.
| Jean-Papoulos wrote:
| I feel this article is honestly disingenuous. One of the "common
| pitfalls of refactoring" mentioned is "Not understanding the code
| before refactoring". Well yeah ? The same would apply to doing
| anything with the code. The following one is "Understand the
| business context" (note that the author has already departed from
| the pattern of listing "common pitfalls" to just write whatever
| he feels like. Or he just published his first draft).
|
| Not a very qualitative article.
| MattHeard wrote:
| I got as far as here:
|
| > If you need to introduce a new pattern, consider refactoring
| the entire codebase to use this new pattern, rather than creating
| one-off inconsistencies.
|
| Putting aside the mis-application of "pattern" (which _should_ be
| used with respect to a specific design problem, per the Gang of
| Four), this suggestion to "refactor the entire codebase" is
| impractical and calcifying.
|
| Consistency increases legibility, but only to a certain point. If
| the problems that your software is trying to solve drift (as they
| always do with successful software), the solutions that your
| software employs must also shift accordingly. You can do this
| gradually, experimenting with possible new solutions and
| implementations and patterns, as you get a feel for the new
| problems you are solving, or you can insist on "consistency" and
| then find yourself having to perform a Big Rewrite under
| unrealistic pressure.
| Diggsey wrote:
| > Putting aside the mis-application of "pattern" (which
| _should_ be used with respect to a specific design problem, per
| the Gang of Four)
|
| This is not in any way a mis-application of the word "pattern".
| There is no exhaustive list of all design patterns. A design
| pattern is any pattern that is used throughout a codebase in
| order to leverage an existing concept rather than invent a new
| one each time. The pattern need not exist outside the codebase.
|
| > Consistency increases legibility, but only to a certain
| point.
|
| It's the opposite: inconsistency decreases legibility, and
| there is no limit. More inconsistency is always worse, but it
| may be traded off in small amounts for other benefits.
|
| Take your example of experimenting with new solutions: in this
| case you are introducing inconsistency in exchange for learning
| whether the new solution is an improvement. However, once you
| have learned that, the inconsistency is simply debt. A decision
| should be made to either apply the solution everywhere, or roll
| back to the original solution. This is precisely the point the
| author is making by saying " _consider_ refactoring the entire
| codebase to use this new pattern ".
|
| This refactoring or removal doesn't need to happen overnight,
| but it needs to happen before too many more experiments are
| committed to. Instead what often happens is that this debt is
| simply never paid, and the codebase fills with a history of
| failed experiments, and the entire thing becomes an unworkable
| mess.
| Y_Y wrote:
| > "pattern" (which _should_ be used with respect to a specific
| design problem, per the Gang of Four)
|
| Why is that true? Particularly if you're not an OOP
| user/believer. It's not like "pattern" is some obscure term of
| art.
| _a_a_a_ wrote:
| For consistency, the same design problems should have the
| same design solutions a.k.a. pattern. If you don't value
| consistency, feel free to take a different approach every
| time. That will confuse your users. I used to work with a
| guy, the simple problem of reading a CSV was done using a
| library, problem sorted. Out of sheer excitement he then
| rewrote it was with combinator parsers, then as some
| astronaut architect functional monstrosity, so complex no one
| else could comprehend it.
|
| This is how _not_ to do it - for same problem, use same
| solution. I admit that 's an extreme case but it's also a
| real one and illustrates the issue well.
|
| (also patterns are not specific to OO, nor is OO incompatible
| with a functional style)
| watwut wrote:
| It is not just about legibility. Developers are trying to
| repeat patterns they see around. If you do not refactor
| previous places, they will reproduce the outdated pattern.
| Plus, it makes code reviews frustrating.
| kolme wrote:
| There was a great refactoring chance in the example where the
| cache was removed.
|
| That is, extracting the caching logic from the API call logic.
|
| Caching could have been a more generic function that wraps the
| API call function. That way each function does exactly one thing,
| and the caching bit can get reused somewhere else.
|
| Instead, this weird advice was given: changing behavior is bad
| refactoring. Which is weird because that's not even what we call
| refactoring.
|
| Edit: removed unnecessary negativity.
| pif wrote:
| Concerning the first example: whoever thought that passing
| property names as string can be better than any other style
| should have his coding licence revoked!
| pif wrote:
| Second example: doesn't the bad refactor also _modify_ the list
| of users?
| creesch wrote:
| I am not sure if I agree with the article, however I do agree
| that not every time code actually needs to be formatted.
|
| > More than a few of them have come in with a strong belief that
| our code needed heavy refactoring.
|
| The code might, but a blind spot for many developers is that just
| because they are not familiar with the code doesn't mean it is
| bad code. A lot of refactoring arguments I have seen over the
| years do boil down to "well, I just don't like the code" and are
| often made when someone just joins a team at a point where they
| haven't really had time to familiarize themselves with it.
|
| The first point of the article sort of touches on this, but imho
| mainly misses the point. In a few teams I worked in we had a
| basic rule where you were not allowed to propose extensive
| refactoring in the months (3 or more) of being on the team. More
| specifically, you would be allowed to talk about it and
| brainstorm a bit but it would not be considered on the backlog or
| in sprints. After that, any proposal would be seriously
| considered. This was with various different types of
| applications, different languages and differently structured
| code. As it turned out, most of the time if they already did
| propose a refactor, it was severely scaled down from what they
| initially had in mind. Simply because they had worked with the
| code, gained a better understanding of why things were structured
| in certain ways and overall gotten more familiar with it. More
| importantly, the one time someone still proposed a more extensive
| refactoring of a certain code base it was much more tailored to
| the specific situation and environment as it otherwise would have
| been.
|
| Edit: Looks like it is being touched on in the fourth point which
| I glossed over. I would have started with it rather than make
| this list of snippeted examples.
| aappleby wrote:
| Good refactoring should significantly reduce the size or
| complexity of a codebase.
|
| These two metrics are interrelated, but as a general rule if the
| gzipped size of the codebase (ignoring comments) does not go
| down, it's probably not a good refactoring.
| _a_a_a_ wrote:
| I'm going to disagree and see what other people say.
|
| I don't think that reduction of size is of any relevance. I
| admit my own refractors tend to make things smaller but it's
| only a tendency. Most definitely some increase the size
| overall. I'm currently refactoring a code base - for each item
| there used to be one class. Each object was examined after
| creation then a runtime flag was set: Rejected or Accepted. As
| the code crew I found I was wasting a lot of time around this
| Accepted/Rejected stuff. Now I'm refactoring so I have two
| classes for each item, one for when it's Accepted and one for
| when it's Rejected. The amount of boilerplate has definitely
| bulked up the code but it will be worth it.
|
| As for complexity, I don't know.
|
| The only thing I refactor for is human comprehensibility. That
| is the final goal. What other goal can there be?
| charlie0 wrote:
| Agreed with everything except the following:
|
| >Remember, consistency in your codebase is key. If you need to
| introduce a new pattern, consider refactoring the entire codebase
| to use this new pattern, rather than creating one-off
| inconsistencies.
|
| It's often times not practical (or even allowed by management due
| to "time constraints") to refactor a pattern out of an entire
| codebase if it's large enough. New patterns can be applied to new
| features with large scopes. This can work especially in the cases
| of old code that's almost never changed.
| Flop7331 wrote:
| The key word is _consider._ If you wouldn 't apply the pattern
| to the whole codebase, maybe you don't actually want to
| introduce it in just this one new place.
| charlie0 wrote:
| It's not that it wouldn't be applied to the whole codebase,
| it's that it wouldn't be applied to the whole codebase __at
| once__. You have to start somewhere and new features are a
| good place to start new patterns. Older code can be
| refactored piece by piece.
| doctorM wrote:
| Reading this I realised I've kind of drifted away from the idea
| of refactoring for the point of it.
|
| The example with the for-loop vs. map/filter in particular - it's
| such a micro-function that whichever the original author chose is
| probably fine. (And I would be suspicious of a developer who
| claimed that one is 'objectively' better than the other in a
| codebase that doesn't have an established style one way or the
| other).
|
| Refactor when you need to when adding new features if you can
| reuse other work, and when doing so try to make minimal changes!
| Otherwise it kind of seems more like a matter of your taste at
| the time.
|
| There's a limit of course, but it's usually when it's extremely
| obvious - e.g. looong functions and functions with too many
| parameters are obvious candidates. Even then I'd say only touch
| it when you're adding new features - it should have been caught
| in code review in the first place, and proactively refactoring
| seems like a potential waste of time if the code isn't touched
| again.
|
| The (over) consolidation of duplicated code example was probably
| the most appealing refactor for me.
___________________________________________________________________
(page generated 2024-08-20 23:00 UTC)