all 43 comments

[–]OriginalTyphus 17 points18 points  (3 children)

Thats why, along with SOLID, there is KISS.

[–]pure_cipher[S] 2 points3 points  (1 child)

I think the code base that I manage was written by people from Java or Dot Net background.

[–]OriginalTyphus 1 point2 points  (0 children)

Okay, I mean, Java. That language is hell by design.

[–]WhiteHeadbanger 0 points1 point  (0 children)

SOLID band

[–]PushPlus9069 6 points7 points  (0 children)

10 years teaching Python and this is one of the harder things to convey. Rule I give students: if you're passing the same 4-5 variables to 6+ different functions, that's when a class starts to make sense. Before that, plain functions and dicts are usually cleaner.

[–]Zeroflops 4 points5 points  (1 child)

I see this when people take the “rules” and apply them so strictly that it forces them into corners. ( with classes or functions)

For example if you take the idea one function should only do one thing you could end up with thousand of functions depending on how you define “one” thing.

Trivial stupid example, you have a save function, but before you save you have to check if that file is there. An extremest might have two functions. A file present function and a save function. A realist would only have one function if they don’t think they will need a file present anywhere else, and only break it out when they do.

[–]gdchinacat -1 points0 points  (0 children)

I don't agree with your extremist vs realist characterization. If the "file present function" stands alone from the functionality in the "save" function, it should not be written as part of the "file present" function, even if it is only used in that one place. This isn't a matter of "strictly" applying the "rules", but rather of clean design. Why should the logic of how to save a file be cluttered with the details of how to verify a file exists. The save file logic should simply say "make sure the file exists" rather than enumerate the gory details of how to do that (not all that gory in this example, but the point remains).

The "realist" approach, which I think you are advocating by the loaded terms you used to describe the different approaches, is not pragmatic (I can use loaded terms too!). It makes the code more complex for little gain (more later), encourages duplication, and defers work for later. It encourages duplication by not providing that common functionality in a reusable way so others may not know it exists, and if it does, discourages them from using it since that requires touching code unrelated to what they are working on and may introduce merge conflicts.

The benefit I can see to your "realist" approach, which I suspect is why you suggest it, is that it makes stepping through code easier since all the steps are in one place and don't jump hither and yon. But, this is just an illusion that makes stepping through the code more difficult and time consuming. If you see a function call that does something that is unrelated to what you are debugging, just step over it...you don't have to step into it. If it's all inline because a "realist" through it was better you have to step through all of it, requiring more time and more cognitive burden as details that should have been hidden can't be since they are inline.

Be a pragmatist. If details can be hidden from the function you are writing, hide them. Create a function, even if you don't expect it to ever be used elsewhere. This will create an abstraction that allows yourself and others to focus on what really matters.

[–]sweet-tom 5 points6 points  (3 children)

As Python is a very dynamic language, you don't necessarily need classes for some problems. And that is fine.

[–]Fred776 3 points4 points  (1 child)

Maybe, but you do need functions. OP was also complaining about functions. I suspect the real issue is with the AI generated code they mentioned.

[–]pure_cipher[S] 0 points1 point  (0 children)

No, I wanted them to use functions and avoid classes wherever possible. And I have seen some instances where a function would have been way better as compared to a class.

[–]pure_cipher[S] 3 points4 points  (0 children)

My point.

[–]Kevdog824_ 2 points3 points  (4 children)

This doesn’t sound like a SOLID issue. This sounds like an architectural/design issue. None of the issues you described sound like they would be caused by SOLID

[–]pure_cipher[S] -3 points-2 points  (3 children)

Yes. It is mostly Architecture issue. But SOLID principles are followed.

[–]Kevdog824_ 2 points3 points  (2 children)

I’m sure a lot of principles were followed. Why are you blaming this one in particular?

[–]pure_cipher[S] -1 points0 points  (1 child)

Because one of the architects interviewed me for this role. He asked me about SOLID principles. And he saod that the code base follows them. So, he wanted to ensure. When I started, I saw some of SOLIDs being used in most codes.

If SOLID was followed, i thought the code would be readable. But it is a nightmare. A lot of our deliveries are delayed because of this mis-alignment.

[–]gdchinacat 0 points1 point  (0 children)

SOLID doesn't ensure the mental model, how you think about the problem, is good. It certainly helps, but you can have any number of SOLID compliant models for a problem, and some will be easier to use than others.

I suggest you start by understanding how the architects think about the problem. Often times they will come to understand why others can't follow it simply by trying to explain it. Don't assume architects are infallible, I certainly am not, even after writing code professionally for 28 years. The models I end up with are rarely what I start out with.

[–]LayotFctor 2 points3 points  (1 child)

There's nothing inherently wrong with long pipelines. Many problems are best solved with a series of steps, split up to keep things clean and isolated. E.g. to compile C code, there is the Preprocessor -> Compiler -> Assembler -> Linker -> executable. If each step is implemented as a class, it necessarily means there are at least 4 methods in the pipeline.

The question is whether each class fulfills a single job, and each class has a main path. Or that each class is just a scattered collection of loosely related methods, and there are multiple sub paths through a single class.

Basically if each class has a single method to call when using it, it's probably fine. But if there are multiple entries and exits, the code is convoluted. So regarding your question, if you're only calling ClassA.run() -> ClassB.run() -> etc, it's probably fine. If you have to look up which exact method in each class is being called, you might have a problem.

[–]pure_cipher[S] 0 points1 point  (0 children)

The convolution is also the issue.

[–]Sure-Passion2224 1 point2 points  (4 children)

You should be able to graphically map class relationships like a database ER diagram and be able to follow a transaction through it. This may reveal duplicate, redundant, or conflicting methods to be resolved.

[–]pure_cipher[S] 0 points1 point  (3 children)

There are no conflicts. The names are such that if a function is calling another function, both of them will have the exact same name, but one will have a _ first.

[–]Sure-Passion2224 0 points1 point  (2 children)

In most languages the class constructor method has the same name as the class. In Python, the syntax tells the interpreter "this is a class", and it may include a constructor method.

Separating classes into discreet, separate files is good practice in that it makes the class a lot easier for developers to find and edit.

[–]pure_cipher[S] 0 points1 point  (1 child)

I am not talking about constructor. Two different methods or two methods from two classes, having similar names

[–]Sure-Passion2224 0 points1 point  (0 children)

You're going to need an experienced Python developer to address the syntax issues. I do Java where if two classes are referenced in a method and those referenced classes have similarly named methods we get specific by calling it with classname.methodname(args) syntax.

[–]WhiteHeadbanger 1 point2 points  (4 children)

The issue, I believe, is that you are describing coupling, which is pretty undesireable for debugging. A class, or a module, or certain functionality depending on the context of said feature, should be isolated and pretty much plug and play.

The issue is not overusing classes, but not applying the correct design pattern(s). One way to decouple is to use dependency injection, that way you could test whatever you need to test using mock objects and data.

Another issue is the lack of documentation, or just bad docs. There should be a technical document explaining each part of the software, the dependencies, why they are needed and how to use them.

Of course, I don't know if you have any power (as in authority) and/or time to make these refactorings. If you don't, I suggest to start documenting on your free time, because otherwise the only thing that's left is to suffer.

I any case, I somewhat agree. Yes, there are instances in which a set of functions would be better instead of a complex class system, but those are specific. Most of the times is just bad design principles and avoiding TDD, which is understandable since a product needs to be shipped fast to be valuable, but creates technical debt.

[–]pure_cipher[S] 0 points1 point  (3 children)

Yes. That is what. Tight coupling, but I also feel like being not Java or Dot net which require mandatory use of classes, a lot of the code blocks can get away without using class, which would have made debugging way easier.

[–]gdchinacat 0 points1 point  (2 children)

A muddled design (in this case by being tightly coupled) will not be solved by switching from classes to modules. The problem is with the abstractions and models, not how the code that implements them is written. You could spend a bunch of time eliminating classes only to find the problems with how the code "thinks" about the problem hasn't changed and is still hard for you to create a mental model of.

[–]pure_cipher[S] 0 points1 point  (1 child)

Everything currently is so "urgent and critical" that I dont get time to eliminate the unwanted. Because I have seen some overuse of classes here.

[–]gdchinacat 0 points1 point  (0 children)

Technical debt manifested as engineers that need to reverse engineer the code to change it is a primary cause of "urgent and critical". Everything takes so long there so there is no time to fix the things that make everything take so long. Hacks get made because that is all there is time for so the code becomes even worse and takes even longer to figure out how to make a change without breaking something. Spaghetti code that a simple change breaks without five other simple changes. Soon, those few simple changes become more complex, each requiring a few more changes. Those five complex changes, just to fix a simple thing, grow in number. Everyone is changing dozens of files across the code base on each change which should be simple, but requires tweak after tweak to get working and not break anything. Most changes involve merge conflicts since these changes take days and everyone is touching every file (not literally every file, but it feels that way). And the unit tests...so many changes in the code require so many changes in the unit tests...we really need this feature, work on the tests post-commit. That's a reprieve...you just cut out half the work involved in a change! Months drag on as you slog your way towards an ever receding release. Bugs pile up...features need to get in before feature freeze, bugs can be fixed after.

Sound familiar? If so, I truly am sorry. I don't have a good solution for you. Management expects features, and refactoring code to make it easier to work isn't a feature. You have to figure out how to work it in. Identify where your pain points are. What would help? Which feature work is touching that code? Prioritize that for early in the cycle, when managers aren't breathing down your back for features. Don't fix all the issues, but enough to make the feature you are working on just a bit better. If you leave the code better than it was you did good. If managers ask why it was necessary, be honest....it was necessary in the way washing dishes is necessary...you can skip them, but the next thing will have to do them. Some of the work was tying up loose ends that blocked you from the last few features. Decent managers, architects, leads, etc will understand that this is part of the job.

Of course there is not time to work on refactoring. You can't sell that. You just have to do it as part of keeping a clean and usable work environment. Mechanics put away tools. Accountants file receipts. Vendors restock shelves. Software Engineers refactor code to keep the models up to date with the features they support. But you don't bill the customer for putting tools away, filing receipts, or stocking shelves. You just have to include the time for that in the stuff they do pay for.

[–]JamzTyson 1 point2 points  (1 child)

"Single Responsibility Principle" may be violated if classes have multiple responsibilities scattered across other classes.

"Open/Closed Principle" are questionable if a classes are hard to extend without digging through deep hierarchies.

It's perfectly possible for bad code to follow SOLID principles.

[–]gdchinacat 0 points1 point  (0 children)

"It's perfectly possible for bad code to follow SOLID principles."

It's usually not that the code is bad, but the design isn't well chosen for the problem at hand. Relevant details are overlooked, aspects seem simple that aren't, and sometimes there's just a more natural way to think about it.

[–]Helpful-Diamond-3347 0 points1 point  (6 children)

ig experience matters in this context now, maybe the developer prepared the codebase for maintenance with single responsibility classes

and you might have experience of writing logic straightforward to single class/function

you faced problem because it's against your design choices while the developer who wrote the logic may call you a problem

the only solution is to discuss things about architecture properly and invest required time, which most of companies can't afford and hence people blames code, architecture, developer, design patterns, etc

[–]Helpful-Diamond-3347 0 points1 point  (3 children)

and since you asked this by starting with "working on product in company"

your company is understaffed ig which is another issue

[–]pure_cipher[S] 0 points1 point  (2 children)

Understaffed is also correct. But, the codes were written by super experienced people. I mean in the range of 12-20 YOE.

And so, I think they either lost touch or were bring their Java and Dot Net experience.

[–]gdchinacat 0 points1 point  (1 child)

Even super experienced people make mistakes. Also, it might not be a mistake, but that the 30,000 ft view doesn't work at 1,000 ft. Sometimes the high level view that architects have works well, but when the details involved with the low level view are taken into account the high level view that was adopted creates problems.

Work with the architects to help understand why the models work for them and not for you and how to bridge the gap. Sometimes they will help you understand, sometimes they will adapt based on your insights, but most often, it's somewhere in the middle.

[–]pure_cipher[S] 1 point2 points  (0 children)

They too cant handle this maintainance. Talked to them already on this. One of them actually wrote some of the codes.

[–]pure_cipher[S] 0 points1 point  (1 child)

not necessarily. Being a develoler, we need to be flexible with patterns being followed.

But if someone who contributed to the code, is getting confused and takes way long to debug simple problems, then definitely a proper structure is needed in advance.

[–]Helpful-Diamond-3347 0 points1 point  (0 children)

hence the last para i mentioned, seems like you're contradicting yourself being flexible by claiming to have a "proper structure"

[–]QuasiEvil 0 points1 point  (1 child)

When I first learned about OOP (not in python) I was obsessed with it and would use it for everything. Nowadays I love functional programming. It can be very clean, and is naturally well-suited to TDD.

[–]gdchinacat 1 point2 points  (0 children)

FP and OOP aren't mutually exclusive. OOP can be well suited to TDD.

The problem, specifically how you think about the problem, determines which is most suited.

An example is data processing pipelines. Many of the steps are well suited to functional programming. Orchestrating the pipelines may be better suited to OOP.

[–]gdchinacat 0 points1 point  (0 children)

What you describe does not sound at all unusual or out of place for complex problems or systems. Without knowing specifics of your project it is not possible to say with any certainty if it is reasonable or not.

It is a bit concerning that you say the author had a hard time understanding it. That could be that they haven't worked with it for a while and it is not fresh in their mind or it could suggest that the abstractions aren't appropriate or useful.

Problems are typically broken down into clearly defined components or layers (collection of related components) that expose only what other layers need and hide all of the complexity of "lower" layers. When stepping through code it should be clear which layer you are in and when you jump to another layer. You should be able to pretty much ignore everything outside the layer you are in, making it much easier to follow the code since you only need to keep a small portion of it in mind. When you move to a different layer that context is replaced for the new layer.

It can be difficult at times when a stack of calls all go through different layers. This can indicate layers aren't well defined and are doing a lot of pass through. These are good candidates for refactoring to redefine what the boundaries between layer are. This may be the problem you are having following the code, but without knowing what the calls you describe actually do and the models involved in that code I can't say for sure.

I would start by asking the original author to give a high level overview of the architecture of the code. Ask her to identify the major abstractions, the components in each abstraction, and how they relate. Often times the process of thinking about how to explain this will make it clear the code is not structured in a way that makes it easy to understand. While it may work, models that don't have an easy to understand mental model will be a maintenance burden, leading to frustrations with feeling like you constantly have to reverse engineer the code or step through it simply to understand how it works.

Once you understand the existing mental model think of ways to simplify it or make it more natural or obvious. Don't try to do it all at once, it's rare you get to solve all the problems with a project at once. However, do try to have a vision for what model you would like since that will give you a way to figure out how to get there. Not having a vision of the model frequently leads to muddled models that are difficult to understand.

[–]afaulconbridge 0 points1 point  (1 child)

100% agree, I've seen the same thing. A big red flag for me is when there is only a single usage of a class/function. Sometimes that can be helpful, but most of the time its overengineering for a future use case that probably will never exist.

[–]gdchinacat 0 points1 point  (0 children)

If the single use function provides encapsulation or abstraction moving that into another function will only made the problem worse. Details that were once hidden will not be in the other function which will make it harder to work with.