Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

Can it possible to make this script short, so that multiple data and multiple output file can be managed efficiently?

def create_output_file(self, **kwargs):
    print 'Now output file created'
    filename1 = 'res_norm.txt'
    filename2 = 'bound.txt'
    filename3 = 'soln_2norm.txt'
    matrixRes = np.vstack((self.iter_list, self.res_norm)).T
    matrixBound = np.vstack((self.iter_list, self.bound)).T
    matrixSol2 = np.vstack((self.iter_list, self.soln_2nrm)).T
    fo1 = open(filename1, "a+")
    fo2 = open(filename2, "a+")
    fo3 = open(filename3, "a+")
    fo1.write('# Residual Norm %s' % (kwargs['s']))
    fo2.write('# Bound %s' % (kwargs['s']))
    fo3.write('# Solution_2Norm %s' % (kwargs['s']))
    fo1.write('\n')
    fo2.write('\n')
    fo3.write('\n')
    np.savetxt(fo1, matrixRes, fmt='%f \t %1.4e')
    np.savetxt(fo2, matrixBound, fmt='%f \t %1.4e')
    np.savetxt(fo3, matrixSol2, fmt='%f \t %1.4e')
    fo1.write('\n')
    fo1.write('\n')
    fo2.write('\n')
    fo2.write('\n')
    fo3.write('\n')
    fo3.write('\n')
    fo1.close()
    fo2.close()
    fo3.close()
    return
share|improve this question

migrated from stackoverflow.com Feb 4 '15 at 13:44

This question came from our site for professional and enthusiast programmers.

Summarizing the above comments, simply using with and embedding the multiple newlines into other strings will yield this smaller and more readable version of your function

def create_output_file(self, **kwargs):
    matrixRes = np.vstack((self.iter_list, self.res_norm)).T
    matrixBound = np.vstack((self.iter_list, self.bound)).T
    matrixSol2 = np.vstack((self.iter_list, self.soln_2nrm)).T

    with open('res_norm.txt', 'a+') as fo1:
        fo1.write('# Residual Norm %s\n' % (kwargs['s']))
        np.savetxt(fo1, matrixRes, fmt='%f \t %1.4e')
        fo1.write('\n\n')

    with open('bound.txt', 'a+') as fo2:
        fo2.write('# Bound %s\n' % (kwargs['s']))
        np.savetxt(fo2, matrixBound, fmt='%f \t %1.4e')
        fo2.write('\n\n')

    with open('soln_2norm.txt', 'a+') as fo3:
        fo3.write('# Solution_2Norm %s\n' % (kwargs['s']))
        np.savetxt(fo3, matrixSol2, fmt='%f \t %1.4e')
        fo3.write('\n\n')

I've done nothing to clean up your choice of variable names or function arguments, and I doubt this is any more efficient than what you had ... well slightly better as there are fewer write() calls.

I don't see any reason for the method to take a **kwargs argument as you only ever look at kwargs['s'] so you could just change the declaration to

def create_output_file(self, s)

and access it directly (and a better name would be a "very good thing").

You could replace the three with blocks by a single one inside a loop, something like

parms = (('res_norm.txt', '# Residual Norm %s\n', matrixRes),
         ('bound.txt', '# Bound %s\n', matrixBound),
         ('soln_2norm.txt', '# Solution_2Norm %s\n', matrixSol2))

for fname, title, table in parms:
    with open(fname, "a+") as f:
        f.write(title % kwargs['s'])
        np.savetxt(f, table, fmt='%f \t %1.4e')
        f.write('\n\n')

Which is more generic, and shows that each file has a common structure though different data. But I'm not sure how much better it is for readability.

share|improve this answer
    
'kdopen' Thank you very much the for loop Idea is very useful for me. – Nachi Feb 3 '15 at 20:32
    
@user46242 YW - Please remember to accept one of the answers at some point. Upvoting is good, but accepting actually indicates to future readers which one answered your question. – kdopen Feb 4 '15 at 19:13
    
Thanks for your guidance for efficient discussion here to a new user. – Nachi Feb 5 '15 at 17:31

Since kdopen already gave a very nice review about how you implemented the process, here goes about some other stuff.

Naming:

Your names are terribly unhelpful. Numbering your variables with 1, 2, 3 should ring the alert bells and get you to defcon 3. Make your names count:

filename1 -> residual_norms_file
filename2 -> bound_file
filename3 -> solution_for_norming_file

Since you didn't explain terribly much about the code this is mostly guesswork. The more you describe the purpose of a variable in it's name, the easier it is to understand the code as someone else or months (even years) later.

Additionally your matrices should be named according to python naming convention (defined in PEP-8), namely in snake_case

Documentation:

You should really document your code with docstrings. This allows others (and your future self) to understand what you were thinking when you did things the way you did them. The syntax is simple:

def create_output_file(self, **kwargs):
    """
    This method creates output files from the matrix data obtained, namely written into the files 'res_norm.txt', 'bound.txt' and 'soln_2norm.txt'
    """
    # code here ;)

Oh and a last nitpick: return is unnecessary. A function that ends without an explicit return statement, an implicit return is added automatically as the last instruction ;-) (Which means it returns None)

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.