Hey! Is there any advantages of having repeated lines extracted

Here are a few scenarios: - A future developer wants to do the same thing as you. They dont find a method to do it, so they reimplement it. You now have three versions of the problem - Your system has a bug. You fix the bug in the first loop but forget that you have a copy of it somewhere else. Suddenly the two versions drift appart, they do the same thing, but one has a bug the other one doesnt - A developer wants to quickly understand what you do. Instead of reading a simple method name, they need to understand your loop. And they need to understand it twice! Your first code is absolutely not good and would not pass a code review. Keep in mind that your software might run 20-30-40 years. And people will need to update or bugfix it

0

12.10.2023

Dmytro Buryak

+1 to every point that @BinaryByter posted, each one is very-very true. And two another arguments to extract them into a method. - Only you know that these two fragments are identical. But not your colleagues. When they will read it, I bet they will spend quite a time to understand whether it's duplicated or has a tiny difference. Because first thought that comes to mind when you see those two pieces - they got to be slightly different, otherwise the author would have extracted it into a method. - What will you win if you won't extract it? Absolutely nothing. Well, maybe 30s-1min of time (in GOOD code of course). But you will get that 1m back and even more, when "future you" will read it later and instead of spending 5-10s to grasp what those pieces are doing, will read a nice clean extracted private method name like "findReportsWithCloseDates" (point N3 from @BinaryByter ). Your code has other significant style (actually "construction", in McConnell's terms) problems, and extracting that piece into a method may become a pain and take much more compared to those mentioned 5-10s in good code. This leads me to another point, not strictly related to your question. Your code style/construction has much bigger issues than few duplicated lines. Even though it's hard to say much without seeing the full class, but I recognize intellij IDE and the way how it uses italic font for *STATIC* fields. Not only you have the HUGE problem that @sanderkoenders mentioned that your methods are non-pure, you also have some global state in those static fields which you modify from instance methods. I can't even express how ugly it looks for experienced eye. It's the same as in C you would declare global variables, which old code is plagued by. Just google for something like "programming why global state is bad". So I highly recommend you two things: - get rid of global state, don't use static fields for anything else than compile-time or initialization time *constants*; and also use static methods only for pure functions (google for something like "programming what is pure function") - make your levelDates to be pure function, don't modify outer state inside of it, and instead receive all the necessary data as parameters, don't modify parameters and create new/copy objects/collections for output. Just like @sanderkoenders suggested you, that's extremely valuable recommendation. I can't even express how important this is to do. Right now your code is hard to maintain, it's very tightly coupled, it's like a web where if you pull one string, it touches all the other. Just like Sander mentioned - somebody changes order/number of execution of your method, and it all may break. Or somebody calls some other method that changes that global static state, and your method may break. You should instead organize your code as Lego pieces - independent, self-contained, loosely-coupled, easily reusable, and easily modifiable. These two recommendations will help you to achieve it.

0

16.10.2023

Dmytro Buryak

PS: generic method names like "walk" is terrible.

0

16.10.2023

s0m31 🇷🇺 Автор вопроса

Dmytro Buryak
+1 to every point that @BinaryByter posted, each ...

Both removing static keywords and making levelDates a pure function is a bad idea in this class. It is used to store data that every other class may access

0

16.10.2023

Dmytro Buryak

s0m31 🇷🇺
Both removing static keywords and making levelDate...

What kind of app is this? Is it server/backend app? Or is it CLI, frontend, etc.?

0

16.10.2023

s0m31 🇷🇺 Автор вопроса

Backend. I was planning to send you a diagram of how this class works bit later and ask if you have any better options than this static variable

0

16.10.2023

Dmytro Buryak

s0m31 🇷🇺
Backend. I was planning to send you a diagram of h...

Then 99.99% you're doing it wrong. If you store data like that, you're making your application stateful and non-scalable. You won't be able to run 2+ instances if load increases. Which practically means your app can't do the job properly

0

16.10.2023

BinaryByter · Accepted Answer

BinaryByter

s0m31 🇷🇺

I dont follow you. are you trying to say that both code examples are similar in code quality?

0

12.10.2023

Похожие чаты

Hey! Is there any advantages of having repeated lines extracted

11 ответов

Похожие вопросы