** UnhandledExceptionEventHandler :: OFFICIAL LC / CHATTER THREAD ** - Page 678 - Computer Technical Help

Two Plus Two Forums Other Topics Computer and Technical Help

** UnhandledExceptionEventHandler :: OFFICIAL LC / CHATTER THREAD **

Post Reply Subscribe

...

Page 678 of 1603

First

178 478 578 628 658 668 673 674 675 676 677 678 679 680 681 682 683 688 698 728 778 878 1178

Last

Page 678 of 1603

First

178 478 578 628 658 668 673 674 675 676 677 678 679 680 681 682 683 688 698 728 778 878 1178

Last

03-03-2015 , 01:20 PM

#16926

gaming_mouse

Carpal \'Tunnel

Join Date: Oct 2004 Posts: 13,786

Quote:

Originally Posted by Grue

code review is going to drive me nuts. Just ran into this hot take:

Code:

formatter: function(d){
	var activities = [
		{displayValue: this.translate('New'), value:d},
		{displayValue: this.translate('None'), value:d},
		{displayValue: this.translate('On track'), value:d},
		{displayValue: this.translate('Some issues'), value:d},
		{displayValue: this.translate('Complete'), value:d}
	];
return activities[d];
}

thats um.. inventive.

this isn't that bad at all. this wouldn't even come close to crossing the threshold of "lol look at this code" for me. It's short and clear. That alone means it doing it pretty good. that it makes objects that don't get used doesn't matter at all unless it's performance critical which it isn't. the "d" and value:d seems odd, but eh...

Quote

$25m Guaranteed WPM on CoinPoker

Join the action now

Daily Rewards • Splash Pots • CoinRaces

03-03-2015 , 03:11 PM

#16927

candybar

Pooh-Bah

Join Date: Aug 2011 Posts: 4,375

Quote:

Originally Posted by Grue

code review is going to drive me nuts. Just ran into this hot take:

Code:

formatter: function(d){
	var activities = [
		{displayValue: this.translate('New'), value:d},
		{displayValue: this.translate('None'), value:d},
		{displayValue: this.translate('On track'), value:d},
		{displayValue: this.translate('Some issues'), value:d},
		{displayValue: this.translate('Complete'), value:d}
	];
return activities[d];
}

thats um.. inventive.

Agree with gm here - this isn't that bad by itself. I guess this is worse to some people:

return {displayValue: ['New', 'None', 'On track', 'Some issues', 'Complete'].map(this.translate, this)[d], value:d}

Quote

03-03-2015 , 03:42 PM

#16928

Grue

Pooh-Bah

Join Date: Mar 2004 Posts: 5,667

Still on ES3.. can never escape it.

Quote

03-03-2015 , 03:47 PM

#16929

suzzer99

Save the Cheerleader, Save the World

Join Date: Nov 2005 Posts: 101,163

Man you guys must look at some god-awful code all day. I think that code is terrible. It doesn't flow in any kind of logical pattern at all. I'd much rather see something like this. At least I can instantly understand what's going on

Code:

formatter: function(d){
  switch(d) {
    case 1:
      return {displayValue: this.translate('New'), value:1};
    case 2:
      return {displayValue: this.translate('None'), value:2};
    case 3:
      return {displayValue: this.translate('On track'), value:3};
    case 4:
      return {displayValue: this.translate('Some issues'), value:4};
    case 5:
      return {displayValue: this.translate('Complete'), value:5};
    default: 
      return null; 
  }
}

Quote

03-03-2015 , 04:22 PM

#16930

candybar

Pooh-Bah

Join Date: Aug 2011 Posts: 4,375

Quote:

Originally Posted by suzzer99

Man you guys must look at some god-awful code all day.

Guilty as charged! Not sure if it's fair to judge based on that snippet though. It's possible the logic is meant to be data-driven eventually (config file, web service, etc) which makes the ridiculous-looking array a better approximation of how the code will evolve than a switch loop.

Rich Hickey had a great point somewhere in his talk about how data is simpler than code:

http://www.infoq.com/presentations/Simple-Made-Easy
https://github.com/matthiasn/talk-tr...pleMadeEasy.md

Not saying this is what's going on, but it's hard to tell without context.

As for terrible code, I even see a lot of people doing stuff that boils down to:

Code:

for (i = 0; i < n; i++)
{
  if (i == x)
  {
     doStuff(i);
  }
}

which to me is a lot worse.

Quote

03-03-2015 , 04:55 PM

#16931

jjshabado

Carpal Tunnel

Join Date: Jul 2006 Posts: 22,732

I think I've been lucky in that I've never worked with people that don't know the basics of programming (in a career setting - I saw tons of bad code when TAing). I still deal with bad code (because everybody writes bad code) but its not in the same category of 'WTF was this person thinking, do they know anything about programming at all?'.

Quote

03-03-2015 , 05:40 PM

#16932

gaming_mouse

Carpal \'Tunnel

Join Date: Oct 2004 Posts: 13,786

Quote:

Originally Posted by suzzer99

Code:

formatter: function(d){
  switch(d) {
    case 1:
      return {displayValue: this.translate('New'), value:1};
    case 2:
      return {displayValue: this.translate('None'), value:2};
    case 3:
      return {displayValue: this.translate('On track'), value:3};
    case 4:
      return {displayValue: this.translate('Some issues'), value:4};
    case 5:
      return {displayValue: this.translate('Complete'), value:5};
    default: 
      return null; 
  }
}

The difference between the above and the original is, for most practical purposes, trivial. I care a lot about code, have no problem "wasting" time to hone for readability, choosing good names, etc. But it's important to have a sense for what kind of things actually matter, and what kind of things are, essentially, equivalent to complaining about white space, braces, and so on. Not that you shouldn't complain about those things, but it should be a different category of complaint.

So while sure, I prefer yours to the original, I consider it a small improvement that hardly matters.

EDIT: And honestly I'm not crazy about either of them, but proper refactoring would require more knowledge of surrounding context.

Quote

03-03-2015 , 05:44 PM

#16933

jjshabado

Carpal Tunnel

Join Date: Jul 2006 Posts: 22,732

I have to vote with it being somewhere in the middle of basically fine and "OMG WORST CODE EVER"

The value:d part was pretty confusing to me until I realized what he was doing. Making up your own hacks for standard code constructs is a definite hit in readability to me.

Quote

03-03-2015 , 06:25 PM

#16934

adios

Carpal \'Tunnel

Join Date: Sep 2002 Posts: 22,558

Quote:

Originally Posted by suzzer99

Code:

formatter: function(d){
  switch(d) {
    case 1:
      return {displayValue: this.translate('New'), value:1};
    case 2:
      return {displayValue: this.translate('None'), value:2};
    case 3:
      return {displayValue: this.translate('On track'), value:3};
    case 4:
      return {displayValue: this.translate('Some issues'), value:4};
    case 5:
      return {displayValue: this.translate('Complete'), value:5};
    default: 
      return null; 
  }
}

Not my area but it looks like this is much better to me. If d == 0 or d > 5 this code appears like it would handle those values. Not sure if activities[] first element starts with a zero or one as well.

Quote

03-03-2015 , 06:56 PM

#16935

Shoe Lace

Pooh-Bah

Join Date: Sep 2004 Posts: 3,654

The thing that bothers me the most is why does it need to be a function?

Just use an activities hash and do a lookup on "d" as the key.

Quote

03-03-2015 , 07:09 PM

#16936

Grue

Pooh-Bah

Join Date: Mar 2004 Posts: 5,667

That's probably the best answer and I should have addressed that in the code review.

Quote

03-03-2015 , 07:54 PM

#16937

skier_5

Pooh-Bah

Join Date: Jan 2006 Posts: 5,443

Quote:

Originally Posted by Grue

code review is going to drive me nuts. Just ran into this hot take:

Code:

formatter: function(d){
	var activities = [
		{displayValue: this.translate('New'), value:d},
		{displayValue: this.translate('None'), value:d},
		{displayValue: this.translate('On track'), value:d},
		{displayValue: this.translate('Some issues'), value:d},
		{displayValue: this.translate('Complete'), value:d}
	];
return activities[d];
}

thats um.. inventive.

I don't think the general concept is crazy, just poorly executed. Or is something like this not good as well?

Code:

formatter: function(d){
  var activities = ['New', 'None', 'On track', 'Some issues', 'Complete'];
  var activity = activities[d];
  if (activity != undefined) {
    return {displayValue: this.translate(activity), value:d};
  } else {
    return null;
  }
}

Quote

03-03-2015 , 08:54 PM

#16938

goofyballer

Carpal \'Tunnel

Join Date: Jun 2005 Posts: 70,719

Quote:

Originally Posted by gaming_mouse

I've come around to the belief that habits matter, and that employing a ****ty habit in a situation where the context that makes it okay is still bad because a.) someone has to look at and understand the context to know that it isn't bad, instead of just not having to evaluate the context at all because the code was written properly to begin with b.) it increases the possibility that habit will slip into other areas of your coding.

Disclaimer: in this particular context, I don't know Javascript or whatever language that is.

Quote

03-03-2015 , 09:13 PM

#16939

gaming_mouse

Carpal \'Tunnel

Join Date: Oct 2004 Posts: 13,786

Quote:

Originally Posted by goofyballer

sure, i don't disagree.

an analogy might clarify my position here. somebody turns in an essay that wasn't spellchecked, and has some sloppy capitalization errors too. that's clearly bad, you shouldn't do it, there's not really an excuse for it.

if you're the teacher, one type of reaction to that paper is, "this person is clearly an idiot, i'm going to skim the paper but basically i've already decided it's going to be a bad grade because how could it not be." imo, such a teacher is making a worse mistake than the student who didn't bother to spellcheck his essay. even though the teacher is correct that there's a correlation between sloppiness and bad writing in a deeper sense, it's not as strong as you'd think. but more importantly, they are fundamentally different kinds of errors, both in importance and in kind.

and you can't ever forget that and just dismiss stuff that's sloppy as if it were fundamentally bad, because your own judgment will suffer. and there is another error, fairly common, in which you can focus lots of energy on things like style guides and consistency and convince yourself that you are a good, careful coder with immaculately high standards, standing above the hoards of sloppy php slingers, while completely missing the big picture, oblivious to bad architectural flaws and other problems that really matter.

Quote

03-03-2015 , 10:18 PM

#16940

suzzer99

Save the Cheerleader, Save the World

Join Date: Nov 2005 Posts: 101,163

It also shows how someone thinks. We have a monster site with up to 10 teams working on different features at once, offshore developers, and a BAU team that just works on bugs and small changes (who often are seeing the code for the first time). Because of this we need developers who think clearly and look for the most straightforward, concise solution to the problem. This is much more important to us than having a javscript guru. The code in question is not clear or concise.

Last edited by suzzer99; 03-03-2015 at 10:23 PM.

Quote

03-03-2015 , 10:50 PM

#16941

adios

Carpal \'Tunnel

Join Date: Sep 2002 Posts: 22,558

Quote:

Originally Posted by suzzer99

And has an obvious defect that should be removed without too much difficulty. Defect removal should be the number one priority in a code review.

Quote

03-03-2015 , 11:13 PM

#16942

suzzer99

Save the Cheerleader, Save the World

Join Date: Nov 2005 Posts: 101,163

Maintainability/readability imo. Defects come out in testing.

Quote

03-03-2015 , 11:23 PM

#16943

gaming_mouse

Carpal \'Tunnel

Join Date: Oct 2004 Posts: 13,786

Quote:

Originally Posted by suzzer99

i disagree. the code is perfectly readable. i understood it almost instantly.

Quote:

Originally Posted by adios

And has an obvious defect that should be removed without too much difficulty. Defect removal should be the number one priority in a code review.

it's actually not clear there is a defect. if the function was part of a public API, then yes. but it could easily be a private function which is (correctly) making the assumption that its inputs are already validated.

sort of in an odd position here, defending something i don't really think is good and which probably is the work of someone less than stellar, but so far the reasons i've seen given for hating on this aren't well founded.

Quote

03-03-2015 , 11:27 PM

#16944

suzzer99

Save the Cheerleader, Save the World

Join Date: Nov 2005 Posts: 101,163

I didn't understand the code instantly. Also I immediately wonder if it's doing something weirder and more complicated because it's overly complicated. So even once i figure out what it's doing, I have to spend more time figuring out if I'm wrong and it's doing something more than that.

Quote

03-03-2015 , 11:47 PM

#16945

gaming_mouse

Carpal \'Tunnel

Join Date: Oct 2004 Posts: 13,786

Quote:

Originally Posted by suzzer99

i agree with your principle. i just don't think the code in question an example of anything particularly odd or rogue. i mean, it's an array lookup. the array lookup in place of the switch is also a common way of implementing the strategy pattern, which is likely where the author got his inspiration. and while you can argue that this mini strategy pattern where an ordinary switch would do is over-engineering on a micro scale, i still think it qualifies as simple and readable enough. maybe just a difference of opinion at this point...

Quote

03-03-2015 , 11:52 PM

#16946

suzzer99

Save the Cheerleader, Save the World

Join Date: Nov 2005 Posts: 101,163

But you're redefining a mostly static array everytime in the function call, just to get the value:d object into it. That isn't conceptually straightforward at all.

At least move the displayValue content part out of it. Unless this.translate is somehow instance-dependent.

Quote

03-04-2015 , 12:04 AM

#16947

gaming_mouse

Carpal \'Tunnel

Join Date: Oct 2004 Posts: 13,786

Quote:

Originally Posted by suzzer99

I'd argue that it's perfectly clear, even though it is inefficient. As I said before, though, the inefficiency is unlikely to have any practical concequences (I can't know that for sure without knowing the context, but it seems safe to say).

Still, from a readability perspective, you can argue that not treating static data as static obscures intent (ie, a reader will do a little double-take, wonder why this data is re-allocated with every function call, wonder if they're missing something, then realize it's just a missing optimization). I'd agree with that argument. But I still don't think the mistake is more than slightly bad in this particular case.

I would point it out in a code review, as a little thing that should be fixed. But for me, it's just so far from blooper-worthy, which was how Grue originally posted it. That's really all this boils down to.

Quote

03-04-2015 , 12:25 AM

#16948

Grue

Pooh-Bah

Join Date: Mar 2004 Posts: 5,667

I think its decently readable and understandable, I just think instantiating 5 objects and only using one is a pretty bad idea and fairly chuckleworthy.

Quote

03-04-2015 , 12:34 AM

#16949

jjshabado

Carpal Tunnel

Join Date: Jul 2006 Posts: 22,732

Quote:

Originally Posted by suzzer99

Absolutely this.

Edit: It also seems likely to me that a junior/new developer would have trouble figuring out what's happening and why.

Quote

03-04-2015 , 12:40 AM

#16950

gaming_mouse

Carpal \'Tunnel

Join Date: Oct 2004 Posts: 13,786

Quote:

Originally Posted by Grue

I think its decently readable and understandable, I just think instantiating 5 objects and only ever using one is a pretty bad idea, ever.

i get where you're coming from, but at the same time, if you are writing idiomatic code in a modern, high-level language, you will (and should) be doing things regularly that "waste" just as much memory as that example. so objectively, it's not doing anything that's going to come close to affecting the performance of your application. i guess i'm arguing for you to examine what your "bad" really means in this case, if the badness doesn't result in either lack of clarity (we agree on that, though jj and suzzer don't, so let's say that's debatable in this case) or measurable performance loss.