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
necrobobsledder
Mar 21, 2005
Lay down your soul to the gods rock 'n roll
Nap Ghost
9 pm is oftentimes these days when I start doing actually productive work instead of low business value BS like maintenance overhead, meetings of no real importance, and running around trying to keep things from falling over completely in production.

Adbot
ADBOT LOVES YOU

Harriet Carker
Jun 2, 2009

necrobobsledder posted:

9 pm is oftentimes these days when I start doing actually productive work instead of low business value BS like maintenance overhead, meetings of no real importance, and running around trying to keep things from falling over completely in production.

Different strokes for different folks. I can count the number of times I’ve opened my work laptop past 6 in the last year on one hand.

Volguus
Mar 3, 2009
Oh god, do people really go to bed at 9 ? I know i'm a galaxy away from healthy with my hosed up sleep pattern, but at 9 ... that's kinda pushing it.

TooMuchAbstraction
Oct 14, 2012

I spent four years making
Waves of Steel
Hell yes I'm going to turn my avatar into an ad for it.
Fun Shoe

Volguus posted:

Oh god, do people really go to bed at 9 ? I know i'm a galaxy away from healthy with my hosed up sleep pattern, but at 9 ... that's kinda pushing it.

If you want to get up before 6AM on a regular basis and not be sleep-deprived, then that's the kind of sacrifice you have to be willing to make.

Harriet Carker
Jun 2, 2009

Yep. I get up around 6 daily. I don’t go to sleep at 9 but I’m in bed then. I chat with my wife or read a book for a half hour to an hour or so before sleeping.

Jose Valasquez
Apr 8, 2005

TooMuchAbstraction posted:

If you want to get up before 6AM on a regular basis and not be sleep-deprived, then that's the kind of sacrifice you have to be willing to make.

N... not... sleep deprived? :confused:

Sab669
Sep 24, 2009

It's 10PM on a Friday and I just shut down my PS4 to go to sleep on a Friday at I'm still in my 20s lol.

Never mind "staying up late" on an actual work night

Careful Drums
Oct 30, 2007

by FactsAreUseless
It's 10 and I'm very ready for bed, but I have to get some hours in on this take-home and sort out a thing for a freelance client >:|

Ideally I'd be in bed by 9 and up at 5 every day but I can hardly get all my kids in bed by 9.


e: More remote job search updates: A small consultancy I interviewed with before the holiday got back to me today and wants to set up a more technical interview. So that's cool.

Careful Drums fucked around with this message at 04:03 on Jan 12, 2019

Volguus
Mar 3, 2009
At midnight i'm not willing, nor ready for bed. But i do wake up at 7 am. Oh well, i guess i'll just die at 50.

spiritual bypass
Feb 19, 2008

Grimey Drawer
Yeah, ok, going to bed at 9pm is good sometimes but if I'm out traveling on business, I do expect my coworkers to be friendly, have a couple drinks or whatever. You know, just decent social skills.

One of the recurring problems that comes up in this thread is the delta between techies' expectation of the need for social skills and the fact that creating business software is more of a social process than it is one-person technical wizardry.

spiritual bypass fucked around with this message at 22:51 on Jan 13, 2019

TooMuchAbstraction
Oct 14, 2012

I spent four years making
Waves of Steel
Hell yes I'm going to turn my avatar into an ad for it.
Fun Shoe
How do y'all get your junior teammates to build better software? At the moment all of my code reviews are involving a lot of back-and-forth because they're sending poo poo out for review really before it's ready. I remain hopeful that they're capable of better (i.e. they're not just crappy devs) but I don't know how to go about improving their output.

On a related note, how do you get your teammates to do more rigorous code reviews? Because those not-really-ready change lists are getting a pass from other teammates despite glaring flaws.

Good Will Hrunting
Oct 8, 2012

I changed my mind.
I'm not sorry.
Are they repeatedly making the same mistakes? Are the specs good? What types of feedback are you giving?

TooMuchAbstraction
Oct 14, 2012

I spent four years making
Waves of Steel
Hell yes I'm going to turn my avatar into an ad for it.
Fun Shoe
This isn't failing to hew to specs. But, uh, some examples...
  • code:
    for x in y:
      if z:
        <entire rest of for loop>
    
    (where it'd be preferable to say "if not z: continue" to avoid excessive indenting)
  • Skipping writing tests, and pulling the old "I'll write the tests after this gets checked in" when they get called on it.
  • Excessively over-extended functions (well over 100 lines long when there are clear points where they could be decomposed)
  • Confusingly-named variables, and variables and functions that shadow other ones in the same scope
  • Lists that should be sets, tuples that should be structs / named tuples, multiple arrays that have to be kept in sync (using the same index in each to access different aspects of the same data) rather than a single array holding a complex datatype.
  • Throwing together six unrelated things into one gigantic change instead of breaking them out into smaller ones.
  • Comments that assume you already have context on the problem the code is solving.
  • Declaring a boolean as foo = False if bar else True. This one always makes me sad to see.

For the most part these don't on their own represent huge problems. I very rarely have to say "no, stop, this is a fundamentally wrong approach." But I end up with like 20-30 nitpicks and requests for changes and clarification even on relatively small changes. What worries me is that they don't seem to be getting better at this: I'm seeing the same categories of mistakes being made repeatedly by the same people.

prisoner of waffles
May 8, 2007

Ah! well a-day! what evil looks
Had I from old and young!
Instead of the cross, the fishmech
About my neck was hung.

TooMuchAbstraction posted:

This isn't failing to hew to specs. But, uh, some examples...
  • code:
    for x in y:
      if z:
        <entire rest of for loop>
    
    (where it'd be preferable to say "if not z: continue" to avoid excessive indenting)
  • Skipping writing tests, and pulling the old "I'll write the tests after this gets checked in" when they get called on it.
  • Excessively over-extended functions (well over 100 lines long when there are clear points where they could be decomposed)
  • Confusingly-named variables, and variables and functions that shadow other ones in the same scope
  • Lists that should be sets, tuples that should be structs / named tuples, multiple arrays that have to be kept in sync (using the same index in each to access different aspects of the same data) rather than a single array holding a complex datatype.
  • Throwing together six unrelated things into one gigantic change instead of breaking them out into smaller ones.
  • Comments that assume you already have context on the problem the code is solving.
  • Declaring a boolean as foo = False if bar else True. This one always makes me sad to see.

For the most part these don't on their own represent huge problems. I very rarely have to say "no, stop, this is a fundamentally wrong approach." But I end up with like 20-30 nitpicks and requests for changes and clarification even on relatively small changes. What worries me is that they don't seem to be getting better at this: I'm seeing the same categories of mistakes being made repeatedly by the same people.

And you're responsible for the code quality and these juniors improving their contribution quality? How much do you want to work out directly with the juniors and how much do you want to put it on the first line reviewers who should have caught it?

Sounds like you want to say, in one way or another, "next time I want details like this fixed before it gets to me / goes out to review."

Doom Mathematic
Sep 2, 2008
Try pair programming with them?

fantastic in plastic
Jun 15, 2007

The Socialist Workers Party's newspaper proved to be a tough sell to downtown businessmen.
bottle it up for months and then unexpectedly explode on them in a meeting

Jaded Burnout
Jul 10, 2004


fantastic in plastic posted:

bottle it up for months and then unexpectedly explode on them in a meeting

Similar advice for celibates in training.

awesomeolion
Nov 5, 2007

"Hi, I'm awesomeolion."

Maybe they are overemphasizing speed? Racing to complete the task without taking their time cleaning it up type thing? If that's the case then being like "I'd rather this takes you an extra three hours and has 4 revisions instead of 30" might help.

Keetron
Sep 26, 2008

Check out my enormous testicles in my TFLC log!

To me it seems not like a problem of people not willing to learn but an environment focusing on the wrong incentives.
About the tests is simple: Writing tests is part of the code, if you do not include tests, your code is not finished and a PR is request for edit. However, if your request carries less weight than the PM/PO/SM being happy with a quick PR being put in, people will go for the fast PR.
About the coding style things, again, if speed is more important than style, people will go for style.
Basically the problem is that you are not the one who has the most leverage. Find out who does and convince him/her to change her/his ways.

Good Will Hrunting
Oct 8, 2012

I changed my mind.
I'm not sorry.
I thought TMA was a TL at a Major Good Company. Yikes, if the coding practices are as bad as described in that post.... :ohdear: That's not even language nits, that's like engineering post-school 101.

Keetron
Sep 26, 2008

Check out my enormous testicles in my TFLC log!

Good Will Hrunting posted:

I thought TMA was a TL at a Major Good Company. Yikes, if the coding practices are as bad as described in that post.... :ohdear: That's not even language nits, that's like engineering post-school 101.

Or viewed from the other side: that is the worst he could find and thus posts about it. More likely he doesn't get the big bads because of the small bads.

TooMuchAbstraction
Oct 14, 2012

I spent four years making
Waves of Steel
Hell yes I'm going to turn my avatar into an ad for it.
Fun Shoe

Keetron posted:

Or viewed from the other side: that is the worst he could find and thus posts about it. More likely he doesn't get the big bads because of the small bads.

Yeah, like I said it's very rare for me to have to throw something back as fundamentally wrong. I don't really feel like these are people who need to go back to school, they just need more proofreading and care/attention.

It also bears mentioning that some of the people contributing software on my team are not formally-trained software engineers. My team does hybrid science/software work, so we have people who did other stuff in college than take three years of software deep dive courses, and their industry experience is at a lab bench rather than a desk. Otherwise, the main issue the team has is that it's very junior.

awesomeolion posted:

Maybe they are overemphasizing speed? Racing to complete the task without taking their time cleaning it up type thing? If that's the case then being like "I'd rather this takes you an extra three hours and has 4 revisions instead of 30" might help.

I've talked about this and posted a short screed for people to read. We'll see how much, if any, of a difference it makes.

Emphasizing speed is certainly an aspect of it for some people on the team, and I've talked with those people about it, but haven't made much progress. Some people just like to go fast and break things. Others though I kind of get the impression are relying on the reviewers to be thorough and to tell them what to do, maybe?

Keetron posted:

To me it seems not like a problem of people not willing to learn but an environment focusing on the wrong incentives.

I do have plenty of leverage on this team. The PM is on my side, because we're at a stage in the product development cycle where we're taking a multi-year view instead of a next-quarter view -- so basically this is exactly the wrong time to be trading product quality for execution speed. Moreover I am a de facto mandatory reviewer for every change in two of our primary languages because I'm the only person on the team who's passed the readability process in those languages (which is a whole separate issue). So I can gatekeep until California floats into the Pacific and there's not much any individual dev can do about it (not that they're complaining about the gatekeeping I'm doing, mind). I just wish I didn't have to gatekeep so heavily.

I'm contemplating telling people "I'm not going to bother reviewing your CL until you sign off on it saying that you've reread it and think it's ready for review." The problem with doing that is twofold: first, it's assuming they aren't already doing that (I have no idea either way), and second, it'd be really depressing if they started signing off and the quality didn't improve. :(

Keetron
Sep 26, 2008

Check out my enormous testicles in my TFLC log!

So you and the PM are all about quality over speed, but do the people:
a) know this?
b) know what is understood when talking about quality?

I think you are doing great and gate-keeping is a nasty, ungrateful and necessary job but I am missing the part where the developers *want* quality software to be written. Right now I get the impression they want the story to be removed from their name and taking the unavoidable review comments for granted but at least it is no longer in progress in their name.

Look, I have no formal education either and I struggle with a lot of basic concepts, but at least I am aware that naming things is hard and writing tests with reasonable coverage that are also fast is also hard. What you mentioned about the boolean, I can see it is ugly but cannot see why. I would rather have a bool have true as a default state, but I doubt that is the issue.
Final point: if someone does not want to learn, cannot take critisism and views code he wrote as *his own*, he has no business being a software engineer.

Achmed Jones
Oct 16, 2004



Keetron posted:

I can see it is ugly but cannot see why. I would rather have a bool have true as a default state, but I doubt that is the issue.

Using conditionals when the ! operator will do is the bad part. Sometimes you’ll also see “!!” to cast a value to a boolean when you want to cast but not negate.

taqueso
Mar 8, 2004


:911:
:wookie: :thermidor: :wookie:
:dehumanize:

:pirate::hf::tinfoil:

foo = False if bar else True
vs
foo = ! bar

It makes it more readable because you don't have to parse the if. I'd rather see a cast than !! though.

vonnegutt
Aug 7, 2006
Hobocamp.
I dealt with this a little at my current company. We have a contractor who writes just the ugliest code sometimes, and it's mainly because unlike the in-house team, they're hourly and feel they must post code every day. Which isn't true, but so far we have been unable to convince them of that.

I've taken a multi-part approach to code review. First, if it's something as huge as missing tests, that gets thrown back as soon as I discover it. I'm not going to review code that's incomplete, and I say so. "This feature is incomplete without tests. Please let me know once you've added them and I will review the code then."

For small in-line errors I mark the first one, post a fix ("Please use foo = !bar instead for setting bools"), explain why, and mark any others of the same kind with "Fix bool (see above)" or similar. Then I will add a comment on the whole check in saying something like "Please proofread for code such as (line number) before submitting code in the future. This kind of sloppiness has become a pattern and shouldn't be checked-in in the future." The key is to categorize the particular sloppiness so it's clear what you don't want to see again. Otherwise, it can come off as random nit-picks and will likely be ignored for the future. This also flags to others to look for this in future code reviews.

For bad naming, long functions, etc, I mark all of those in line, with explanations and/or links to external sources. It's become clear that sometimes the ugly code is just because they never actually learned the conventions, and once it's explained they're happy to oblige.

So far, it's felt like death by a thousand cuts, as soon as our dev learns not to write ugly indentations, they've figured out another way to drive me crazy. But I can see progress happening, so I continue. I've decided to pick my battles and let certain ugliness stay, but when I can see a clear pattern (like the bool thing) I try to call it out and make an example of it.

Che Delilas
Nov 23, 2009
FREE TIBET WEED

Keetron posted:

So you and the PM are all about quality over speed, but do the people:
a) know this?
b) know what is understood when talking about quality?

These are important. TMA, if you haven't done this yet, I suggest a meeting (though I'm usually loathe to do so) wherein you explain the direction you want to take the dev team(s), go over some guidelines and lay down a couple hard rules. I know you've written something but people don't read things - they get distracted, they are busy and forget, etc. Make the meeting participatory. Show examples and ask them to offer ideas of how to make things better. Emphasize that nobody's doing anything "wrong" (except for skipping unit tests - that poo poo don't fly), just that the emphasis is changing (quality over speed).

If you don't have a document in your dev wiki called "how to make a pull request," make one and go over it in the meeting, firmly suggest that they have it open side-by-side when they make their PRs.

If we're talking about a lack of training, maybe your company can help remedy that. Do they have a training budget? At the very least there are books and videos out there that talk about quality, clean code, that kind of thing, that you should be able to expense. Maybe hold regular lunch-and-learns where you teach a specific concept or two in-depth.

If these things sound like a pain in the rear end, that's because they are. But you're already in pain, and keep in mind that teaching is pretty much the best way to learn something, so you're going to improve yourself as a direct result.

Keetron posted:

Final point: if someone does not want to learn, cannot take critisism and views code he wrote as *his own*, he has no business being a software engineer.

Seconded. If all else fails, those people who refuse to learn and refuse to follow the rules you set down, your recommendation to your bosses should be that they get let go, because they drag down the whole endeavor.

minato
Jun 7, 2004

cutty cain't hang, say 7-up.
Taco Defender
If devs are writing a lot of code but not modifying other devs' code, then they're not going to see the value of some of your suggestions. Poor names, lovely data structures, and long functions work just fine for them because it's already in their head and 100% grokked. They'll see the value once they have to read some similar code that someone else wrote, or come back to their own code in 6 months time.

Re: testing, I used to either skip tests or write them at the end (usually a simple best-case-scenario "car drives across bridge on a sunny day with no wind" test). This was because I didn't know the value of testing, and didn't know how to write testable code (e.g. injecting dependencies, writing Mocks/Fakes/etc), and didn't know how to design tests. I got turned around when our team did a TDD course. We didn't end up adopting TDD for everything, but it gave us the Testing Is Good religion, and that we needed to think about testing up-front when writing code.


TooMuchAbstraction posted:

Throwing together six unrelated things into one gigantic change instead of breaking them out into smaller ones.
To be honest, this is something I still struggle with myself. I often go in to fix one specific thing and end up cleaning up a lot of other stuff along the way, like fixing formatting or improving variable names.

I know the right thing to do is to split it out into a separate PR, but sometimes there's strong friction against that: either the VCS tooling (or its crappy UX) doesn't easily allow you to split these changes out, or the process of getting such PRs approved is onerous.

Some mitigations:
- learning the tooling can help. E.g. I just learned about "git add -p" which lets you interactively stage parts of a file change, letting you split up commits better. Learning how to quickly make a new branch, submit a small fix, and fire off a PR also helps.
- Allow PRs with a random grab bag of improvements, as long as they only include refactors that don't require new tests.
- Make sure the code-review turnaround time for quality fixes is small. Helps if you've got CI running automated tests.


Edit: Linters can also help, e.g. they can call out over-long functions, function complexity, and overly-nested loops. Then you're no longer the bad guy, it's the linter policy doing the blaming.

minato fucked around with this message at 19:37 on Jan 15, 2019

TooMuchAbstraction
Oct 14, 2012

I spent four years making
Waves of Steel
Hell yes I'm going to turn my avatar into an ad for it.
Fun Shoe

minato posted:

If devs are writing a lot of code but not modifying other devs' code, then they're not going to see the value of some of your suggestions. Poor names, lovely data structures, and long functions work just fine for them because it's already in their head and 100% grokked. They'll see the value once they have to read some similar code that someone else wrote, or come back to their own code in 6 months time.
Yeah, I only really internalized this lesson myself when I had a past job that involved rehabilitating a substantial codebase that had been written entirely by academics who had no formal software development training :ohno:

quote:

Re: testing, I used to either skip tests or write them at the end (usually a simple best-case-scenario "car drives across bridge on a sunny day with no wind" test). This was because I didn't know the value of testing, and didn't know how to write testable code (e.g. injecting dependencies, writing Mocks/Fakes/etc), and didn't know how to design tests. I got turned around when our team did a TDD course. We didn't end up adopting TDD for everything, but it gave us the Testing Is Good religion, and that we needed to think about testing up-front when writing code.
We have a strong company culture of being able to say "Where are the tests?" and basically blocking code review until tests get written. Part of this is just that my tendency when faced with a new code review is to dive in and start reviewing it, rather than making sure it checks the various checkboxes it's supposed to and blocking code review until it does.

quote:

To be honest, this is something I still struggle with myself. I often go in to fix one specific thing and end up cleaning up a lot of other stuff along the way, like fixing formatting or improving variable names.

I know the right thing to do is to split it out into a separate PR, but sometimes there's strong friction against that: either the VCS tooling (or its crappy UX) doesn't easily allow you to split these changes out, or the process of getting such PRs approved is onerous.
Oh yeah, this is a problem for me too. What I do (and what I'm trying to convince the rest of my team to do) is to stop, say "wait, this change has gotten huge, what's an easily-separable part I can submit that doesn't do anything on its own, but lays groundwork for the rest of it?", and repeat that until the "change that actually does something" is of a manageable size. I think I need to sell people on the fact that this will actually make their code reviews turn around faster and improve the quality of their code. People are a lot more likely to quickly review several 100-line changes than they are to block out the time required to review a 600-line change.

Che Delilas posted:

These are important. TMA, if you haven't done this yet, I suggest a meeting (though I'm usually loathe to do so) wherein you explain the direction you want to take the dev team(s), go over some guidelines and lay down a couple hard rules. I know you've written something but people don't read things - they get distracted, they are busy and forget, etc. Make the meeting participatory. Show examples and ask them to offer ideas of how to make things better. Emphasize that nobody's doing anything "wrong" (except for skipping unit tests - that poo poo don't fly), just that the emphasis is changing (quality over speed).

You're probably right. I think part of my resistance to this has been that it means formally recognizing that our team has a problem with code quality, which isn't something that anyone likes to admit. But the first step of fixing a problem is admitting that it exists. :(

Thanks for the suggestions, everyone!

Keetron
Sep 26, 2008

Check out my enormous testicles in my TFLC log!

TooMuchAbstraction posted:

Thanks for the suggestions, everyone!
Anytime, bro/sis!

Jaded Burnout
Jul 10, 2004


As much as the particular execution of it winds me up, Rubocop (or your language equivalent) does a decent job of automatically enforcing your org’s styleguide.

Becoming a highly paid human code parser ain’t a great life.

TooMuchAbstraction
Oct 14, 2012

I spent four years making
Waves of Steel
Hell yes I'm going to turn my avatar into an ad for it.
Fun Shoe
To be clear, we have gobs of automated enforcement, and our existing test coverage actually is pretty good across most of the codebase. The linters, presubmit tests, and other automated analyses save a lot of work. But they can't catch everything.

LLSix
Jan 20, 2010

The real power behind countless overlords

TooMuchAbstraction posted:

[*]Confusingly-named variables
[*]Comments that assume you already have context on the problem the code is solving.
These are endemic problems everywhere I've ever worked. Not just junior developers but even my fellow code-reviewers don't do well at these parts of the job. If someone has to get their code past me I can get them to start writing good comments in 12 - 18 months, but its a long, slow process. I'm always on the look out for other things I can point at to help them realize it is important. Do you have any links or guides you could recommend?

asur
Dec 28, 2012
Can't you put an automatically enforced requirement that X% of code be covered by tests. I think I'd start there to gatekeep writing tests. Fuxk reviewing code that isn't test. That's such a huge waste of everyone's time.

Doh004
Apr 22, 2007

Mmmmm Donuts...

Doom Mathematic posted:

Try pair programming with them?

This is correct.

necrobobsledder
Mar 21, 2005
Lay down your soul to the gods rock 'n roll
Nap Ghost
Sounds like you may be a candidate for being a mentor to them. But if you keep the adversarial attitude this will not have a good ending for anyone.

JawnV6
Jul 4, 2004

So hot ...

necrobobsledder posted:

Sounds like you may be a candidate for being a mentor to them.

Why, just imagine! In a few years he could have some formal structure over them like being a TL.

New Yorp New Yorp
Jul 18, 2003

Only in Kenya.
Pillbug

asur posted:

Can't you put an automatically enforced requirement that X% of code be covered by tests. I think I'd start there to gatekeep writing tests. Fuxk reviewing code that isn't test. That's such a huge waste of everyone's time.

Code coverage does not guarantee that code is bug-free or well-tested, just that someone has attempted to test it. Code coverage requirements are just asking to be gamed.

Keetron
Sep 26, 2008

Check out my enormous testicles in my TFLC log!

New Yorp New Yorp posted:

Code coverage does not guarantee that code is bug-free or well-tested, just that someone has attempted to test it. Code coverage requirements are just asking to be gamed.

Which is why we have mutation testing, an awesome and confusing thing that when used right is super helpful. Unfortunately, it is hardly ever used right.

Adbot
ADBOT LOVES YOU

asur
Dec 28, 2012

New Yorp New Yorp posted:

Code coverage does not guarantee that code is bug-free or well-tested, just that someone has attempted to test it. Code coverage requirements are just asking to be gamed.

Nothing gurantees well tested or bug free code. It does however remove the excuse that you'll write tests later by setting a standard that a commit needs to include an acceptable level of testing.

Anything can be gamed, and the same argument can be made for almost all automatic validation. You can't prevent purposefully bad actors, those people should be fired, but you can provide incentives to do things correctly and if you have to write tests to get a commit through then most people will write the tests they normally would have rather than waste time trying to game a system.

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