|
Thread Rules 1. This is not a "do my homework for me" thread. If you have specific questions, ask, but don't post an assignment or homework problem and expect an exact solution. 2. No recruiting for your cockamamie projects (you won't replace facebook with 3 dudes you found on the internet and $20) 3. If you can't articulate why a language is bad, don't start slinging shit about it. Just remember that nothing is worse than making CSS IE6 compatible. 4. Use [code] tags to format code blocks. |
On July 07 2017 09:08 berated- wrote:Show nested quote +On July 07 2017 08:30 Blisse wrote: Thoughts? A coworker merges a PR where you completely disagreed with their implementation. One senior/not-really person OK'd the PR but he never leaves comments. How would you handle this? Can you verbalize all the reasons you disagree with their implementation? How much code are we talking about? If it's under a couple hours work, I generally try to rework it in the way that I think that it should be done differently without pushing any code off hours. This has a couple benefits, if you figure out along the way that you've missed some assumptions that are actual trade offs that they already considered, you did so without putting your foot in your mouth first. It also provides the benefit that if it works out as you expected then you would now have a talking point that you can approach, I saw your code but was curious did you consider x or y? Either way, hopefully you work in an environment where you can freely and openly question why things are done. Seek to understand why they did it their way instead of imposing your own way. If you really think you know better then you should be able to ask them questions that lead them to your own conclusion without making any statements making them defensive. I personally also like to have these meetings 1 on 1 to make people feel less vulnerable and less likely to get defensive.
Sorry my response took so long. The change was very straightforward.
void function() { string a; if (condition) {
// BEGIN NEW CODE if (condition) { doSomething doSomethingElse return } // END NEW CODE
a = some a } else { a = other a }
string b; if (condition) { b = some b }
call(a, b, c, ...) }
Simplified code of course, but basically the exact control flow. I agree with and attempt to try all of your suggestions (minus coding it myself to test, as I think asking them why they couldn't do it my other way works similarly) if this was an architectural or over-reaching change, but this was a straightforward change in one function that affected control flow that I feel could've been integrated much better. Now there's just an arbitrary short circuit.
I think I'm more annoyed with upstream things being: the one coworker doesn't seem to give much feedback in PRs when he approves them, and that the PR was merged early by my coworker when there was still ongoing questions about the PR.
Plus I was kind of annoyed the coworker's reasoning for putting the code there was that "it would be faster". I think I understand why he thinks so, as he's coming from embedded.
|
I mean if you have no real code review policy at your company, there's not much you can do, save for go back and change it yourself later. Not really worth the bad blood and stress over such a small thing. There's nothing horribly wrong with that code. If you sent in a comment on their change and they didn't take your advice, you kinda just have to move on.
|
We do have a code review policy. It's one approval. 2 people who left feedback (me + coworker) didn't approve it. A third reviewer approved it but didn't leave feedback. I believe the root problem is the one coworker who approves things but never leaves any feedback and I asked my manager about it.
Also, the code adds unnecessary nesting and an abrupt change in control flow. Just generally lowers the quality of the codebase. I happily push back against that. I disagree with the idea of moving on instead of fixing the root problems, though I can understand if it sounds like I'm nitpicking on small details.
|
So if someone accepts it that overrides the explicit disapproval of 2 other people? Or did you just not approve it as in not say whether it was ok or not?
|
On July 12 2017 01:00 spinesheath wrote: So if someone accepts it that overrides the explicit disapproval of 2 other people? Or did you just not approve it as in not say whether it was ok or not?
The approver who doesn't leave feedback is the real problem. Seems like someone is just pushing buttons instead of actually reviewing the pull request.
|
On July 11 2017 15:34 Blisse wrote: We do have a code review policy. It's one approval. 2 people who left feedback (me + coworker) didn't approve it. A third reviewer approved it but didn't leave feedback. I believe the root problem is the one coworker who approves things but never leaves any feedback and I asked my manager about it.
Also, the code adds unnecessary nesting and an abrupt change in control flow. Just generally lowers the quality of the codebase. I happily push back against that. I disagree with the idea of moving on instead of fixing the root problems, though I can understand if it sounds like I'm nitpicking on small details.
Do you guys use tools that measure cyclomatic complexity? If not, then, it feels like "unnecessary nesting" is a matter of opinion vs fact. If you do, and it didn't trigger violations, then I would start to lean towards being overly nitpicky.
Sucks that someone can approve when other people bring up concerns without any sort of explanation at all.
|
Good point about cyclomatic complexity. If it were measured, it'd be easy to point out in a numbers game, though I wouldn't want to work at a place that had strict policies like that.
Regarding the code, I just haven't heard a satisfactory argument re: how it's not objectively worse. If there was worry about speed (which there is not) we could just early return at the top. The existing function has a flat, logical structure to each block of code. The new code adds nesting complexity and breaks the intended logic in each code block.
I can live with the code -- given next time (unlikely) I work in the area I'll opportunistically refactor it, but there were larger factors in play, and complaining about it here is useful for narrowing down exactly what I was annoyed with :p
I think the Approval/Decline system (we use Bitbucket) is relaxed because being too strict is bad for developer morale. But not being strict can lead to moments of disagreement like this. I've worked at places where you'd tag the PR as "Needs Improvement" and wouldn't be okay to merge until that tag was removed. Good for enforcing your disagreement is heard, bad for contributing to the PR if you don't feel like stamping the PR with a giant red flag.
|
On July 12 2017 07:55 Blisse wrote: Regarding the code, I just haven't heard a satisfactory argument re: how it's not objectively worse. If there was worry about speed (which there is not) we could just early return at the top. The existing function has a flat, logical structure to each block of code. The new code adds nesting complexity and breaks the intended logic in each code block.
I think part of the problem is that the existing code is substandard too (or at least not defensively written). It's declaring a variable outside a scope, then doing a bunch of logic to populate the string (or not?), and then repeat. This "pattern" should actually be enforced in code by splitting it into separate functions. That way it's easier to understand, and a lot harder to change it "incorrectly". e.g.
void function() { string a = calculateA(); string b = calculateB(); call(a, b, ...); }
Now you can't mess with the control flow of "function()" while calculating "a", unless you do something drastic.
|
On July 12 2017 06:45 berated- wrote:Show nested quote +On July 11 2017 15:34 Blisse wrote: We do have a code review policy. It's one approval. 2 people who left feedback (me + coworker) didn't approve it. A third reviewer approved it but didn't leave feedback. I believe the root problem is the one coworker who approves things but never leaves any feedback and I asked my manager about it.
Also, the code adds unnecessary nesting and an abrupt change in control flow. Just generally lowers the quality of the codebase. I happily push back against that. I disagree with the idea of moving on instead of fixing the root problems, though I can understand if it sounds like I'm nitpicking on small details. Do you guys use tools that measure cyclomatic complexity? If not, then, it feels like "unnecessary nesting" is a matter of opinion vs fact. If you do, and it didn't trigger violations, then I would start to lean towards being overly nitpicky. Sucks that someone can approve when other people bring up concerns without any sort of explanation at all. Even if you use tools for computing cyclomatic complexity, you still run into a lot of murky areas for instance I don't think its possible convincing someone that a cyclomatic complexity of 10 is a generally a bad thing without resorting to opinions.
|
Anyone have insight on using Gower distance + a hierarchical clustering method vs using k-prototypes for clustering mixed categorical + continuous data?
|
On July 14 2017 23:14 sabas123 wrote:Show nested quote +On July 12 2017 06:45 berated- wrote:On July 11 2017 15:34 Blisse wrote: We do have a code review policy. It's one approval. 2 people who left feedback (me + coworker) didn't approve it. A third reviewer approved it but didn't leave feedback. I believe the root problem is the one coworker who approves things but never leaves any feedback and I asked my manager about it.
Also, the code adds unnecessary nesting and an abrupt change in control flow. Just generally lowers the quality of the codebase. I happily push back against that. I disagree with the idea of moving on instead of fixing the root problems, though I can understand if it sounds like I'm nitpicking on small details. Do you guys use tools that measure cyclomatic complexity? If not, then, it feels like "unnecessary nesting" is a matter of opinion vs fact. If you do, and it didn't trigger violations, then I would start to lean towards being overly nitpicky. Sucks that someone can approve when other people bring up concerns without any sort of explanation at all. Even if you use tools for computing cyclomatic complexity, you still run into a lot of murky areas for instance I don't think its possible convincing someone that a cyclomatic complexity of 10 is a generally a bad thing without resorting to opinions.
Sure but at least there are guidelines around when you should maybe start to have conversations and bring in more view points -- and when someone might be just being picky for not a lot of gain. Programming is so much about trade offs that there is rarely a one size fits all. I definitely agree with you though.
|
Russian Federation4235 Posts
On July 14 2017 21:06 netherh wrote:Show nested quote +On July 12 2017 07:55 Blisse wrote: Regarding the code, I just haven't heard a satisfactory argument re: how it's not objectively worse. If there was worry about speed (which there is not) we could just early return at the top. The existing function has a flat, logical structure to each block of code. The new code adds nesting complexity and breaks the intended logic in each code block. I think part of the problem is that the existing code is substandard too (or at least not defensively written). It's declaring a variable outside a scope, then doing a bunch of logic to populate the string (or not?), and then repeat. This "pattern" should actually be enforced in code by splitting it into separate functions. That way it's easier to understand, and a lot harder to change it "incorrectly". e.g. void function() { string a = calculateA(); string b = calculateB(); call(a, b, ...); }
Now you can't mess with the control flow of "function()" while calculating "a", unless you do something drastic.
Funnily enough, I find this code pretty bad because it makes me think: - Is a used anywhere else? Is b? Why are they variables? Am I missing something? Almost unavoidable in a messy language like Java, but there's no excuse for that in C or C++.
|
On July 15 2017 08:29 BluzMan wrote:Show nested quote +On July 14 2017 21:06 netherh wrote:On July 12 2017 07:55 Blisse wrote: Regarding the code, I just haven't heard a satisfactory argument re: how it's not objectively worse. If there was worry about speed (which there is not) we could just early return at the top. The existing function has a flat, logical structure to each block of code. The new code adds nesting complexity and breaks the intended logic in each code block. I think part of the problem is that the existing code is substandard too (or at least not defensively written). It's declaring a variable outside a scope, then doing a bunch of logic to populate the string (or not?), and then repeat. This "pattern" should actually be enforced in code by splitting it into separate functions. That way it's easier to understand, and a lot harder to change it "incorrectly". e.g. void function() { string a = calculateA(); string b = calculateB(); call(a, b, ...); }
Now you can't mess with the control flow of "function()" while calculating "a", unless you do something drastic. Funnily enough, I find this code pretty bad because it makes me think: - Is a used anywhere else? Is b? Why are they variables? Am I missing something? Almost unavoidable in a messy language like Java, but there's no excuse for that in C or C++.
And what is your solution for that abstraction in C/C++ that isn't possible in Java... ? Other than exposing methods unnecessarily it looks.. fine?
|
On July 15 2017 08:44 Blisse wrote:Show nested quote +On July 15 2017 08:29 BluzMan wrote:On July 14 2017 21:06 netherh wrote:On July 12 2017 07:55 Blisse wrote: Regarding the code, I just haven't heard a satisfactory argument re: how it's not objectively worse. If there was worry about speed (which there is not) we could just early return at the top. The existing function has a flat, logical structure to each block of code. The new code adds nesting complexity and breaks the intended logic in each code block. I think part of the problem is that the existing code is substandard too (or at least not defensively written). It's declaring a variable outside a scope, then doing a bunch of logic to populate the string (or not?), and then repeat. This "pattern" should actually be enforced in code by splitting it into separate functions. That way it's easier to understand, and a lot harder to change it "incorrectly". e.g. void function() { string a = calculateA(); string b = calculateB(); call(a, b, ...); }
Now you can't mess with the control flow of "function()" while calculating "a", unless you do something drastic. Funnily enough, I find this code pretty bad because it makes me think: - Is a used anywhere else? Is b? Why are they variables? Am I missing something? Almost unavoidable in a messy language like Java, but there's no excuse for that in C or C++. And what is your solution for that abstraction in C/C++ that isn't possible in Java... ? Other than exposing methods unnecessarily it looks.. fine?
I believe that he'd like it to look like that:
void callSomeMethods() { call(calculateA(), calculateB(), ...); }
Also, Java since version 8 became much less of a mess (it just needs more type inference and getting rid of primitives like they did in Scala).
|
I hope this is the right place to post this. Is there any library or API to get your opponents name at the start of a SCII match? Would prefer python, if possible
|
On July 18 2017 00:18 MrBrokenPenis wrote: I hope this is the right place to post this. Is there any library or API to get your opponents name at the start of a SCII match? Would prefer python, if possible
Doesn't the name just show up? It has been awhile since I played SC2 but doesn't your handle just come up on load screen?
Or are you talking about your real name that you register Bnet with?
|
No, I would like to have script running, that grabs the name of my opponent
|
You'll probably get banned by Blizzard for using third-party things to read their game data. Against ToS most likely. If you want opponent data just use the SC2 api https://dev.battle.net/io-docs
|
|
can anyone help a c noob? -- say there is a text file with a string, say "abcdefghijk"
read in that file and delete a substring from it (have file update the new string) -- is there a very simple way to do this?
so far all I got is fopen with the r+ mode and fgetc which doesn't seem to be what i'm looking for..
any easy to read resource or pointers(heh) for the best approach would be greatly appreciated. I've only gotten around to parsing individual strings from file or going by each character, but not substrings :/
|
|
|
|