Register a SA Forums Account here!
JOINING THE SA FORUMS WILL REMOVE THIS BIG AD, THE ANNOYING UNDERLINED ADS, AND STUPID INTERSTITIAL ADS!!!

You can: log in, read the tech support FAQ, or request your lost password. This dumb message (and those ads) will appear on every screen until you register! Get rid of this crap by registering your own SA Forums Account and joining roughly 150,000 Goons, for the one-time price of $9.95! We charge money because it costs us money per month for bills, and since we don't believe in showing ads to our users, we try to make the money back through forum registrations.
 
  • Post
  • Reply
Woebin
Feb 6, 2006

Yeah, that's not a style change at all in my eyes, it's pure optimization. Maybe very minor optimization, but still.

Adbot
ADBOT LOVES YOU

Pollyanna
Mar 5, 2005

Milk's on them.


Sounds good. This why I rely on goons to make decisions for me :haw:

CPColin
Sep 9, 2003

Big ol' smile.
I remember, ages ago, asking a coworker why they did things one way instead of another in a pull request and the coworker going 'ugh FINE" and changing it to work the other way I mentioned and I was completely horrified. I had to show it to my boss and ask if I said something I didn't mean to.

kedo
Nov 27, 2007

I'm pretty sure 90% of all developers suffer from crippling imposter syndrome, and code reviews reinforce the idea that they really have no idea what they're doing on a near-daily basis.

Bruegels Fuckbooks
Sep 14, 2004

Now, listen - I know the two of you are very different from each other in a lot of ways, but you have to understand that as far as Grandpa's concerned, you're both pieces of shit! Yeah. I can prove it mathematically.

CPColin posted:

I remember, ages ago, asking a coworker why they did things one way instead of another in a pull request and the coworker going 'ugh FINE" and changing it to work the other way I mentioned and I was completely horrified. I had to show it to my boss and ask if I said something I didn't mean to.

I can understand this because I've had reviewers be very Socratic when they really meant "do it this way." They learn to not to take that approach with me because I will just answer their question and only make a change if it's requested explicitly.

biceps crimes
Apr 12, 2008


Bruegels Fuckbooks posted:

I can understand this because I've had reviewers be very Socratic when they really meant "do it this way." They learn to not to take that approach with me because I will just answer their question and only make a change if it's requested explicitly.

Same, you teach people how to interact with you. I don’t have patience for the asking questions trail of treats to the desired outcome. Be direct or stop wasting my time

BigPaddy
Jun 30, 2008

That night we performed the rite and opened the gate.
Halfway through, I went to fix us both a coke float.
By the time I got back, he'd gone insane.
Plus, he'd left the gate open and there was evil everywhere.


A number of years ago there was a whole to do about why I asked the Indian developers non yes or no questions all the time. When I told them it was to see if they understood what they were being asked since they had a habit to answer yes to anything they didn’t understand so they didn’t have to admit it. Oh you’re racist, that is bigoted etc….

So someone from HR sat in on a few meetings so they understood what I mean. When I ask someone how they plan to meet a certain requirement, what pattern are they going to use and they answer “Yes” then I know they don’t understand and this happened a few times and I didn’t get an apology it just magically went away.

Macichne Leainig
Jul 26, 2012

by VG

BigPaddy posted:

A number of years ago there was a whole to do about why I asked the Indian developers non yes or no questions all the time. When I told them it was to see if they understood what they were being asked since they had a habit to answer yes to anything they didn’t understand so they didn’t have to admit it. Oh you’re racist, that is bigoted etc….

So someone from HR sat in on a few meetings so they understood what I mean. When I ask someone how they plan to meet a certain requirement, what pattern are they going to use and they answer “Yes” then I know they don’t understand and this happened a few times and I didn’t get an apology it just magically went away.

Offshore developers are the worst about being Yes people.

Do you understand how we're describing how this feature should be implemented?

Yes.

*feature is completely out of spec*

I don't think it's racist in my case after working with nearly half a dozen companies and many more dozen individuals to make assumptions about offshore developers.

CPColin
Sep 9, 2003

Big ol' smile.

Bruegels Fuckbooks posted:

I can understand this because I've had reviewers be very Socratic when they really meant "do it this way." They learn to not to take that approach with me because I will just answer their question and only make a change if it's requested explicitly.

I know that move and I felt so bad because I was honestly just asking because I was curious! I freaked out and went way back through old PR's to see if I'd been doing it a lot or something.

fourwood
Sep 9, 2001

Damn I'll bring them to their knees.

Itaipava posted:

Did the reviewer say why they preferred their version? Because if it's just about the early return/breaks, those could just be added to your original code
Yeah, I'd go for some middle ground of just adding breaks in the loops to bail early once the right foo/bar is found. The weird multi-return logic seems like the bigger potential future foot gun without that extra optimization being known to be worth it. Or maybe just re-write the boolean stuff to be cleaner so you can still sort of short circuit the second loop.

ExcessBLarg!
Sep 1, 2001

BigPaddy posted:

A number of years ago there was a whole to do about why I asked the Indian developers non yes or no questions all the time.
So, I understand what you're talking about, but why not just transition to asking all of your candidates non-yes/no questions? Or am I misunderstanding and that's what you did?

chglcu
May 17, 2007

I'm so bored with the USA.
In my opinion multiple returns are totally understandable and going out of your way to avoid them is weird. The optimized code could still be written using a single return though.

And I also don’t understand how avoiding an entirely extraneous loop is debatable. It’s just an obvious optimization and I can think of no acceptable reason not to do it.

e: I guess if you want to be super clear about what's going on and your language has short circuiting, you could just have is_foo and is_bar functions, and is_baz would just be implemented as is_foo() && is_bar(), though that seems like overkill to me if the actual code is as simple as the example.

chglcu fucked around with this message at 19:06 on Apr 21, 2022

Macichne Leainig
Jul 26, 2012

by VG
In unrelated news the CTO at my company is still pushing this Pesto virtual workplace poo poo and I've spoken to handful of people about it. Not a single one is enthused about the idea of a "virtual workplace" that you install on your PC. So that's fun. :shepface:

ExcessBLarg!
Sep 1, 2001

Pollyanna posted:

Say I’ve got this pseudocode:
At risk of bikeshedding, why not write it as this:
code:
func isMatch(elements, match):
  for e in elements:
    if e == match:
      return true
  return false

func isBaz(fooElements, barElements):
  return isMatch(fooElements, matchingElement) &&
         isMatch(barElements, otherMatchingElement)
It's shorter, less code duplication, does the loop optimization with an early return instead of a break, and relies on operator short-circuit evaluation to avoid testing bar. I suppose for type reasons you might need separate isFooMatch and isBarMatch functions, but it should still be easier to follow. I mean, personally, I'm OK with loop breaks and early returns from functions, it just depends on the context.

So then, getting back to this:

Pollyanna posted:

How do you deal with stylistic change requests in code reviews?
When I review code, I avoid commenting on the style as long as it's internally consistent and defensible, even if it's not the way I'd personally write it. It's not my job to rewrite the code, just to review it.

But if the style is objectively bad, or if could potentially introduce downstream issues that the author may not have anticipated, I'll call it out and perhaps offer suggestions. When I'm senior, I'd expect such suggestions to be considered and responded to--either adopting the suggested or a third approach, or a defense of the current one.

Edit: To be clear, I actually think the PR-suggested version is bad style. While it's optimized, I agree that the logic is harder to follow, especially the early "return isFoo" in the bar-checking loop. If I saw that in a code base I'd be asking my "why not write it this way?" question as above (which is no less optimized), but much more forcefully.

ExcessBLarg! fucked around with this message at 19:12 on Apr 21, 2022

BigPaddy
Jun 30, 2008

That night we performed the rite and opened the gate.
Halfway through, I went to fix us both a coke float.
By the time I got back, he'd gone insane.
Plus, he'd left the gate open and there was evil everywhere.


ExcessBLarg! posted:

So, I understand what you're talking about, but why not just transition to asking all of your candidates non-yes/no questions? Or am I misunderstanding and that's what you did?

That is what I did and I still do now. The explanation I gave as to why I did it caused all the problems.

ExcessBLarg!
Sep 1, 2001

Protocol7 posted:

I don't think it's racist in my case after working with nearly half a dozen companies and many more dozen individuals to make assumptions about offshore developers.
It is xenophobic though, unless (i) you're already familiar with a specific off-shore team known to be yes-men, or (ii) you do a quick understanding check for all new teams you interact with, not just the offshore ones.

I get how anecdotal experience works, but racism/xenophobia sucks. Imagine that you're part of a really-talented off-shore team but you're continually stuck with lovely contracts because everyone thinks you're poo poo. Let's be better than that.

tak
Jan 31, 2003

lol demowned
Grimey Drawer
Early return is idiomatic in go, but in this case it doesn't really make the code easier to read imo, which would be the reason for preferring early returns in the first place

Canine Blues Arooo
Jan 7, 2008

when you think about it...i'm the first girl you ever spent the night with

Grimey Drawer

ExcessBLarg! posted:

It is xenophobic though, unless (i) you're already familiar with a specific off-shore team known to be yes-men, or (ii) you do a quick understanding check for all new teams you interact with, not just the offshore ones.

I get how anecdotal experience works, but racism/xenophobia sucks. Imagine that you're part of a really-talented off-shore team but you're continually stuck with lovely contracts because everyone thinks you're poo poo. Let's be better than that.

I have a bit of a hot take.

I do not assume offshore developers are bad because they are offshore. I assume they are bad because they are cheap. I also assume domestic dev houses are bad if they charge $900 / dev week.

There are exceptions. There are certainly people in these dev houses that are probably pretty good, who are trying to break out and find a company that more aligns with their skill level, but an outsourcing manager can't spend their time trying to figure out which studios and which individuals within those studios will be able to turn around good work. If I was the one making a call on this, I'd probably suggest we just hedge our bets, hire a well known entity in NA or EU, and call it a day.

ExcessBLarg!
Sep 1, 2001

Canine Blues Arooo posted:

but an outsourcing manager can't spend their time trying to figure out which studios and which individuals within those studios will be able to turn around good work.
Isn't this literally the job of an outsourcing manager to figure out though?

Canine Blues Arooo posted:

If I was the one making a call on this, I'd probably suggest we just hedge our bets, hire a well known entity in NA or EU, and call it a day.
I totally get going with a well-known entity or one with good references. And yes, it may well be worth going with a more expensive bid if you believe it's likely to result in better work. But where all other factors are equal, there's bad dev shops in the NA and EU, and good ones elsewhere too.

Look, I get the anecdotes about outsourcing. I'm just saying that we should try to be as impartial as possible when evaluating folks from overseas. Or just admit to being xenophobic.

Macichne Leainig
Jul 26, 2012

by VG
Our company did poach one offshore developer and we're working on getting him moved to the US so I'm not absolutely saying it's all terrible. But the vast majority of cases I have seen still do not inspire confidence, especially having been privy to the politicking that can happen when working with offshore teams (primarily between the actual engineering team and the offshore company's management, I mean).

Even companies that were previously good have problems with retention, so one company that might be good isn't 6 months down the road. So you almost are incentivized to headhunt the individuals who stand out.

I don't blame the individuals, it probably sucks rear end trying to get a half decent engineering job in India.

Macichne Leainig fucked around with this message at 20:50 on Apr 21, 2022

Canine Blues Arooo
Jan 7, 2008

when you think about it...i'm the first girl you ever spent the night with

Grimey Drawer

ExcessBLarg! posted:

Isn't this literally the job of an outsourcing manager to figure out though?

OK, actually think about this problem now. How do you *possibly* do this with any level of reliability? What questions do you ask? How do you make a good evaluation?

Here's a fun story: I have a friend who works on a public-facing service that tried to outsource their QA effort to a Chinese firm ("Look at how much we can reduce our QA costs!"). They evaluated them by asking them to do exploratory testing on their live product and to put in bugs. Impressions were positive as they sourced quite a few legitimate bugs. Firm gets hired. After a whole quarter of paying this firm to do QA work, they put in a grand total of SEVEN bugs. They found more bugs in the public facing service then they did on in-development builds. An investigation was done and it turns out they just scraped bug reports from reddit etc. during their initial evaluation.

There is no good way to evaluate these firms without having them do spec work, which is it's own problem.

If I heard from a someone I knew and trusted in the industry that some overseas firm did good work, I'd at least consider them, but it is a veritable minefield. It has nothing to do with race or country of origin and framing it as such is pretty silly. I wouldn't hire an overseas firm for the same reason I wouldn't hire a brand new firm with no verifiable work out of Silicon Valley: It's super risky (and in the case of the Silicon Valley firm, probably also super expensive).

Canine Blues Arooo fucked around with this message at 21:07 on Apr 21, 2022

Macichne Leainig
Jul 26, 2012

by VG
Also, yes, knowing what it costs for a team of 4 developers monthly for example, I think you can absolutely apply "you get what you pay for" in this situation. And naturally not all of the more expensive ones mean more quality. Some companies even have limits on what they pay engineers and they have refused our offers to pay them any more even if it comes directly out of our pocket.

wilderthanmild
Jun 21, 2010

Posting shit




Grimey Drawer

Protocol7 posted:

Even companies that were previously good have problems with retention, so one company that might be good isn't 6 months down the road. So you almost are incentivized to headhunt the individuals who stand out.

I noticed this problem a lot at my last job. Even when quality was good out of off-shore companies, often it was only good for a short while until they lost the good devs. Headhunting the good ones had its own problems, mostly on the company wanting cheap developers, not wanting to pay benefits, and be able to fire them on a whim, which becomes harder if you want to negotiate someone into leaving their current employer.

Rocko Bonaparte
Mar 12, 2002

Every day is Friday!
gently caress my Python brain kicked in reading that and I was wondering, "why not for-else?"

Bongo Bill
Jan 17, 2012

There are lots of people out there who have a tendency to reflexively answer Yes to avoid appearing ignorant. I find that some kinds of company are more likely to either attract or instill that mentality, and trying to cut costs is a big risk factor. So any time I work with new people, I try to assess whether they have that habit, and only ask "Do you understand Y/N?" to those who have demonstrated that they don't hesitate to answer No.

Pollyanna
Mar 5, 2005

Milk's on them.


It’s Pythony cuz that’s easier to type on my phone. It’s actually Go.

ExcessBLarg! posted:

At risk of bikeshedding, why not write it as this:

It’s somewhat overkill, as mentioned previously. I do prefer composing logic from small, easily-testable functions (within reason, not everything needs to be a function), but I’ve received feedback to steer away from that unless truly necessary so I’m trying to rein it in.

And early return is idiomatic in Go, true. But it’s generally used in guard clauses written for the same of avoiding the expanding arrow problem. I’ve still never liked early return, but when in Rome you do as the Romans do and I am in Go’s holy city.

Either way, the codebase as it is right now follows the requested change. Consistency within the codebase and working well with the team is more important than writing it “my way”.

prom candy
Dec 16, 2005

Only I may dance
We hired a guy to build an iOS app for us. American guy, working with iOS since the late 2000s, had lots of war stories from the early days. His code is dog poo poo, and it still cost us a fortune. I was learning Swift and Xcode (with the help of some awesome goons in the iOS thread) so I could kind pitch in on the project and then take it over when the project was done and the slow realization that he was doing everything in the most obtuse way possible and that I would be responsible for maintaining it was awful.

thotsky
Jun 7, 2005

hot to trot
I know a person who does the whole socratic feedback thing and then ignores any response but the change they are implying they want you to make. I think it is meant to come across as less combatative, but it wastes time and feels really passive aggressive.

Xarn
Jun 26, 2015

Pollyanna posted:

How do you deal with stylistic change requests in code reviews?

Say I’ve got this pseudocode:

code:
// if isFoo and isBar, then isBaz
func isBaz(fooElements, barElements):
  isFoo = false

  for e in fooElements:
    if e == matchingElement:
      isFoo = true

  isBar = false

  for e in barElements:
    if e == otherMatchingElement:
      isBar = true

  return isFoo && isBar
and that I’ve received feedback suggesting the code be written as such:

code:
// if isFoo and isBar, then isBaz
func isBaz(fooElements, barElements):
  isFoo = false

  for e in fooElements:
    if e == matchingElement:
      isFoo = true
      break

  if !isFoo:
    return false

  for e in barElements:
    if e == otherMatchingElement:
      return isFoo

  return false
Given that the team doesn’t have a style guide for this codebase, how important is it that these particular changes be made? My natural instinct led me to write the first example in a way that illustrates the fact that if foo and bar, then baz. I accepted and implemented the change request, but now I worry that the newer code is less clear than the original despite making a reviewer happy. That said, I don’t want to hold the code up over stylistic opinions, especially for something as minor as this. But I also don’t wanna slap requested changes on something just to get it through cuz blahhhh.

Is this actually important, or not really an issue?

The second is kinda horrible (return isFoo :wtf:), but I would definitely prefer it due to shortcircuiting checking the whole list if only part suffices.

Xarn
Jun 26, 2015

Bruegels Fuckbooks posted:

I can understand this because I've had reviewers be very Socratic when they really meant "do it this way." They learn to not to take that approach with me because I will just answer their question and only make a change if it's requested explicitly.

If you actually think about the answers, then I like you. If I ask why you did it this way, then I do want an answer. Maybe you will give good answer and I will learn something, maybe not and you will learn something, but either way I've found that people learn much more when they have to think about their design decisions again.

Aramoro
Jun 1, 2012




We've got a contract team just now who are complaining they don't like the work they've been given and want to do something different. To they point the just rejected some work.

My response is 'you're contractors so why don't you go gently caress yourselves' , the engineering director is trying to word it more diplomatically.

Sagacity
May 2, 2003
Hopefully my epitaph will be funnier than my custom title.
What kind of dev work do you have that's so bad that even contractors won't touch it?

champagne posting
Apr 5, 2006

YOU ARE A BRAIN
IN A BUNKER

Sagacity posted:

What kind of dev work do you have that's so bad that even contractors won't touch it?

Underpaid

Aramoro
Jun 1, 2012





It absolutely isn't, it's onshored and paying plenty for it. It was just all bits and pieces they wanted full features to work on.

champagne posting
Apr 5, 2006

YOU ARE A BRAIN
IN A BUNKER

Aramoro posted:

It absolutely isn't, it's onshored and paying plenty for it. It was just all bits and pieces they wanted full features to work on.

What does that mean? Uneven number of hours?

Aramoro
Jun 1, 2012




champagne posting posted:

What does that mean? Uneven number of hours?

nah just like a bunch of small tasks, stuff that was getting in the way of our main teams or was completely self contained. Updating site maps , SEO stuff etc. They're FTE's effectively from a contracting company so their hours aren't affected.

BigPaddy
Jun 30, 2008

That night we performed the rite and opened the gate.
Halfway through, I went to fix us both a coke float.
By the time I got back, he'd gone insane.
Plus, he'd left the gate open and there was evil everywhere.


If you are contracted directly to the company then you do what you are given because that is why you are there. If you are part of a consultancy firm and have been contracted to do Project XYZ and you start to be given other small piecemeal fixes to unrelated code then you can push back but when I see that the push back is mainly to amend the contact to cover this work so they won’t try and wriggle out of paying for it since it wasn’t specified as work we would be doing in the contract.

Either way they are being big babies and if they don’t like the work go find another contract and hope word doesn’t get around they are a pain in the arse.

redleader
Aug 18, 2005

Engage according to operational parameters

ExcessBLarg! posted:

I get how anecdotal experience works, but racism/xenophobia sucks. Imagine that you're part of a really-talented off-shore team but you're continually stuck with lovely contracts because everyone thinks you're poo poo.

then you do what you should have done already: up and out to a better job

meanolmrcloud
Apr 5, 2004

rock out with your stock out

I work with an architect who is a really smart person, but he is self-taught, and the majority of his chops come from being able to parse system design and framework concepts quickly.

It’s very different from other seniors/architects I’ve worked with, who have a freakish understanding of the mechanics of a given language and can explain nuance and quirks with a deep knowledge.

It’s good, because if we are puzzling over a bit of inconsistency, we can figure it out together. But it’s also bad because I might be writing something fundamentally bad, but if it functions and isn’t glaringly bad he isn’t likely to pick up on it.

I don’t know if I have a point, just that it makes me really scrutinize my PR’s before I send em out and I kinda miss a super-wizard breaking things down for me occasionally.

meanolmrcloud fucked around with this message at 16:00 on Apr 30, 2022

Adbot
ADBOT LOVES YOU

YanniRotten
Apr 3, 2010

We're so pretty,
oh so pretty
I would rely on an architect for high level system design across many individual teams and not care if they were a good safety net for whether my application-level code is good. Maybe architect has a different meaning where you are? I'm certainly not having one of my company's few architects review my API code changes, they might review a design document about a large multi-system feature I'm proposing but not a pull request to implement MYPAPI-123.

  • 1
  • 2
  • 3
  • 4
  • 5
  • Post
  • Reply