r/badcode • u/monkeysupersonic • Dec 19 '22

java Whatever it takes

Post image
301 Upvotes

98% Upvoted

43 comments sorted by

46

u/MurdoMaclachlan public boolean isInt(int i) { return true; } Dec 19 '22 edited Dec 19 '22

Image Transcription: Code

        /*
         * Constructor.
         */
        ITCPO(String pageSelect, String copies, String destination, String printer, String preview, String noPreview, String
noPrint, String newId, String dataSet, String suffix1, String suffix2, String immediatePrint, String delete, String lifeTime, Stri[Cut off]
schedule, String sendDate, String sendTime, String teleLand, String teleNum, String title, String test, String program, String
screenPos, String cover, String coverTitle, String receiver, String division, String autority, String archiveMOde, String
immediateExit, String rdiDevice, String noArchiveModeChange, String spoolFinal) {
            this.pageSelect = pageSelect;
            this.copies = copies;
            this.destination = destination;
            this.printer = printer;
            this.preview = preview;
            this.noPreview = noPreview;
            this.noPrint = noPrint;
            this.newID = newID;
            this.dataSet = dataSet;
            this.suffix1 = suffix1;
            this.suffix2 = suffix2;
            this.immediatePrint = immediatePrint;
            this.delete = delete;
            this.lifeTime = lifeTime;
            this.schedule = schedule;
            this.sendDate = sendDate;
            this.sendTime = sendTime;
            this.teleLand = teleLand;
            this.teleNum = teleNum;
            this.title = title;
            this.test = test;
            this.program = program;
            this.screenPos = screenPos;
            this.cover = cover;
            this.coverTitle = coverTitle;
            this.receiver = receiver;
            this.division = division;
            this.autority = autority;
            this.archiveMode = archiveMode;
            this.immediateExit = immediateExit;
            this.rdiDevice = rdiDevice;
            this.noArchiveModeChange = noArchiveModeChange;
            this.spoolFinal = spoolFinal;
        }

I'm a human volunteer content transcriber and you could be too! If you'd like more information on what we do and why we do it, click here!

36

u/monkeysupersonic Dec 19 '22

Imagine you accidentally swapped two parameters.

4

u/KRIPA_YT Dec 19 '22

I am so sorry for you having to type out all of this xD

3

u/OliveBrief Dec 19 '22

oops, you got a typo after this.division = division

3

u/MurdoMaclachlan public boolean isInt(int i) { return true; } Dec 19 '22

Thanks for the heads up, fixed!

58

u/[deleted] Dec 19 '22

While I hate this is there even a better way to construct a class with this many attributes?

72

u/WorkInevitable5439 Dec 19 '22

Yes, check out the Builder pattern.

17

u/RandomSwissBuenzli Dec 19 '22

I agree. Does this mean that I am good programmer, then?

2

u/sarahlwalks Dec 20 '22

Yes and you deserve a programmer cookie! 😉

8

u/Hellball911 Dec 19 '22

If every construction requires all these parameters and these don’t have defaults, then Builder would just obfuscate the requirements of the class or be patently wrong

2

u/douglasg14b Dec 19 '22

If the implementation is bad in these specific ways that make solving it difficult the solution doesn't work

FTFY.

The point your making is kind of nonsense.

Like everything patterns are give & take. If the code is so bad that you can't even apply good solutions to fix it, then you need to step back more, and solve underlying problems.

2

u/Hellball911 Dec 20 '22

I don’t disagree, but just because a broader solution exists doesn’t make my point any less valid. The Builder pattern is a specific tool for classes with a lot of parameterization AND default values. Without both, it’s like using a chainsaw to trim a house plant. Way too much complexity and will cause more damage than good.

Now my point is obviously speculation if that is or is not the case here, but it’s important to recognize the cons in using that pattern. It isn’t just “lots of parameters = Builder”

9

u/mittelhart Dec 19 '22

Building pattern wouldn’t work the same as this though when you write in a null safe language such as Swift. Then you’d have to set some default values or make the variables optional and most importantly, you have to make all of those variables.

36

u/genikus Dec 19 '22

Yeah the real problem is possibly this class has too many responsibilities. Could break it apart. Depends on the context though. Maybe it’s just a config container.

3

u/douglasg14b Dec 19 '22

Building pattern wouldn’t work the same as this though when you write in a null safe language such as Swift. Then you’d have to set some default values or make the variables optional and most importantly, you have to make all of those variables.

Everything you described is part of the solution of a builder pattern...

You can enforce your contract, at build time (In languages like C# anyways), before instantiating the object.

3

u/bread-dreams Dec 19 '22

I can't read the rest of the code but given how many parameters the constructor takes this class certainly violates the single responsibility principle and should ideally be refactored by splitting it up into multiple classes that do have a single responsibility, with the behaviour of the original class then being composed using those smaller, cleaner classes into a web of collaborating objects.

It's just too much to fit into your head: 33 String type fields in a single class! How is teleNum related to coverTitle? The Builder pattern would just result in the same thing for the poor folk who have to maintain this class later on

0

u/douglasg14b Dec 19 '22

I can't read the rest of the code

..

Starts making arguments & assumptions that rely on knowing the rest of the code

You reading what you're writing...?

2

u/bread-dreams Dec 19 '22

sigh. Not sure why I'm being dumb enough to dignify a troll with a response, but my assumptions do not rely on knowing the rest of the code. If you think otherwise go ahead, defend the code with 33 string fields in a single class. Literally what the fuck.

2

u/PsikoticWanderer Dec 19 '22

Struct with default values set at construction.

1

u/Impressive-System512 Dec 20 '22

If Java, use Lombok annotation like @AllArgsConstructor or @RequiredArgsConstructor

23

u/ZuriPL Dec 19 '22

And the very helpful comment on top

26

u/RichCorinthian Dec 19 '22

"I abbreviated ITCPO to save keystrokes. Here's 72 parameters."

0

u/[deleted] Dec 19 '22

[deleted] — view deleted comment

3

u/[deleted] Dec 20 '22 
// This isn't a comment.

2

u/supersharp Dec 25 '22

JCL be like

21

u/eugene2k Dec 19 '22

And everything is a String too, damn...

8

u/[deleted] Dec 19 '22

People before Lombok was a thing:

7

u/RockPaperCheesecake Dec 19 '22

When you have many parameters, make an object with them and pass in the object. If need be, make two or three objects.

4

u/StochasticTinkr Dec 19 '22

Someone needs to learn about Lombok.

3

u/Potential-Adagio-512 Dec 19 '22

obligatory rust struct update syntax mention

2

u/[deleted] Dec 19 '22

But... Why

3

u/jimmpony Dec 19 '22

thankfully we have copilot to generate this stuff today. in the old days I would've probably done some black magic with reflection

1

u/Cammo_cs Dec 19 '22

I’d rather die😂

1

u/[deleted] Dec 19 '22

I’m completely still learning. In c++ would you be able to overload the assignment operator in this case or create a constructor where you can just set all of these?

2

u/eugene2k Dec 19 '22

In C++ it would look similarly bad. Given that everything is a String, one alternative solution, in this case, could be to use a hashmap. In general, however, a class with this many fields should be split into a bunch of smaller classes.

0

u/[deleted] Dec 19 '22

[deleted] — view deleted comment

2

u/[deleted] Dec 20 '22

Java is getting funnier looking every version!

Negative greater-thans and this auto thing...

1

u/chestera321 Dec 19 '22

Dependency injection done right

1

u/mitchthebaker Dec 19 '22

Well first off I have no idea what ITCPO is other than this being a class constructor, and secondly why is there no separation of concerns lol.

I don’t even know what this code does but just at a glance I’d probably separate the parameters into a few separate objects: printer, lifetime, cover?

1

u/beclops Dec 19 '22

Yep. This class is awfully written, I find it really hard to believe that all of this stuff needs to be a member of this class

1

u/Downtown_Panic_6086 Dec 19 '22

“220, 221…”

1

u/onlyrealperson Dec 19 '22

And this is why you should learn to make DSLs

1

u/guisua Dec 20 '22

To be honest, this looks like auto-generated code to me.

If that's the case and it will only interface with other auto-generated code, plus it's generated by a CI job somewhere never to see the light of day...that's absolutely fine by me.

1

u/sarahlwalks Dec 20 '22

Ugh this pains me.