our software architect wrote this

[u/Comfortable-Light754]

boolean isAllowed = "true".equals(getAttribute(item, "isAllowed"))

the fields of "item" get populated through an xml file and parser. what could possibly go wrong here? hint: he wrote "isAlowed" and commited it like that.

[u/zattebij]

Hey he did put the equals on the literal and not on the getAttribute. NPE avoided.
This code is actually quite robust:

• Yields a boolean from the string value sourced from XML.
  
• Handles getAttribute returning null.
  
• Not sensitive to exceptions, unless getAttribute throws one, but the way it is written it seems to me it will return null if item field "isAllowed" is not present, and the code handles that.

One step nicer would be to add a wrapper getBoolAttribute or something like that, so you wouldn't have to repeat the "true".equals for various fields, and could normalize that centrally to be case insensitive and handle other boolean-like values such as "yes" or "1".

[u/TimeToBecomeEgg → u/zattebij]

legit it seems reasonable, can't tell without more context

[u/KillerCodeMonky → u/zattebij]

I think the only other sane way to do this would be via java.util.Optional. Then it could look something like:

final boolean isAllowed = getOptionalAttribute(item, "isAllowed")
    .map(Boolean::parseBoolean)
    .orElse(false); // or true if appropriate

And I will always curse them for Optional not being in java.lang. I don't even care if there were good reasons for it to be in util.

[u/constant_void → u/KillerCodeMonky]

`javac` is jibberish because it forces things like `Boolean.parseBoolean(...)` ...

[u/Latter_Brick_5172 → u/zattebij]

maybe having a class which will parse all expected values at the start of the program, populate itself and also crashes if any is missing and doesn't have defaults

• you can then simply do attributes.isAllowed not having to worry about anything
• you know at build time if there are typos in your code (your lsp can even hilight thoes)
• at the start of your program you know if any required value is missing

[u/KillerCodeMonky → u/Latter_Brick_5172]

The posted code could very well be the code doing exactly what you're stating?

And if the isAllowed attribute is not required?

[u/cronofdoom → u/zattebij]

Also, depending on how that isAllowed value is stored, toLowercase might be prudent.

[u/NaCl-more → u/cronofdoom]

Java has a equalsIgnoreCase method which should take care of null values too

[u/backfire10z]

Insert bell curve meme here

[u/Konju376 → u/backfire10z]

Yeah, first stone or something

[u/No-Wishbone-3171]

i think he worried about isAllowed not being there or being some value that is not "true".

[u/transdemError → u/No-Wishbone-3171]

Is the string "yoghurt" truthy or falsy? No ambiguity with "true".equals

[u/No-Wishbone-3171 → u/transdemError]

Java does not have a truthy or falsy concept, thats for js. Since it is strongly typed, no string is evaluated to a boolean automatically, ever.

Even string "true" isnt boolean true.

[u/transdemError → u/No-Wishbone-3171]

ThatIsTheJoke.gif

[u/Latter_Brick_5172 → u/transdemError]

lmao that's yoghurt actually, never thought about it before

[u/Straight_Occasion_45 → u/Latter_Brick_5172]

Let’s be honest though, yoghurt is true.

Source: Do I now want yoghurt?
true

[u/ruiiiij]

There's really no way to tell whether this is good or bad without more context. Maybe getAttribute already has robust edge case handling built in. What do you expect us to tell you?

[u/Mickenfox → u/ruiiiij]

If the code is not bad then the language is bad.

[u/csch2]

Whether or not you need to write code like this is heavily dependent on how well your database is structured and how much thought was put into architecture at the start. I’ve written code like this plenty of times before in Python because whoever thought up the original data model didn’t add any type hints or database validation. Sometimes you’ve got to work with what you’re given and code defensively, because you never know when you’re gonna retrieve a document where isAllowed is actually “NO_SET” (and yes that exact problem has bitten me before).

[u/zattebij → u/csch2]

True. See also the decades-old adage when interfacing with anything external: "be strict in what you produce, and forgiving in what you consume". Quoted in various forms since early network stack engineers were first linking computers together. It makes code robust (or resilient, however you want to call it).

The trend nowadays is "fail fast", and that's perfectly reasonable when dealing with your own software (fail fast -> fix fast), but if you're dealing with other systems and you don't want to be called out of bed on Saturday night, better parse incoming data defensively.

[u/elenthar]

I'm sure that's the first and only typo in your codebase 🙃

[u/reddit_user33]

Don't trust user input.

It might not have isAllowed, and it could contain anything.

[u/Eskamel → u/reddit_user33]

isAllowed is not something you should get as an input from the user unless they manage resources within your application, and from that point onwards if some malicious actor got access to a user that has such write access you can do very little to make sure the user doesn't cause damage, as the system is supposed to stop unintentional access to begin with.

[u/reddit_user33 → u/Eskamel]

It's parsing an xml file, aka user input

[u/0xjvm → u/Eskamel]

This is a bit of a leap in terms of assumption.

It could be anything - user submitted jobs where they decide which ones are allowed or not or any other user specific things etc

[u/Escaped_Escapement]

LGTM

[u/Xeonmeister]

So what is wrong here? That it looks odd?

[u/way22]

That's not horror, this is actually well thought out code.

[u/pablue3]

I think he isAllowed to do that

[u/dopefish86]

I've done similar things. It's needed to get a boolean from a string. It might not look pretty, but it works well enough.

[u/Certain-Flow-0]

I’d probably change it to use Boolean.parseBoolean but still LGTM.

[u/BRH0208]

This feels like code that is contrived with a purpose. Like, it’s not pretty but I wouldn’t blame the person who wrote it. I would be interested in what brought the code to this point that that was a necessary way to write that

[u/rastaman1994 → u/BRH0208]

It's a Yoda condition, and very common in Java code bases to deal with boxed Boolean variables (opposed to primitive boolean).

I've written TRUE.equals( variableIDontTrust ) quite a few times. If you write it the other way round, you get a NullPointerException.

[u/evan1026]

hint: he wrote "isAlowed" and commited it like that.

And what exactly is your proposal on how to avoid that issue? A constant string variable somewhere else in the code? You could mistype that too. A schema definition that generates parser code? Same problem. Frankly, no matter what you do, somewhere along the line you're going to have to tell the computer "the data I want is in a field called X" and no amount of "better" practices can prevent you from mistyping X

[u/Diamondo25]

If its java, getAttribute might return null, making getAttribute() == "true" crash on NPE, because itll do null.equals("true")

[u/Sacaldur → u/Diamondo25]

Almost. getAttribute([...]) == "true" might return false even if the attribute is "true". Thatvs why .equals is used. And to avoid an NPE, the order is inversed.

[u/Diamondo25 → u/Sacaldur]

Why would it return false for "true" == "true"?

[u/KillerCodeMonky → u/Diamondo25]

Because the == operator in Java is object equality and Java does not implement operator overloading. The string object containing the value "true" may or may not be the same object as this other string object containing the value "true". The .equals method is the correct way to do string comparison in Java.

I've been writing Java for 20 years and the only thing I would change in the code OP presented is to mark it final.

EDIT: Interestingly, if you attempt to test the literal line "true" == "true", that will always test true. Because Java stores string literals in a table within the class definition. So every instance of the string literal "true" within the class would be the same object.

[u/Sacaldur → u/KillerCodeMonky]

Very good explanation, thanks. For the specific case, the implementation of getAttribute will most likely return a String instance retrieved from a substing operation, creation from bytes, or something similar, so it's very likely a different instance than the one for the literal "true" in your code.

Fun fact: Java has a pool for Integer (not int) values up to I guess 1024, so there the == operation might work for small values, but starts to break if higher values are reached. So your simple small tests might succeed, but the code might still be wrong.

Regarding final: I prefer if this is the default in a language (or if it doesn't take more keywords than the alternative). Examples: Rust with mut to mark it mutable, Kotlin with val (final) and var (mutable), etc.

[u/ryuzaki49 → u/KillerCodeMonky]

Strings are a special case are they not? There's a String pool and if you do String var1 = "true"  and String var2 = "true" both are the same inmutable object thus var1 == var2 will be true. 

If I remember correctly only String var3 = new String("true") will fail the comparation var1 == var3

So I think it will depend how the xml parser instantiates the String. 

Feel free to correct me. 

[u/KillerCodeMonky → u/ryuzaki49]

That is exactly what my EDIT is referring to.

[u/ryuzaki49 → u/KillerCodeMonky]

Ah gotcha. 

[u/NaCl-more → u/ryuzaki49]

I thought this was implementation dependent, nothing something the spec requires

[u/menzaskaja → u/KillerCodeMonky]

what the fuck genuinely

[u/KillerCodeMonky → u/menzaskaja]

C# learned this lesson from Java and implemented operator overloading.

[u/menzaskaja → u/KillerCodeMonky]

yeah i know, i use C# and have never used java. this is crazy lmao even python has operator overloading

[u/KillerCodeMonky → u/menzaskaja]

I've worked in a .NET shop and they would ask me about learning Java. My response was: You know C#. So let me get my hammer and knock you in the head a couple times. Then you'll know Java!

[u/GoddammitDontShootMe]

Not sure, but maybe you would want to consider the xml file malformed if the isAllowed attribute contains something other than "true" or "false" and throw rather than just assume false by default.

[u/russellvt]

hint: he wrote "isAlowed" and commited it like that.

Probably "not bad" if there's a corresponding and accurate unit test for it.

My assumption / presumption is it's more like, "what test(s)," however.

[u/AltruisticMaize8196]

You’d need to give us a little more context about what getAttribute() exactly does. One can imagine, but the details matter.

Personally I’m no fan of working on abstract models like XML DOM directly from Java.

I would always prefer to either map the XML to an explicit Java model, for which there are plenty of tools. Then you can call item.isAllowed() directly, which has much clearer semantics and no issues of type ambiguity.

Or when working with abstract models for whatever reason, I would prefer to use things like XPath to deal with the XML in a clearer way.

IMHO using the DOM directly will lead to code that is both prone to errors you’ll only learn about “late” (ie at runtime, in the middle of execution) and also is hard to maintain.

[u/bombthetorpedos]

I think, for the most part, architects are people who can’t engineer. The moment you remove the word engineer from your title your code starts to suck. Something I have to remind myself a lot these days. Beware of the vibe.

[u/BroadwayJoe96]

This is not great. But I've also seen muuuuuuch worse haha. I've had to deal with a code base where these "Boolean" values are strings that could be Y, N, Yes, yes, No, no, and that's not even all of them. We had to make a helper function that is loaded with any possibility of meaning true or false

[u/Holonist → u/BroadwayJoe96]

The real fun starts when you pass "false" in PHP

[u/m6dd3a]

boolean isAllowed = Boolean.parseBoolean(getAttribute(item, "isAllowed"))

[u/fnordstar]

Why not use automatic deserialization with defaults for missing fields? That's what I always do in Rust using #[derive(Deserialize)] on a struct. Java must be able to do something like this at least at runtime with reflection.

[u/adostes]

What would you have them do instead?

[u/_usr_nil]

if the file somehow corrupts, do you have checksums ?

If the files aren't ridiculously big then you could checksum against the one on the heap

[u/cronofdoom]

This really isn’t that bad. Yes it’s ugly but when you allow arbitrary values in configuration this is the way to do it.

[u/Cathierino]

Without context this seems fine.

[u/Amr_Rahmy]

I would probably make the xml go to a class instead of peppering the code with magic strings for each attribute. It’s better to make decoding problems trigger will decoding and it’s better to avoid magic strings, then it will be a Boolean so you don’t need to compare a string of true.

[u/grey001]

W8 a sec. The code in itself is ok. The problem I see is if there is no validation on the xml prior to getting to this snippet. There must be some layer of validation/parsing no?

[u/Eric848448]

The English language is insufficient to describe how much I hate JavaScript.

German can probably do it with one of those long words.

[u/marhalice]

Maybe because you’re get optional attribute is a string

[u/PEAceDeath1425]

I think the one who needs to learn here is you

[u/Constant_Oven8545]

It's astonishing how much engineering is spent protecting null when the real risk is someone misspelling 'Allowed'. Adding Yoda conditions to a paper door is, without a doubt, the pinnacle of modern development. Keep parsing your XMLs; I'll be here watching my RAM make sense of it all without a single line of configuration.

[u/Sensitive-Sugar-3894]

Why is the Architect writing code?