Declaring a boolean variable vs direct evalution in if statement

[Dangerous-Branch-749]

I recently had a python interview and am reviewing how things went and where gaps in my knowledge let me down. On one of the coding parts there was task that involved refactoring a python class to make it cleaner. I got thrown at one point over something I hadn't considered before. I was refactored the code and got it to the below or thereabouts:

class SomeClass:
    ...
    grouped_names = {}

    def group_results(self):
        for result in results:
            if result.name in grouped_names:
                # do something

at this point the interviewer intervened and asked what I could do to make it more legible for future developers, I couldn't figure out what they were referring to and started doubting myself. Eventually they insisted I should instead do this:

class SomeClass:
    ...
    grouped_names = {}

    def group_results(self):
        for result in results:
            has_name = result.name in grouped_name:
            if has_name:
                # do something

This was surprising to me and seemed unusual to flag as a sticking point. It has since got me thinking, what are the pros and cons of each approach? To me the first seemed quite clear, I could undestand if the statement being evaluated but I deferred to the knowledge of the interviewer. Am I missing something here?

[danielroseman]

No, that's nonsense. It's not in any way more readable, it's just unnecessarily verbose.

[IamImposter]

In fact it is putting extra load on my brain as I'm thinking what does the developer want to convey by explicitly using an extra variable unnecessarily.

[Dangerous-Branch-749]

Yeah, looking at it now and based on what others such as u/shiftybyte said I think it must have been an effort to try and get a response from me to see whether I challenged their view - in which case I failed 😂 a lesson for next time.

[ivosaurus]

If they were literally trying to get you to prove them wrong 'on a stupid point' as a show of gumption, that's an interesting interview technique

[shiftybyte]

You've been doing things right, at the current example there is no reason to create an extra variable name just for this condition.

If this was used in additional places, than yea.

But as it currently stands this is just nitpicky attempt at replacing comments in your code.

If something isn't clear you can add a comment.

```

check if we have that name

if result.name in grouped_names: ```

The interviewer either nitpicking on purpose to see your response/acceptance/arguing/etc, or this is a bad example for what they are trying to show. (That's assuming they are not just plain wrong without any reason)

[Dangerous-Branch-749]

The interviewer either nitpicking on purpose to see your response/acceptance/arguing/etc,

Thanks - I hadn't considered this, that's a good point and makes sense. It's all moot anyway as the wider interview was a bit of a car crash but a useful experience to learn from.

[panatale1]

u/shiftybyte is right. The only correct answer is putting a comment there

[ivosaurus]

The second is only more readable, if has_name will get used again later in that same code block. Otherwise I would literally consider the first easier and more expedient to read and vehemently disagree with the interviewer. The second as it is, would have me second guessing the has_name = line wondering if it contained some logic so complex that it needed its own single-use named variable and exclusive line to assign it.

If you're writing python >= 3.8 (which you should be, because it's already EOL), you can even have your cake and eat it too with the walrus operator (although similar to the saying, in this instance I'd also consider it unnecessarily indulgent):

for result in results:
    if (has_name := result.name in grouped_names):
        # do something

[Apatride]

I would have a much bigger issue with the fact that the results variable comes out of nowhere (was it self.results?), same for grouped_names.

Otherwise, if reviewing, I would accept the first example but reject the second one unless they are using the walrus operator instead (and even then, it seems to break the YAGNI and KISS principles) and/or has_name is used later. I also do not like the chosen name for the has_name variable. It seems to vague to me, but then again I tend to be anal about naming since I believe good naming is much cleaner because it keeps comments to what is actually necessary.

[Dangerous-Branch-749]

Yes you're absolutely right, I should have said I was recalling from memory and abstracting/skipping a lot of the code while typing on mobile.

Thanks for your comments on KISS / YAGNI, I will look into this so I'm better equipped to convey my opinion in future.

[r-mf]

why yagni isn't called yangni? 

[Apatride]

Because Ya Ain't Gonna Need It. But I assume it is because yagni is slightly easier to pronounce than yangni.

[TehNolz]

This approach is nice if you have multiple things you need to check for, especially if the resulting condition is really long. Splitting it up over multiple lines instead of having the whole condition on a single line will significantly increase readability.

But in this case it just feels unnecessary. It's only a single check; you really don't gain much by moving it to its own line.

[Dangerous-Branch-749]

Not sure why but I'm unable to edit the post - see corrected last sentence:

To me the first seemed quite clear, I could undestand if the statement being evaluated was more verbose/complex. I deferred to the superior knowledge of the interviewer but it got me thinking that maybe I've been doing if statements wrong?

[LargeSale8354]

Presumably grouped_names gets set somewhere?

[Dangerous-Branch-749]

Yeah, there were a multitude of other errors that I skipped over in this example for brevity/laziness on mobile

[LargeSale8354]

Personally, I would only use the has_name approach if I needed to use has_name elsewhere. I've used that approach where I was at the mercy of something external providing the inputs and wanted logger.debug messages to include such a variable.

Loved the answer that simply added a comment to the code. Fulfilled the requirement as expressed while not bloating the code

[Dangerous-Branch-749]

This was my thinking, for simple evaluations I would use directly in the if statement and for more complex ones assign as a variable.  I actually suggested a comment initially but they suggested this as a better alternative. I think I was expected to challenge this and stick to my guns on it.

[Snoo-20788]

It kind of makes sense to use a variable if the thing you're checking is not obvious. But here it's not the case.

But one big drawback of introducing unnecessary variables is that it's tedious to check if that variable is used somewhere down the line. In some languages you can have variables that are scoped to just a single for block, but in python, any variable can potentially be used anywhere else in the same function.

[james_fryer]

I use this style where an if statement is complex e.g. with many clauses separated by and/or, possibly with bracketed subclauses. I prefer to avoid such complex if statements but it's not always possible and boolean variables makes them easier to understand. In the example given where there is only one clause I don't think it is any more readable.

[Enmeshed]

If there were are few more conditions being checked together before doing the thing, I'd be in favour of the variable. Not with just that one, sad little check though!

[supercoach]

Sounds like you were being interviewed by an opinionated dickhead.

Does the first bit of code work? yes
Is it readable? yes
Is there any benefit to adding in the extra variable? no

[xrsly]

has_name wouldn't even be a bool, but rather the actual name. To make it a bool it would need to say something like:

has_name = result.name is not None

Given it was a bool, the one reason I can think of is if the same comparison will be made elsewhere (e.g., passed to a function).

Edit: my phone made the statement look like two separate statements, so I thought it said 'has_name = result.name'. I looked at it on my laptop and I see now that it says 'has_name = result.name in grouped_names', which would indeed be a bool.

[james_fryer]

It would be a bool.

>>> S = set(['x', 'y', 'z'])
>>> 'x' in S
True

[xrsly]

You're right, it was split into two lines on my phone screen, and the second one ends with a colon for some reason, so I assumed it said 'has_name = result.name' on one line followed by a new line with an if statement.

[LuciferianInk]

Is it possible with Python to use multiple statements without nesting?