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
Pollyanna
Mar 5, 2005

Milk's on them.


Skandranon posted:

Possibly? How did the conversation go? If the lead is trying to instill in you that intuition, how would you rather have been talked to? It's not necessarily a bad thing.

It’s about how it reflects on me as an engineer. Having occurrences like these is a signal that I’m not as good as I should be, right?

CPColin posted:

Yeah, if they have certain expectations and didn't tell the newbie about them, that's probably on them. On the other hand, it can be a good instinct to see if the backend functionality can be ripped out when the frontend functionality that used it is ripped out. At my last job, I tried to get the rest of the team on board with at least writing down a reminder to file a clean-up ticket later and would gently remind people of that philosophy during code reviews. Of course, if the structure of the code is such that you can't easily tell when backend stuff is unused, that puts it back on the veterans to educate the newbie.

I’ve been in the “rip all this poo poo out!!!” mindset before as a newbie and learned to not give in to that, cause more often than not it just confuses people and wastes time. Major refractors and removals like that should be done with a good understanding of the entire system lest it gently caress things up even worse. I’m bearish on doing that without exact prompting.

Pollyanna fucked around with this message at 20:40 on Dec 12, 2017

Adbot
ADBOT LOVES YOU

Hughlander
May 11, 2005

Pollyanna posted:

Got a ticket to replace one piece of UI with two other indicators and I get dinged and a talking to from the team lead for not completely ripping out every bit of the now unused internal functionality from the codebase. :negative: Am I just missing some sort of intuition that all other developers have or something?

To clarify, the ticket just says “remove Old Thing icon” and doesn’t make mention of removing the entire functionality.

I'd expect something like that to be documented in DoD or WoW. But generally "Don't keep unused code in code base." is a valid part of refactoring something. Again though it should be documented and you can treat this as documented. WebStorm/PHPStorm will help you there by giving you these nice greyed out blocks that you can than refactor safe delete away...

Skandranon
Sep 6, 2008
fucking stupid, dont listen to me

Pollyanna posted:

It’s about how it reflects on me as an engineer. Having occurrences like these is a signal that I’m not as good as I should be, right?

Well... you always want to be improving, so you should never actually BE who you could be, it's a goal. The job of a team/tech lead is to have these conversations, to guide the more junior developers in the directions he/the company wants. It doesn't necessarily mean you are significantly worse than any other random developer. Considering you just started, he SHOULD be spending more time with you to let you know how things should be done. If he was yelling, that's bad, but if it was just "try to focus more on overall clean code", I wouldn't be upset, just take that into consideration next time you do a similar task. Maybe you don't know HOW to properly remove everything, but investigate & ask.

CPColin
Sep 9, 2003

Big ol' smile.

Pollyanna posted:

It’s about how it reflects on me as an engineer. Having occurrences like these is a signal that I’m not as good as I should be, right?

Nahh, don't worry about it. It's still your first month, so you're all still getting used to each other's expectations.

Pollyanna posted:

I’ve been in the “rip all this poo poo out!!!” mindset before as a newbie and learned to not give in to that, cause more often than not it just confuses people and wastes time. Major refractors and removals like that should be done with a good understanding of the entire system lest it gently caress things up even worse. I’m bearish on doing that without exact prompting.

Yeah, if I were mentoring a new hire who took a low-point ticket to remove some little UI thing and spent way longer on it than I expected because they took a bunch of time to rip out unused stuff, I'd kick myself for not setting the right expectations and not checking in early enough to avoid the extra work. But I'd try to frame the correction as something positive, instead of a "ding".

Edit: The remote situation probably makes it harder to keep a tight feedback loop, though. More stuff you all can look forward to getting used to! Excitement!

AWWNAW
Dec 30, 2008

Don’t take every PR comment as a “ding” or slight. The less personally you take code reviews, the better.

That said, if you’re making changes that obsolete other code then it’s usually a good idea to remove the obsolete code lest it confuse your future self.

Skandranon
Sep 6, 2008
fucking stupid, dont listen to me

CPColin posted:

Edit: The remote situation probably makes it harder to keep a tight feedback loop, though. More stuff you all can look forward to getting used to! Excitement!

I forgot about this as well. I'd take it as a good sign your lead is making the effort to do this. I'd be more worried if he didn't care.

Pollyanna
Mar 5, 2005

Milk's on them.


I just don’t want to get fired for not being psychic.

Skandranon
Sep 6, 2008
fucking stupid, dont listen to me

Pollyanna posted:

I just don’t want to get fired for not being psychic.

Well, that might still happen, you may have a psychopath for a boss. But at a month in, I would be hard pressed to be upset you did a task as written, but not as I imagined it. All you can do is the best you can. Try not to kill yourself with stress.

Hughlander
May 11, 2005

Pollyanna posted:

I just don’t want to get fired for not being psychic.

So maybe they should tell you these things. Like maybe given that you're remote they'll tell you through some sort of text. Like maybe a PR comment. Or maybe a Slack PM. Say how did the team lead contact you again? Or to put it more bluntly, what would have made you happy here?

Pollyanna
Mar 5, 2005

Milk's on them.


Hughlander posted:

So maybe they should tell you these things. Like maybe given that you're remote they'll tell you through some sort of text. Like maybe a PR comment. Or maybe a Slack PM. Say how did the team lead contact you again? Or to put it more bluntly, what would have made you happy here?

The entirety of a unit of work to be done, and no more, should be captured in a single ticket. The contract is to complete the unit of work specified in the requirements to the best of the engineer’s ability. Don’t ask for a subset of work in a ticket then turn around at the end of it and pile on more poo poo I didn’t know about. If you want me to do more, make a new ticket or get it all down at the beginning.

Skandranon posted:

Well, that might still happen, you may have a psychopath for a boss. But at a month in, I would be hard pressed to be upset you did a task as written, but not as I imagined it. All you can do is the best you can. Try not to kill yourself with stress.

This is a crucial moment, though, at the beginning. The more reliable you are, the better people feel about having hired you, and that sets the stage for the next few years.

AWWNAW posted:

Don’t take every PR comment as a “ding” or slight. The less personally you take code reviews, the better.

Reason I call it a ding is that this sort of confusion and clarification shouldn’t happen in the first place, and in the context of my previous comment is a kind of judgment on the new employee. “If you’re as good as you say, why did you have to be corrected”, as such.

quote:

That said, if you’re making changes that obsolete other code then it’s usually a good idea to remove the obsolete code lest it confuse your future self.

I’m making changes that obsolete the code I immediately touched, but it’s possible that code is integral to some other functionality that isn’t obsolete as per requirements. If I pull on that string believing it to not be load-bearing, I eat poo poo if it brings the house down.

Hughlander
May 11, 2005

Pollyanna posted:

The entirety of a unit of work to be done, and no more, should be captured in a single ticket. The contract is to complete the unit of work specified in the requirements to the best of the engineer’s ability. Don’t ask for a subset of work in a ticket then turn around at the end of it and pile on more poo poo I didn’t know about. If you want me to do more, make a new ticket or get it all down at the beginning.


So did the single ticket specify that tests need to be written? Did the single ticket specify that it had to pass a code review? Did the single ticket specify that documentation as it exists should be upgraded? Did the single ticket specify that it should/should not be test in integration tests? Did the single ticket specify that unused code should not be checked in? I'd assume not because those things are common in every single ticket and are usually documented elsewhere as a Definition of Done or Ways of Working. Expecting to paste in a giant list in every single ticket instead of a single documented spot is completely stupid.

Pollyanna
Mar 5, 2005

Milk's on them.


Hughlander posted:

So did the single ticket specify that tests need to be written? Did the single ticket specify that it had to pass a code review? Did the single ticket specify that documentation as it exists should be upgraded? Did the single ticket specify that it should/should not be test in integration tests? Did the single ticket specify that unused code should not be checked in? I'd assume not because those things are common in every single ticket and are usually documented elsewhere as a Definition of Done or Ways of Working. Expecting to paste in a giant list in every single ticket instead of a single documented spot is completely stupid.

If we did that and we had it captured as a How We Work then sure, but would you really expect someone who is still getting used to the codebase to rip out code at their discretion, without forewarning of what can bring the whole thing down? I’d rather not do it if it means I avoid breaking poo poo.

Skandranon
Sep 6, 2008
fucking stupid, dont listen to me

Pollyanna posted:

The entirety of a unit of work to be done, and no more, should be captured in a single ticket. The contract is to complete the unit of work specified in the requirements to the best of the engineer’s ability. Don’t ask for a subset of work in a ticket then turn around at the end of it and pile on more poo poo I didn’t know about. If you want me to do more, make a new ticket or get it all down at the beginning.

This is a crucial moment, though, at the beginning. The more reliable you are, the better people feel about having hired you, and that sets the stage for the next few years.

I think your defensiveness over this is subverting your stated goal of being reliable. There is not a single perfect way to work. You need to figure out how they work and try to fit with that as best possible. Turning over the applecart in your first month and telling everyone their tickets suck (even if 100% true) isn't going to be well received and then you will be perceived as unreliable and then fired (your doomsday scenario).

Yes you should be worried about things at the beginning, but I think you are worried about the WRONG things. Completing tickets perfectly is not one of them. Being a valuable team member is, which includes a lot more than just completing tickets. You need to work on the soft skills more, especially since you are remote, which puts you at a disadvantage deploying whatever soft skills you have.

You can lose every battle and still win the war. Stop focusing on each little thing and try to see a larger picture.

KoRMaK
Jul 31, 2012



Pollyanna posted:

If we did that and we had it captured as a How We Work then sure, but would you really expect someone who is still getting used to the codebase to rip out code at their discretion, without forewarning of what can bring the whole thing down? I’d rather not do it if it means I avoid breaking poo poo.

Was that what your manager or lead was trying to tell you? To get comfortable making that kind of call?

Hughlander
May 11, 2005

Pollyanna posted:

If we did that and we had it captured as a How We Work then sure, but would you really expect someone who is still getting used to the codebase to rip out code at their discretion, without forewarning of what can bring the whole thing down? I’d rather not do it if it means I avoid breaking poo poo.

Which was why in the part of my you didn't quote that I said I would expect it to be documented in the DoD or WoW and that it is a regular thing to be documented there. And if that's part of the DoD then yes I would expect it. Otherwise the story isn't done. How do you know that things still work? The tests pass. UAT succeeds. etc...

If there's no documented DoD than that's something you can bring up. Maybe there is and you haven't been pointed to it yet?

LLSix
Jan 20, 2010

The real power behind countless overlords

Hughlander posted:

Which was why in the part of my you didn't quote that I said I would expect it to be documented in the DoD or WoW and that it is a regular thing to be documented there. And if that's part of the DoD then yes I would expect it. Otherwise the story isn't done. How do you know that things still work? The tests pass. UAT succeeds. etc...

If there's no documented DoD than that's something you can bring up. Maybe there is and you haven't been pointed to it yet?

What's a DoD or WoW look like? I've never heard of either before.

CPColin
Sep 9, 2003

Big ol' smile.
"DoD" is "Definition of Done" and on both the Scrum teams I've been on, it's been, "The work is done!" because nobody ever feels the need to push it farther. I wish the Scrum Guide recommended an example that teams should start from, although that's not their style. I kept trying to wheedle a stronger definition out of my teammates by using the question, "How do I know when I'm allowed to commit?" over and over. It didn't really work.

Volguus
Mar 3, 2009

CPColin posted:

"DoD" is "Definition of Done" and on both the Scrum teams I've been on, it's been, "The work is done!" because nobody ever feels the need to push it farther. I wish the Scrum Guide recommended an example that teams should start from, although that's not their style. I kept trying to wheedle a stronger definition out of my teammates by using the question, "How do I know when I'm allowed to commit?" over and over. It didn't really work.

The Definition of Done for feature X is specified by the product owner and is written down in the backlog. If there is no PO nor a backlog, meh ... whatever goes i guess.

Hughlander
May 11, 2005

LLSix posted:

What's a DoD or WoW look like? I've never heard of either before.

Sample Definition of Done (For a team with internal customers)
Appropriate Documentation and removal of outdated docs
Appropriate Tests are written (Functional, Unit, Integration) or Tech Debt stories relating to why such tests are impossible are written.
Passes Code/Test/Documentation Review to the Standards of the Team
Passes Functional Review with a user of the system.
Merged To Master
Features Announced to Customers

Ways of Working Docs are a bit more specific to the team but have seen things like:
Define Swim Lane Limits
How to break a User Story down to Sub Tasks
Pointing and Refinment agreements (What value is max complexity before a User Story needs to be broken down further)
What's a Spike? What's the outcome of a Spike?
etc...

Pixelboy
Sep 13, 2005

Now, I know what you're thinking...

CPColin posted:

The real horror is how much work one of my other coworkers had to do to get the right version of the compiler onto TFS!

Edit: T-25 minutes until my weekly one-on-one. Currently getting antsy for no reason!

Its a drop down menu. A bloody drop down menu.

I've done it... I am lost as to why people would think it's hard, aside from not being aware of it and doing backflips to re-implement it.

CPColin
Sep 9, 2003

Big ol' smile.
I think it was something about how the build process was set up on TFS. It might be doing something similar to when a Java project uses Maven to call an Ant script and suddenly there's extra layers of configurations and tool dependencies. Probably didn't help that all the StackOverflow answers pointed to a complicated upgrade process.

Volguus posted:

The Definition of Done for feature X is specified by the product owner and is written down in the backlog. If there is no PO nor a backlog, meh ... whatever goes i guess.

Those might be "acceptance criteria" you're talking about. The DoD is supposed to be decided by the Dev Team alone (i.e., not the PO), according to the Scrum Guide™ and is supposed to apply, regardless of what's being worked on. It's supposed to get revised during the Sprint Retrospective, but neither team I've been on has ever concerned itself with doing so.

CPColin fucked around with this message at 00:40 on Dec 13, 2017

Carbon dioxide
Oct 9, 2012

I've written a draft DoD recently and then discussed it with my fellow team members to get the first version of an useful document.

Writing a DoD is not easy, because you really want everything written down in it to be measurable in some way, so that you could actually use it as a checklist, and more importantly, use it to review the process if some release went horribly wrong.

So for instance "the code needs to be tested" is not good enough for a DoD. What kind of tests? Unit tests? Integration tests? End user tests? All of the above?

And even if you add integration tests, for example, as a point in the DoD, well, it immediately gets a label "when applicable", because there's no point in integration tests if the story was about making some standalone piece of code. So, how do you make this measurable? You end up with something meaningless like "a majority of the team's developers should agree it has been sufficiently tested".

I don't really know, making a good DoD is hard, and I think the only way to do it is just make some kind of DoD, seriously use this as a checklist (this requires a lot of discipline, and no, I haven't ever seen a team who uses it in this way), and then update it where necessary.

One good realisation we had when writing the DoD was that while the DoD itself should be a checklist of measurable points, in the end, the DoD reflects team values, values about standards, about what code should look like, about how important preventing bugs is to your team, and there's nothing preventing you from writing down these values and adding them as an explanation or attachment to the DoD, so that the intent of the points, instead of the exact metric, becomes clear. And that makes it a more useful document to look at when deciding how to tackle a complicated story.

Hughlander
May 11, 2005

Carbon dioxide posted:

I've written a draft DoD recently and then discussed it with my fellow team members to get the first version of an useful document.

Writing a DoD is not easy, because you really want everything written down in it to be measurable in some way, so that you could actually use it as a checklist, and more importantly, use it to review the process if some release went horribly wrong.

So for instance "the code needs to be tested" is not good enough for a DoD. What kind of tests? Unit tests? Integration tests? End user tests? All of the above?

And even if you add integration tests, for example, as a point in the DoD, well, it immediately gets a label "when applicable", because there's no point in integration tests if the story was about making some standalone piece of code. So, how do you make this measurable? You end up with something meaningless like "a majority of the team's developers should agree it has been sufficiently tested".

I don't really know, making a good DoD is hard, and I think the only way to do it is just make some kind of DoD, seriously use this as a checklist (this requires a lot of discipline, and no, I haven't ever seen a team who uses it in this way), and then update it where necessary.

One good realisation we had when writing the DoD was that while the DoD itself should be a checklist of measurable points, in the end, the DoD reflects team values, values about standards, about what code should look like, about how important preventing bugs is to your team, and there's nothing preventing you from writing down these values and adding them as an explanation or attachment to the DoD, so that the intent of the points, instead of the exact metric, becomes clear. And that makes it a more useful document to look at when deciding how to tackle a complicated story.

You got it exactly right I think. Just keep it up. The way that I've handled it on multiple team in the past is to push the "where applicable" type things to the 'code' review. You review the code, the documentation, the tests. And you reject the change if you can't come to an agreement with the appropriateness of the code/documentation/tests.

KoRMaK
Jul 31, 2012



Carbon dioxide posted:

I've written a draft DoD recently and then discussed it with my fellow team members to get the first version of an useful document.

Writing a DoD is not easy, because you really want everything written down in it to be measurable in some way, so that you could actually use it as a checklist, and more importantly, use it to review the process if some release went horribly wrong.

So for instance "the code needs to be tested" is not good enough for a DoD. What kind of tests? Unit tests? Integration tests? End user tests? All of the above?

And even if you add integration tests, for example, as a point in the DoD, well, it immediately gets a label "when applicable", because there's no point in integration tests if the story was about making some standalone piece of code. So, how do you make this measurable? You end up with something meaningless like "a majority of the team's developers should agree it has been sufficiently tested".

I don't really know, making a good DoD is hard, and I think the only way to do it is just make some kind of DoD, seriously use this as a checklist (this requires a lot of discipline, and no, I haven't ever seen a team who uses it in this way), and then update it where necessary.

One good realisation we had when writing the DoD was that while the DoD itself should be a checklist of measurable points, in the end, the DoD reflects team values, values about standards, about what code should look like, about how important preventing bugs is to your team, and there's nothing preventing you from writing down these values and adding them as an explanation or attachment to the DoD, so that the intent of the points, instead of the exact metric, becomes clear. And that makes it a more useful document to look at when deciding how to tackle a complicated story.

Screw this, I'm going back to writing PHP. I'm a software developer, not a gat dang structural engineer building bridges

Carbon dioxide posted:

You end up with something meaningless like "a majority of the team's developers should agree it has been sufficiently tested".
I think this is fine, if the team agrees then its O.K.

Volmarias
Dec 31, 2002

EMAIL... THE INTERNET... SEARCH ENGINES...

Pollyanna posted:

I just don’t want to get fired for not being psychic.

I'll let you in on a dirty secret that it took me a long time to learn. It's true of every industry, but especially for computer touchers:

Everyone is terrible.

I'm terrible, and so is everyone else posting here.I It'st a matter of degrees. Don't sweat it, especially in a junior developer role. You have code reviews for a reason; if no one else asked about dependant code, they should shoulder some of this psychic blame as well.

Regardless of whether it was constructive criticism or not, take it to heart, and try to incorporate it where you can. If your codebase is so fragile that you're afraid that it's load bearing string, ask someone before you tug at a knot. Try to track down for yourself how a thing could possibly call it. Ensure that code coverage exists, and if it doesn't, add it. Tests should help you know whether or not that string is load bearing if it suddenly goes away.

Seriously though, don't beat yourself up. You're fine.

Clanpot Shake
Aug 10, 2006
shake shake!

Volmarias posted:

Everyone is terrible.

And the corollary - All Code is Bad. Every line of code in existence has to be maintained, tested, read, and understood. Code approaching goodness will do one of these things well, but code like that is exceedingly rare. Most code is written by people who either don't know what they're doing, or don't care about it past getting their PR approved (if that hurdle even exists) or their contract ends. Our days are spent not writing Good Code, but sifting through the litterbox that is The Codebase, holding our nose and trying not to step in buried turds while we do our work, just wishing someone would spend the time to clean the damned thing or throw it away and start fresh.

Pollyanna posted:

Got a ticket to replace one piece of UI with two other indicators and I get dinged and a talking to from the team lead for not completely ripping out every bit of the now unused internal functionality from the codebase. :negative: Am I just missing some sort of intuition that all other developers have or something?

To clarify, the ticket just says “remove Old Thing icon” and doesn’t make mention of removing the entire functionality.

All code is bad, deleting code is removing badness, therefore deleting code is good. Delete all the code.

Volguus
Mar 3, 2009

CPColin posted:

I think it was something about how the build process was set up on TFS. It might be doing something similar to when a Java project uses Maven to call an Ant script and suddenly there's extra layers of configurations and tool dependencies. Probably didn't help that all the StackOverflow answers pointed to a complicated upgrade process.


Those might be "acceptance criteria" you're talking about. The DoD is supposed to be decided by the Dev Team alone (i.e., not the PO), according to the Scrum Guide™ and is supposed to apply, regardless of what's being worked on. It's supposed to get revised during the Sprint Retrospective, but neither team I've been on has ever concerned itself with doing so.

You're right. I forgot. The broader scope is usually defined by the team and usually promptly forgotten. The 'acceptance criteria' is all that remains of a DoD after some period of time.

Bongo Bill
Jan 17, 2012

Try to overcome your fear of being corrected. This may be difficult given your background. In a healthy working environment, being notified of an error on your part is something to be welcomed, because it means it was caught sooner rather than later and because you can learn from it. Competent developers make mistakes all the time. The most important job skill we possess is knowledge of how to minimize the ways our constant and unpreventable fuckups can ruin what we're working on.

Ghost of Reagan Past
Oct 7, 2003

rock and roll fun
A number of years ago, I sat down with my dissertation advisor to work over a paper I'd sent her. She ripped it to shreds. Absolutely tore apart the paper. My argument was lousy. Some of my formalism was hosed up in a subtle but embarrassing way. I missed several key, obvious references. And so on.

At the end of the meeting, after going through all this: "Good job! Keep up the good work." She meant it genuinely, because fundamentally, even as bad as the paper was, it was good work. poo poo is hard. poo poo's hard in the software world, too.

When you read something, you don't see all the work that went into it. All the criticism people put it through. All the rewrites, all the stupid bullshit bugs and mistakes. You've done it: you write some code you're confident is gonna work and it shits the bed immediately upon testing. Then you fix it, and it works. Then you deploy it and it breaks. So you fix it. That's literally what the job is. You're constantly fixing poo poo that idiots wrote, and because we're all idiots, you're gonna have to fix that poo poo, too. Making mistakes, even big ones, is not a big deal. Because everyone, from the greenest to the greatest, is gonna gently caress up. You're gonna misread a ticket in a glaringly obvious way. You're gonna forget part of process. You're going to forget a loving import. You'll forget a semicolon. You'll write a loop with an off by one error that destroys your database.

poo poo's hard. Don't sweat the mistakes.

Ghost of Reagan Past fucked around with this message at 05:07 on Dec 13, 2017

KoRMaK
Jul 31, 2012



Clanpot Shake posted:

And the corollary - All Code is Bad. Every line of code in existence has to be maintained, tested, read, and understood.

Gonna shorten your post to be the mantra that I instill in my team. Good code is code that can be maintained. that means someone, any reasonable programmer, can walk into and day 1 be able to 1) read and understand 2) make changes to to fix bugs 3) improve without breaking other stuff

No code is permanent, it is transient. it will never remain in its current form for all time.


There's a great rubyconf talk that I watch once a year to remind myself, and also pass around to others that highlights this. It's a combo of Sandi Metz teaching and Uncle Bob and some other stuff. Your code needs to be easily changed in the future, that means many seams - dependency injection. Easily readbale, at a blur test - e.g. not too many indents. Single Purpose classes that aren't more than probably 100 lines of code. Method names that are basically short descriptive actions. Got an if statement with 3 joiners in it (&& or ||) - then it should probably be moved into a method e.g. "can_do_a_thing?"


Good code is maintaible code. That means the important question is: what is maintainable code? Repeating myself here but, imo, its 1) obivous! 2)easily readable and understandable 3) can be altered to fix a bug without bringing down the whole house of cards 4) optional: hopefully has tests


Don't be clever, it makes other people hate you if they have to maintain your code (including your future self)
https://www.youtube.com/watch?v=LdWMcs9EEOE&t=10572s


e: oof, that thumbnail
ee: Better source, timestamped to 2h56minutes.

KoRMaK fucked around with this message at 18:02 on Dec 13, 2017

B-Nasty
May 25, 2005

KoRMaK posted:

There's a great rubyconf talk that I watch once a year to remind myself, and also pass around to others that highlights this. It's a combo of Sandi Metz teaching and Uncle Bob and some other stuff. Your code needs to be easily changed in the future, that means many seams - dependency injection. Easily readbale, at a blur test - e.g. not too many indents. Single Purpose classes that aren't more than probably 100 lines of code. Method names that are basically short descriptive actions. Got an if statement with 3 joiners in it (&& or ||) - then it should probably be moved into a method e.g. "can_do_a_thing?"

Some great tips.

I would add naming variables with long, descriptive names is also very useful. For example:

var isAuthorizedToCreateNewOrder = ...
var hasItemsInShoppingCart = ...
var isLocatedInCountriesThatWeServe = ...

leads to an improved understanding 30 lines down when the inevitable:

if(isAuthorizedToCreateNewOrder && hasItemsInShoppingCart && isLocatedInCountriesThatWeServe)

is used to control flow.

In a code review, if I see a variable with less than 3 syllables, and it's not a throwaway index in a loop, I'm going to question why abbreviating it was considered prudent.

Che Delilas
Nov 23, 2009
FREE TIBET WEED
var isNotMissingDisableFlag

KoRMaK
Jul 31, 2012



B-Nasty posted:

Some great tips.

I would add naming variables with long, descriptive names is also very useful. For example:

var isAuthorizedToCreateNewOrder = ...
var hasItemsInShoppingCart = ...
var isLocatedInCountriesThatWeServe = ...

leads to an improved understanding 30 lines down when the inevitable:

if(isAuthorizedToCreateNewOrder && hasItemsInShoppingCart && isLocatedInCountriesThatWeServe)

is used to control flow.

In a code review, if I see a variable with less than 3 syllables, and it's not a throwaway index in a loop, I'm going to question why abbreviating it was considered prudent.
I agree, but underscore instead of camel case.

Took me a second to get used to, but haveing vars lowercased with underscores separating words helps me read code way faster.

For instance, the first is preferable:
is_authorized_to_create_new_order

isAuthorizedToCreateNewOrder

It seems like your brain (my brain) just parses it faster

pigdog
Apr 23, 2004

by Smythe

Pollyanna posted:

It’s about how it reflects on me as an engineer. Having occurrences like these is a signal that I’m not as good as I should be, right?

YOU ARE NOT "GOOD".

You are not "bad" either.

What you, and everyone, is is being at a certain level at a thing (coding).

That level is not static, but you will improve it by learning (especially by doing) and experience, especially from failures.

It's a matter of static vs agile mindsets, which is an incredibly powerful metagame that drives our psychology, and frankly deserves its own thread.

Basically, if you have a static mindset, then you're either "good" at something, or when you run into a problem or criticism about something you think you're "good" at, then it must mean you're actually "bad" at it, and your focus shifts to hiding your "badness" and putting up an impression of "being good". And if that doesn't work, blame everything around you, and maintain that you didn't want the thing anyway. Etc.

INSTEAD, just accept that your skill level is dynamic and mutable. You may find that you're not as good at a thing as you thought, but you will improve if you put effort into it. Doing the thing and failing is not an indicator that you're "bad", but something that gives you experience in, and TRULY makes you better at the very thing. Without any need for pretending.

Don't waste time (outside of sales?) putting up a facade that you're "good", and heaven forbid someone finds out you're "actually" "bad". Accept that your skills are where they are, you don't know everything just like anyone, but find comfort in the fact that you CAN learn to do any goddamn thing better, by effort and experience and especially experience from failing.

Think of life as an RPG, where even if you lose a fight and don't get the loot, you still gain EXP and improve from that experience.

Edit: Similarly to whether you're "good" at development, you might ask yourself if you are physically fit? Say you're at 200 lbs. Compared to 700lb landwhales you're fit. As a Mr Olympia contestant, you're not. What you can do is compare your skill level to your own some time back. Am I getting "fitter", or better at coding, compared to where I was X months ago? It's the vector of where you're moving that's important - not the current level.

pigdog fucked around with this message at 11:16 on Dec 13, 2017

pigdog
Apr 23, 2004

by Smythe

KoRMaK posted:

Good code is maintaible code. That means the important question is: what is maintainable code? Repeating myself here but, imo, its 1) obivous! 2)easily readable and understandable 3) can be altered to fix a bug without bringing down the whole house of cards 4) optional: hopefully has tests
Talking about repeating, number one rule should be No Duplicate Code.

I swear, some 70% of bugs I've had to fix stem from there being two+ separate ways of doing some thing, and a later change modifying only one of them as the changer was unaware of the other(s).

Clanpot Shake
Aug 10, 2006
shake shake!

KoRMaK posted:

No code is permanent, it is transient. it will never remain in its current form for all time.

Code also lives way longer than you think it will. How many times have you seen "//TODO this is a hack, clean this up" that hasn't been touched in 5 years? At my last place we had a minor fire drill when all of our automated tests started failing during the holiday code freeze, when nothing new had been introduced. Turns out 10 years ago someone thought, "ten years is a sufficiently long time for this test data to be active," then 10 years roll by and all of the sudden it's no longer valid.

KoRMaK posted:

I agree, but underscore instead of camel case.

Took me a second to get used to, but haveing vars lowercased with underscores separating words helps me read code way faster.

For instance, the first is preferable:
is_authorized_to_create_new_order

isAuthorizedToCreateNewOrder

It seems like your brain (my brain) just parses it faster

This is absolutely a language thing. I have a mental record-skip when I see snake case because I don't work in a language like C.

ChickenWing
Jul 22, 2010

:v:

Clanpot Shake posted:

This is absolutely a language thing. I have a mental record-skip when I see snake case because I don't work in a language like C.

It's also a workplace thing. Always follow the style guidelines set out by your workplace. Luckily, this is usually tied to the language you use - I'm at a java shop, and snake case is punishable by 50 lashes.


also


Working in Development: All Code is Bad and Everyone is Terrible

B-Nasty
May 25, 2005

pigdog posted:

Talking about repeating, number one rule should be No Duplicate Code.

Of course, D.R.Y. can cut both ways also. Changing that reused function may have unintended consequences in a whole other area of code you weren't even dealing with. Being too strict with DRY can also lead to over-complicated or over-engineered functions that would be better as simple copy-pastes without unnecessary options or genericized concepts.

Like most things, it's a judgement call. If you feel like you're being 'tricky' or 'clever', that's probably when you should step back and rethink the implementation.

Volguus
Mar 3, 2009
There is only one rule that is true always in software development: Never ever have blanket rules, unless you're dealing with drunken monkeys that absolutely need them. For the rest, use common sense. If the developers don't seem to have common sense, then you put them in the drunken monkeys category.

Adbot
ADBOT LOVES YOU

Xarn
Jun 26, 2015

Pollyanna posted:

The entirety of a unit of work to be done, and no more, should be captured in a single ticket. The contract is to complete the unit of work specified in the requirements to the best of the engineer’s ability. Don’t ask for a subset of work in a ticket then turn around at the end of it and pile on more poo poo I didn’t know about. If you want me to do more, make a new ticket or get it all down at the beginning.

This sounds a lot like "I don't want to think for myself and have responsibility". I know that you think about yourself as a junior, but that shouldn't preclude you from having some initiative.


Or would you prefer if people wrote tickets like this?

"Extend frobnicator to also frobulate.
Add tests showing that frobnicator can now frobulate.
Refactor old tests to properly put frobnicator into frobnicating mode
Update comments to reflect that frobnicator also frobulates
Update documentation to reflect that frobnicator also frobulates
...
...
..."

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