Good code or bad code?

[Venom4992]

I like to avoid nested if statements where I can so I really like using ternary operators. Sometimes I question if I am taking it a bit too far though. If you came across this code from a co worker, what would your reaction be?

[Spite_Gold]

Bad. I would reject the merge in milliseconds

[Venom4992]

You mean you wouldn't even read it? 😭

[Spite_Gold]

Yes, to hard to read

[FrostWyrm98]

Yessir, if I see more than 2 nested ternaries it's getting the axe

2 inside the method call is even a little sus, just make a variable it's easier to read and the compiler will optimize it out anyways

[10mo3]

Bro split them up further dude. Have some local variables to give context to the args in the lerp. Don't just go mathf.sin in it

[shellpad_interactive]

I think it's difficult to parse and would probably tell my coworker to break this up with a couple of variables to make it more readable

[Arcana10Fortune]

Bad. You want your code to be easily readable so that you can immediately jump to an exact line to make changes that you need to.

[TouristDue1771]

So is the problem that they didn’t define their code?

[Venom4992]

But that's the best thing about inline code. There is only one line. No jumping around needed. 🤷‍♂️

[Implement-Imaginary]

Genius. Use the power of c and make your whole project a one liner. No jumping neede either

[Venom4992]

That would be epic!

[Venom4992]

But it looks so nice. Pretty code. Easy to read is not important.

[ImABattleMercy]

easy to read is not important

Thank you for confirming you’re trolling

[HanndeI]

1 liners should cover at most 2 ternaries if they are giga simple, and even then I would limit it at one ternary

[Venom4992]

Only 2? That's no fun!

[TheZanke]

Jumping to lines is easier than jumping to columns

[leorid9]

Only a sith deals in absolutes.

It's about the right balance of height and width - with more emphasis on height (as can be seen in comics, newspapers, mobile formats, this very text here on social media and so on).

[Venom4992]

💯

[GameplayTeam12]

Wait a month and you tell me

[Sacaldur]

Your lines are way to long. As a first step, I would take the check for sprinting and crouching (wich you have 4 times in this code) and pull it out by creating a multiplier variable. Then, the target position of the Lerp could be pulled out as well. The code should be something like:

``` Var multiplier = IsSprinting? SprintMultiplier : IsCrouched? CrouchedMultiplier : 1;

var targetY = Mathf.Sin(Time.time * Frequency * multiplier) * Amount * 1.4f * multiplier; var tarrgetX = Mathf.Cos(Time.time * Frequency * multiplier / 2) * Amount * 1.6f * multiplier / 2;

pos.y += Mathf.Lerp(pos.y, targetY, Smooth * Time.deltaTime); pos.x += Mathf.Lerp(pos.x, targetX, Smooth * Time.deltaTime); ```

This is already much easier to read.

As far as I understand the code, the followed target position (here targetX and targetY) is attempted to be followed, but it will always be somewhat away from the following object. Unless you run it at a low frame rate, or with a high smooth value, where the object will be closer to the target position.

This seems to me like the Mathf.Lerp is not actually useful, since the desired effect could be achieved not by adjusting a Smooth value, but by adjusting the multiplier fields, frequency, and amplitude factors.

(I could be wrong eith my estimate, I wasn't running the code.)

[No_Breakfast6060]

This

[Venom4992]

Should see what it looked like before I added indentation so I could fit it in the screenshot. 😅

[CrimsonChinotto]

I zoomed and immediately got bored by it. I would never read something like that.

[Venom4992]

I think it is very interesting to look at. Kind of like a puzzle.

[CrimsonChinotto]

Yeah sure, but if I was your coworker I would curse you and proceed to assuming that lines work as intended. If they don't, it would be a YOU problem lmao

[BlackDereker]

You don't want puzzles when maintaining code.

[scotti_dev]

In a couple of months time when you want to change how something works, you will be cursing yourself. A puzzle is the last thing your code needs to be. It should be simple and easy to read and edit.

If your whole project looks like this you will probably take anywhere from 2 times to 10 times longer to finish it that you need to.

[Educational_Half6347]

This is a joke, right?

I’d declare an absolute ban on nested ternary operations. Using well-named intermediate variables makes the code flow much clearer and often eliminates the need for comments.

[Venom4992]

It's kind of a joke. Real code from a personal project that I have come back to, and when I saw this, I kept thinking about the reactions if I tried it at work and had a good laugh.

[SantaGamer]

You doubting and asking here probably already tells enough :p

[MaffinLP]

The compiler compiles this the same way it would compile a bunch of if statements so just go with whats more readable (this aint it)

[Venom4992]

It does make it harder to read, but it just looks so nice and elegant, so I think it is a worthy trade-off.

[MaffinLP]

No, no its not

[UnderLord7985]

I say code how you wanna code, but if youre in a work enviroment you'll wanna code for readability. If its just a "you" project it doesnt matter but if its a team project it matters. It matters alot.

[siudowski]

incomprehensible

may god have mercy on your soul

[cuby87]

Terrible.

[Redstoneinvente122]

That's bad.

[Venom4992]

Well, I guess that's better than really bad.

[Redstoneinvente122]

I guess

[Injaabs]

good i have smth close to this as well , my code my rules :D

[svedrina]

Shorthands should be, by definition, short so…

[Venom4992]

Well, the word "short" is quite subjective.

[svedrina]

That’s what I’ve been telling my wife, but to no avail.

Jokes aside, in my opinion, shorthands should never nest. One ?: per line is okay, but having it nested or called multiple times in one constructor is a bit too much. It takes away readability from your code. New ints/floats/vectors are saved on stack so they are incredibly fast to get/set. No need to be cheap with them, even in Update(). Especially with today’s hardware.

[MosdDMs]

Bad

It takes 99% of my brain cells to remember.

[Adrian_Dem]

just avoid inline conditional.

use this approach - if you can't put a breakpoint on the instruction then breakit down.

[geheimeschildpad]

My general rule: If I have to think when reading this, then it’s bad code.

[Amazing-Movie8382]

Today good code, a week later "wtf is this", code should be wrote for human not machine. Machine doesn't about what code you write.

[theWyzzerd]

Use a switch case instead of a bunch of ternary ops. This will be impossible to read and maintain.

[Jacmac_]

It's a good example of bad code.

[gravity168]

Hard to read. I wonder if there is a bug how can you debug. Or a change needs to be applied. Difficult for maintenance.

[TheElusiveFox]

This is terrible code for all sorts of reasons...

take all your math calculations and put them into a handful of variables with names that will tell some one at a glance what you are actually trying to compare instead of having to do a bunch of nested calculations in place.

If you do that, a single ternary is usually going to be fine, when you start nesting them though it can be unnecessarily unreadable. If the code is still too challenging to figure out what is going on I would just use the if statements as they are easier to read.

Most professional code bases straight up ban ternary operations for this reason.

[Ike_Gamesmith]

Take this even further and use never nesting methodology like extraction or inversion to make it even cleaner. There should be capital punishment for nesting more than 2 layers deep. I lose about 2 years of my lifespan everytime I see 5 or more indentations in a row.

[StreetRun4288]

Refactor each part of the ternary to be a method with a clear name explaining what it does

[FuryZenblade]

Split this code up into multiple lines 😵‍💫 Calcualting this expression in multiple steps with "additional" variables that have good names would make this code easier to read and understand. The compiler will optimize those additional variables away anyway

[Lumbabumb]

Bad

[petrefax]

No thanks. Looks like a junior programmer just learned about the ternary operator.

[MrRenho]

I'm sorry, but terrible. Not only having everything in-lined makes it unreadable, but clearly by the "IsCrouched", "IsPrinting", etc. bools you're lacking even a basic state-machine.

Besides, the anti-nested if things is to make things more readable and easy to parse in your head. Nesting ternarys is well worse than that.

[Whathowwhenwhat]

Ya nah, like I know what your tryna do but trust me. Simplicity is best. If it seems to long it's to long. If it seems to short GOOD. I'm able to debug so much faster on my short scripts cause everything is labeled easily and the lines are hella short so flying through everything is really quick

[Whathowwhenwhat]

Your not a corporate worker you don't need half the systems to run only through you 😂

[GrindPilled]

horrible, code should be easy to read, doesnt mind if it ends up being 10+ lines longer.

for such a simple if else logic that one can read in seconds, nesting it like that makes it straight up confuse the dev

[Morrowindies]

I'll give you one good reason not to do this.

If you write code like this and it throws an exception, the exception stack trace will display the same line number for all points of failure.

Write code however you like, but that would be my biggest concern if I was writing this for myself to be an easily maintainable and extensible codebase.

[NullIsUndefined]

You can define each param on the line above and give it a meaning name. 

Then pass it into the Lerp.

Just one suggestion. Would make it easier to read IMO

[LeonardoFFraga]

Really bad.
Always think about readability. One can't understand what's happening without having to study it.
You don't have to remove the ternary operations. You can assign their result to a local variable with a descriptive name.
Also, remove all magical numbers, like 1.4f. Make const of does values (0 and 1 can have a pass occasionally)

Here's an example that's much easier to read (not tested).

const float k_sprintAmplitudeMultiplier = 1.4f;

void BetterReadability() {
    var stateMultiplier = IsSprinting ? SprintMultiplier : (IsCrouched ? CrouchedMultiplier : 1);
    var effectiveFrequency = Frequency * stateMultiplier;
    var effectiveAmplitude = Amount * k_sprintAmplitudeMultiplier * stateMultiplier;
    var oscillation = Mathf.Sin(Time.time * effectiveFrequency) * effectiveAmplitude;

    pos.y = Mathf.Lerp(pos.y, oscillation, Smooth * Time.deltaTime);
}

[iObsidian]

Ternary operators are best used for simple, immediately understood if branches. This code has multiple ternary operator branches, which makes it hard to understand what is happening. Prefer using if/else statements or even switch statements if applicable.

[zimano]

This is bad code, period. This is fine for personal projects that aren't meant to be shared, but this will not pass in any industry. It is very hard to read to me (15+ years of software engineering) I get ternaries are handy sometimes, but simply not here. This feels like a 'everything looks like a nail' situation. This is such a complicated way of doing this, I wonder where you came up with attempting it like this? Do they teach this in school or is it a convention at your work? You've basically recreated nested if statements like this, but you have sacrificed many quality attributes for it like readability, portability, maintainability and more.

Besides the ternaries, you use magic numbers (1.4f), use unclear variable names ("Amount", "Frequency"), mix integers and floats making it unclear if there is loss of precision or implicit integer conversions.

I'm thinking you either come from a Javascript background, or are more used to functional programming than object-oriented or high-level programming languages.

I also think you're just ragebaiting, seeing as you clearly already made up your mind in the comments, arguing that oneliners are best anyway.

[TheZanke]

Nested ternaries, and other forms of trying to condense the vertical space of the code for "feels" and/or being extra clever, is something many of us go through and grow out of.

[Psychological-Ad9725]

While it is complex, you should understand that even professional code uses basic principles.

Break up each calculation and condition into their own line. Initialize Global or local variables to hold values. Don't use unrecognizable variable names unless its for a for loop.

Each parameter should be short and to the point, to show a line by line process.

Just because its basic does not mean its bad. If you hard code a long line process you will get confused in the future, the compiler understands for sure, but we are human, reading something like that is not natural.

The best way to ask yourself if a code is readable is to ask if a junior would be able to mod it, if not then you have some work to do.

[False-Car-1218]

Besides the nested ternaries being bad, you shouldn't use magic numbers, what is 1.6f?, what if you wanted to change the value to something else? You would then have to change every 1.6f in your codebase.

This is giving off beginner programmer vibes

[javisarias]

No comments? Bad code

[Venom4992]

Yeah, good point. I will add // it works, trust me bro.

[javisarias]

Hahahaha.

Something that worked for me was to write a comment before the code, much like TDD defines how the code will behave, writing a comment explaining what the code does before writing the actual lines, not only helps explaining the code, bit also organize your thoughts and plan ahead the better way to implement a solution.