3 ms·

https://github.com/kislyuk/dmtk/blob/master/scripts/comparePulseData.py https://github.com/kislyuk/dmtk/blob/master/scripts/compareP... Feel free to criticize.

by ak217 14y ago

https://github.com/kislyuk/dmtk/blob/master/scripts/comparePulseData.py https://github.com/kislyuk/dmtk/blob/master/scripts/compareP... Feel free to criticize. That code has lots of shortcomings. Compromised readability due to line length is not one of them.

deleted 14y ago
[deleted]
ajross 14y ago
You can't be serious. Check lines 388-400. Now, even ignoring the fact that github itself is chopping those lines because they're too long, they're simply not meaningfully readable. The ridiculous repetition (which is the proximate cause of those awful line lengths) totally obscures the meaning. Note all the repeated use of this weird dictionary name "my_stats[condition][strand]". This thing appears, often multiple times, on ever line of that loop. You're telling me that this couldn't be made more readable by using a simple local "stats" reference name "st"? Repetition of expressions is the sign of lazy code, it's the same kind of thought process that gives us cut-and-paste bugs. Note likewise the repated use of "kc_stats[condition]". Same deal. Same treatment. Note likewise the oddball commented out line (assigning a tuple of m2_ci_lo_fwd, m2_ci_hi_fwd) which is disabled without explanation. Remember my list of attendant symptoms? And, on the design side: note the mean/median/stdev calls. Those are generally pretty well-defined properties of "sets". Why is there a "Metric" postpended to the method names needlessly? Surely the reader would understand "st.stdev()" already, right? And why do the methods take five named parameters?. That's insane -- they should take zero. It looks like this is trying to pass in some extra "stuff" (maybe to control some side effects, or to filter the data) to what should be an idempotent operation on a fixed piece of data. If you have to filter, or if the object does some extra counting or whatever, those should be set up on the data ahead of time. This code is crap. Sorry.
- ajross 14y ago
  Just because I'm feeling bored today, here is that same loop (around line 388) "cleaned up" to work inside 80 columns. I added a "metricGenrator" abstraction to the API because it seemed like a sane addition to clean up the "too many arguments to mean()" problem, but all other changes are is pure syntax. I dare you to argue that this isn't more readable than the original: my_stats = {'Modified': {'+': {}, '-': {}}, 'Control': {'+': {}, '-': {}}} for condition in 'Modified', 'Control': for strand in '+', '-': st = my_stats[condition][strand] kc = kc_stats[condition] kc = kc.metricGenerator(metric, ref_pos=ref_pos, trim=opts.exp_trim, coverage_ceiling=opts.coverage_ceiling, strand=strand) st['mean'] = kc.mean() st['median'] = kc.median() off = st['stdev'] = kc.stdev() if use_se: kc.setOpts(ref_pos, metric, trim=0.0, coverage_ceiling=opts.coverage_ceiling, strand=strand) off /= sqrt(len(kc._getMetricData())) st['spread_lo'] = st['mean'] - off st['spread_hi'] = st['mean'] + off my_report_row.extend(st['mean'], st['median'], st['stdev'], st['spread_lo'], st['spread_hi'], st['coverage']])
  - arh68 14y ago
    I don't know if this was easy or quick for you (less than an hour, in any case), but you seem to prove the parent correct: "That code has lots of shortcomings. Compromised readability due to line length is not one of them." Maybe you're simply an amazing programmer, but to jump in and refactor a small bit of code deep inside something you've never seen before proves to me that the initial code was indeed readable. In either case, both examples are literally geometrically square, which I think drives readability more than line width. When the long lines poke out and don't relate to other long lines, it's hard to read. If all the lines are long and share structure, it's just verbose, but reasonably easy to read. With that perspective, I'd say the parent's code isn't a great example of long-line code: it doesn't demonstrate a _need_ for 160 chars; I'll agree with you.
    - ajross 14y ago
      I started typing a longer response, but honestly I think the clearest answer here is just sarcasm: Some of us have a higher standard for "readable" code than "can be refactored so that it doesn't suck". :)
Hytosys 14y ago
self.opts.modified_positions_fwd = [ int(p) for p in self.opts.modified_positions_fwd.split(",") ] if self.opts.modified_positions_fwd != None else [] Without explicitly attacking Python's syntax, I had to horizontally scroll left and right multiple times to understand the entire impact of this statement. Both yours and my claims are anecdotal. Unfortunately I don't know of many research studies, though I'm sure some has been done for this issue in typography.
- ak217 14y ago
  You're right, those lines (and a few others in that file) aren't good. For the ones you cited, that run-on if really is too far out. If I was writing them today, I'd break it up into an if..else.
thangalin 14y ago
The code is illegible for a number of reasons: * Duplication everywhere. * No internal comments indicating the code's purpose. * Long functions with too much nesting. * Lack of constants. * Indenting by 4 spaces (instead of 2). * Lines like 316-320 and 326-335 should line break and indent after the first open parenthesis to avoid huge whitespace. * Inconsistent line break usages for long lines. * Line 412, for example, should line break at "ref_pos". The code has many shortcomings, including readability due to extreme line length.

by ak217 14y ago

deleted 14y ago

[deleted]

ajross 14y ago

You can't be serious. Check lines 388-400. Now, even ignoring the fact that github itself is chopping those lines because they're too long, they're simply not meaningfully readable. The ridiculous repetition (which is the proximate cause of those awful line lengths) totally obscures the meaning. Note all the repeated use of this weird dictionary name "my_stats[condition][strand]". This thing appears, often multiple times, on ever line of that loop. You're telling me that this couldn't be made more readable by using a simple local "stats" reference name "st"? Repetition of expressions is the sign of lazy code, it's the same kind of thought process that gives us cut-and-paste bugs. Note likewise the repated use of "kc_stats[condition]". Same deal. Same treatment. Note likewise the oddball commented out line (assigning a tuple of m2_ci_lo_fwd, m2_ci_hi_fwd) which is disabled without explanation. Remember my list of attendant symptoms? And, on the design side: note the mean/median/stdev calls. Those are generally pretty well-defined properties of "sets". Why is there a "Metric" postpended to the method names needlessly? Surely the reader would understand "st.stdev()" already, right? And why do the methods take five named parameters?. That's insane -- they should take zero. It looks like this is trying to pass in some extra "stuff" (maybe to control some side effects, or to filter the data) to what should be an idempotent operation on a fixed piece of data. If you have to filter, or if the object does some extra counting or whatever, those should be set up on the data ahead of time. This code is crap. Sorry.

ajross 14y ago
Just because I'm feeling bored today, here is that same loop (around line 388) "cleaned up" to work inside 80 columns. I added a "metricGenrator" abstraction to the API because it seemed like a sane addition to clean up the "too many arguments to mean()" problem, but all other changes are is pure syntax. I dare you to argue that this isn't more readable than the original: my_stats = {'Modified': {'+': {}, '-': {}}, 'Control': {'+': {}, '-': {}}} for condition in 'Modified', 'Control': for strand in '+', '-': st = my_stats[condition][strand] kc = kc_stats[condition] kc = kc.metricGenerator(metric, ref_pos=ref_pos, trim=opts.exp_trim, coverage_ceiling=opts.coverage_ceiling, strand=strand) st['mean'] = kc.mean() st['median'] = kc.median() off = st['stdev'] = kc.stdev() if use_se: kc.setOpts(ref_pos, metric, trim=0.0, coverage_ceiling=opts.coverage_ceiling, strand=strand) off /= sqrt(len(kc._getMetricData())) st['spread_lo'] = st['mean'] - off st['spread_hi'] = st['mean'] + off my_report_row.extend(st['mean'], st['median'], st['stdev'], st['spread_lo'], st['spread_hi'], st['coverage']])
- arh68 14y ago
  I don't know if this was easy or quick for you (less than an hour, in any case), but you seem to prove the parent correct: "That code has lots of shortcomings. Compromised readability due to line length is not one of them." Maybe you're simply an amazing programmer, but to jump in and refactor a small bit of code deep inside something you've never seen before proves to me that the initial code was indeed readable. In either case, both examples are literally geometrically square, which I think drives readability more than line width. When the long lines poke out and don't relate to other long lines, it's hard to read. If all the lines are long and share structure, it's just verbose, but reasonably easy to read. With that perspective, I'd say the parent's code isn't a great example of long-line code: it doesn't demonstrate a _need_ for 160 chars; I'll agree with you.
  - ajross 14y ago
    I started typing a longer response, but honestly I think the clearest answer here is just sarcasm: Some of us have a higher standard for "readable" code than "can be refactored so that it doesn't suck". :)

Hytosys 14y ago

self.opts.modified_positions_fwd = [ int(p) for p in self.opts.modified_positions_fwd.split(",") ] if self.opts.modified_positions_fwd != None else [] Without explicitly attacking Python's syntax, I had to horizontally scroll left and right multiple times to understand the entire impact of this statement. Both yours and my claims are anecdotal. Unfortunately I don't know of many research studies, though I'm sure some has been done for this issue in typography.

ak217 14y ago
You're right, those lines (and a few others in that file) aren't good. For the ones you cited, that run-on if really is too far out. If I was writing them today, I'd break it up into an if..else.

thangalin 14y ago

The code is illegible for a number of reasons: * Duplication everywhere. * No internal comments indicating the code's purpose. * Long functions with too much nesting. * Lack of constants. * Indenting by 4 spaces (instead of 2). * Lines like 316-320 and 326-335 should line break and indent after the first open parenthesis to avoid huge whitespace. * Inconsistent line break usages for long lines. * Line 412, for example, should line break at "ref_pos". The code has many shortcomings, including readability due to extreme line length.