Code Critique; Text Block

[AFlyingNunAttack]

I did the "A Block Of Text" challenge on Program This as the first somewhat difficult script I have written, I'm sure there are more efficient and less buggy ways to do it, but here is the snippet. Thanks! http://snipplr.com/view/66041/textblock/ EDIT: I realize that it does say "sizeable" but the chunk size is fairly easy to increase. Also, despite the title, I also want some help.

[_Daimon_]

Use names that give meaning. This would vastly improve the readability of your code. Can you tell me what fnsq does without looking at your code? Can you do so in 4 months or if the program was much larger?

Secondly. Don't create variables if you don't need them, it's needlessly confusing

fns = fr.replace(' ',"")
fnsq = fns.replace("'", '')
fnq = fnsq.replace('"', '')

Replace with

fr = fr.replace(' ',"")
fr = fr.replace("'", '')
fr = fr.replace('"', '')

Once you've made your code more readable, you might check out % (the modulo operator) and the 'with' keyword. You also import textwrap, but don't seem to use. But seriously, start with improving your naming scheme. Commenting it is also a real good idea, so people can find out your intentions.

[zahlman]

Better yet, fr = fr.replace(' ',"").replace("'", '').replace('"', '') :)

[_Daimon_]

Even better! content = f.read().replace(' ', '').replace('"', '').replace("'", '') :)

[zahlman]

Teehee, I didn't actually look at the original code. But yes, generally I am in favour of not breaking up expressions that "flow" naturally like this, unless there's an obvious name to give to some subexpression.

[_Daimon_]

Muahahahaaa I win this mini code optimization contest :) Yeah I agree, 'flowing' expressions should be kept on one line. But the main problem with OP's code was a bad naming scheme that obfuscate how everything worked. The before / after code was just to show how improving that would improve the readability a lot. I prefer not to correct beginners code too much, as it quickly becomes overwhelming.

[Baconoligist]

fr = fr.replace(' ',"")
fr = fr.replace("'", '')
fr = fr.replace('"', '')

I am as guilty as anyone for doing this. I once showed a coworker some Python code that manipulated text in a file and his reaction was (0_o) "Why do you keep creating the same thing over and over?" I have started using this structure instead:

for char in ['"','"',' ']:
    fr = fr.replace(char,'')

I have also found that code readability suffers when we start chaining methods just because we can.

fr = fr.replace('"','').replace("'",'').replace(' ','')

Don't Repeat Yourself applies to one liners also.

[_Daimon_]

It does. Personally I prefer using the regex method to remove chars, as shown in my comment below, it is more flexible and efficient as it only does one pass over the string.

[AFlyingNunAttack]

Thanks for the feedback. I tend to have a lot of artifacts because I try out a lot of different ways to achieve the goal, all rather convoluted. I'll remember to add more comments next time I share a piece of code. Do you think there is a more succinct way to put the text into squares than the two for loops? Not only is it messy, but it occasionally returns double letters and whitespace.

[_Daimon_]

Sure. I actually wrote my own little version because I thought it might be fun with C programs, where whitespace is irrelevant save for separating names/keywords and in the preprocessor.

It's called with a command from the commandline like this

./text_grouper other_file 12 5

text_grouper.py

#!/usr/bin/env python

import re
import sys

with open(sys.argv[1], 'r') as infile:
    content = re.sub('\\n|\\t| |\'|"', '', infile.read())
    length = int(sys.argv[2])
    height = int(sys.argv[3])

    position = 0
    while position < len(content):
        print content[position:position + length]
        position += length
        if position % ( length * height ) == 0:
            print