1
\$\begingroup\$

The code works well and does what I intend for it to do. In essence, it opens a file referenced as 'resource'. This is a .csv file. I then searches for the keys in the dictionary and for each key that it encounters, it replaces it with the value - the dictionary being 'rep'. It then exports this to a new .csv file.

The above just seems slightly cumbersome and so I just wonder if there is a slicker / more pythonistic way of achieving the above? I think the non-slick part of my code is the writing of the amended data to a new file but I seek any feedback regarding improving my code.

with open(resource,'rb') as f:
   data = f.read()
   rep = {"Incoming Call" : "Voice (in)",
          "Outgoing Call" : "Voice (out)",
          "Incoming SMS" : "SMS (in)",
          "Outgoing SMS" : "SMS (out)"}
   for i, j in rep.iteritems():
      print i, j
      data = data.replace(i, j)
   with open("output.txt", "w") as outputfile:
      outputfile.write(data)
      outputfile.close()
\$\endgroup\$
2
\$\begingroup\$

The process you're using is better than trying to read and write the same file, which is just asking for a headache. However there is another way, which will particularly help if you need to use this on a larger file. You can read the file line by line, writing the replaced lines to your output file. This is less memory intensive as only a line is read at a time.

with open(resource,'rb') as f, open("output.txt", "w") as outputfile:
    for line in f:
        for i, j in rep.iteritems():
            line = line.replace(i, j)
        outputfile.write(line)

Note: I'd move the rep declaration out before you open either file, and I removed the superfluous print call now that it's running over every line.

Additionally, I don't think you need a dictionary for your mapping. It seems to semantically make sense, but offers no actual advantage to you. Just a tuple of tuple pairs would work fine, and I'd rename it too:

replacements = (
                ("Incoming Call", "Voice (in)"),
                ("Outgoing Call", "Voice (out)"),
                ("Incoming SMS", "SMS (in)"),
                ("Outgoing SMS", "SMS (out)"),
               )

Also a minor improvement for your loop, you can use the unpacking operator to simplify things a little:

for pair in replacements:
    line = line.replace(*pair)

* is the unpacking operator, it unpacks a 2-item tuple into 2 arguments for you. I prefer this as I find it more readable and makes for a better name (pair rather than i, j).

| improve this answer | |
\$\endgroup\$
1
\$\begingroup\$

You do weird things with your context managers. Namely outputfile.close() is completely unnecessary as it is already handled by the with statement.

Also you do not need to keep the input file opened until the end of the process. You can close it right after you get the data (data = f.read()) by de-indenting by one level everything that comes after.

| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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