5
\$\begingroup\$

I recently made a post on Code Review entitled Writing lists to .csv file. I found the feedback extremely helpful, and wished to provide a continuation of that particular topic; of working with Python and .csv files.

I therefore set the somewhat more complex challenge of creating and populating a table within a .csv file. In particular, taking on-board previous feedback, I have begun using user-defined functions to separate concerns.

This creates a .csv file from 'scratch', prompting the user for the required input along the way:

#Import modules: 
import datetime

#Declarations:
fileName = ""
dateList = []
date = ""
formattedDate = ""
revisedDateList = []
timeList = []
studyTime = ""
revisedTimeList = []
formattedTime = ""
timeRangeList = []
listOfTimes = []
eachRow = []
subject = ""
rowDate = ""
timeRangesAsString = ""
rowTextList = []


def createCsvFile() :
    #Creates and populates a .csv file, with time periods as columns and dates as rows. 

    fileName = input("Please enter a name for your file: ")
    with open(fileName+".csv","w+") as usersFile :
        revisedDateList = listOfDates()
        timeRangeList = inputTimes()
        timeRangesAsString = ",".join(timeRangeList)
        usersFile.write(","+timeRangesAsString+"\n")
        for date in revisedDateList :
            fileText = rowContent(revisedDateList,timeRangeList,date)
            usersFile.write(fileText)
    return

def listOfDates() :
    #Asks user to input dates in specified format and returns a list of strings representing dates. 

    datesAsString = input("Please enter your dates as required (dd/mmm/yy dd/mmm/yy etc.) ") # Example: 17/jan/16 18/jan/16
    revisedDateList = formatDate(datesAsString)
    return revisedDateList

def formatDate(datesAsString) :
    #Converts a list of strings, representing dates, from dd/mmm/yy to dd/mm/yyyy. 

    dateList = datesAsString.split()
    for date in dateList :
        structTime = datetime.datetime.strptime(date,"%d/%b/%y") #Example: 17/jan/16
        formattedDate = datetime.datetime.strftime(structTime, "%d/%m/%Y") # Example: 17/01/2016
        revisedDateList.append(formattedDate)
    return revisedDateList

def inputTimes() : 
    #Asks the user to input times, to be used as column headers, returns a list of strings representing time ranges. 

    studyTime = input("Please enter your study periods throughout the day (hh:mm hh:mm): ") 
    timeList = studyTime.split()
    for time in timeList :
        structTime = datetime.datetime.strptime(time, "%I:%M%p")
        formattedTime = datetime.datetime.strftime(structTime, "%H:%M")
        revisedTimeList.append(formattedTime)
    timeRangeList = formatTimes(revisedTimeList)
    return timeRangeList

def formatTimes(revisedTimeList) :
    #Grouping individual times to create time ranges:

    listOfTimes = revisedTimeList 
    iteratorList = iter(listOfTimes)
    for time in iteratorList :
        timeRange = time + " - " + next(iteratorList)
        timeRangeList.append(timeRange)
    return timeRangeList

def rowContent(revisedDateList,timeRangeList,date) :
    #Creates strings ready for writing to .csv file, as individual rows of data:

    eachRow = [] #'eachRow = []' is required to reset the variable and prevent an accumulation of data from the 'for loop'. 
    for timePeriod in timeRangeList :
        subject = input("Please enter your topic of study between " + timePeriod\
                + " on " + date + ": ")
        if timePeriod == timeRangeList[0] :
            rowData = date + "," + subject
        elif timePeriod == timeRangeList[-1] :
            rowData = subject + "\n"
        else :
            rowData = subject
        eachRow.append(rowData)
    dataToWrite = ",".join(eachRow)
    return dataToWrite

createCsvFile()
print("Writing to file complete! ")

At this stage, please note that I have included the formatting of the dates and times purely for exercise purposes; I wanted practice using the datetime module.

The code works the way it is intended. However, it does raise a number of questions in my mind. This is why I would very much appreciate the feedback of more experienced programmers.

Firstly, the code seems very long and convoluted. I have tried to separate concerns where possible, by defining my own functions. However, are there any particular lines, or segments of code, that are needlessly convoluted. Are there any sections of code that could be written more succinctly? Am I overlooking some obvious methods, classes or modules that could make this program a lot cleaner/simpler? Or is it the case, that even such simple programs require a significant amount of code?

I suppose the overriding question for me, and one I would guess plagues the mind of many a novice, is; is my code overly long and therefore not very well written, or is it meaningful and of necessary length?

This leads me nicely to my second question, as an 'apprentice' in computer programming, do you consider that I am too overly concerned at this stage with aspects of coding such as readability, efficiency and technique. Would my time be better spent learning languages in depth first, before learning about etiquette, efficiency, succinctness etc.?

Finally, would repeatedly calling the rowContent() function, within a for loop, slow processing and/or needlessly take up memory space. If so, can you think of a more eloquent solution to this particular segment of the code; this function creates and returns the strings ready for writing to file.

\$\endgroup\$
3
  • \$\begingroup\$ Could you provide an example usage session? I cannot enter a correct study time to test it. \$\endgroup\$
    – Caridorc
    Jan 24, 2016 at 11:16
  • 1
    \$\begingroup\$ @Caridorc Please excuse my ignorance, however I do not understand what you mean by 'providing an example usage session'? The input study times need to be entered in the following format (start) hh:mm (finish) hh:mm For example, 09:00AM 11:00AM. You can add several study times like so 09:00AM 11:00AM 11:15AM 01:00PM 02:00PM 03:30PM 03:45PM 05:00PM. Each pair having a start and an end time. Apologies, as I do not think I have made this clear. \$\endgroup\$ Jan 24, 2016 at 14:08
  • \$\begingroup\$ @ Caridorc, I have searched the site a little, and discovered ideone.com for sharing code, I trust this is what you mean by ‘providing example usage session’. One limitation is that you cannot create new files, therefore I have provided a working example of the specific section in question; i.e. the user input of ‘studyTime’. I hope it provides you with what you need. Thanks again. ideone.com/GWaCY7 \$\endgroup\$ Jan 24, 2016 at 15:44

1 Answer 1

3
\$\begingroup\$

Keep variables local

This part of code is very worrysome:

#Declarations:
fileName = ""
dateList = []
date = ""
formattedDate = ""
revisedDateList = []
timeList = []
studyTime = ""
revisedTimeList = []
formattedTime = ""
timeRangeList = []
listOfTimes = []
eachRow = []
subject = ""
rowDate = ""
timeRangesAsString = ""
rowTextList = []

I suggest declaring the variables just before using them inside the function that uses them, not all at once. Like this the reader will have to keep all of them in mind.

Give inputs to functions

Your functions for the most part rely on global variables. It means that any global variable may be used and changed from anywhere, a very confusing fact.

Smaller and simpler?

I wrote a smaller version of this code by myself to get an idea of how much it would take, I did not handle formatting nor of time nor of dates, just taking input and writing to the .csv and it came out 7 lines long. I think handling your formatting may take 10 more lines so less than 20 lines.

Re-use + generators + avoiding reinventing the wheel

Much of your code is overly long, look at this example:

def formatTimes(revisedTimeList) :
    #Grouping individual times to create time ranges:

    listOfTimes = revisedTimeList 
    iteratorList = iter(listOfTimes)
    for time in iteratorList :
        timeRange = time + " - " + next(iteratorList)
        timeRangeList.append(timeRange)
    return timeRangeList

Becomes:

def create_ranges(revisedTimeList) :
    for start, end in pairwise(revisedTimeList):
        yield "{start} - {end}".format(**locals())

Where pairwise may be taken from StackOverflow and list may be needed to be called on the result if you want to index on it. Creating a generator using yield is simpler than returning a whole list.

Your function was also virtually impossible to test because it relied on modifying the external variable timeRangeList.

Mine can easily be run in the interactive shell or in a doctest:

>>> list(create_ranges(["10:23", "10:45", "11:22", "13:10"]))
['10:23 - 10:45', '11:22 - 13:10']

Another example is formatDate(datesAsString) that applies a (series of) function(s) to all the items of a list, just like map does.


To sum up, here is how I write functions, I think this method will help you too.

  • Stop and think about a function before writing it, spend extra time on its name, it is more important than it seems at first glance.
  • Decide the inputs and the outputs, make it interact with the user and the file-system only if strictly necessary.
  • Decompose it into smaller chunks, describe it in English, write an example.
  • Use the chunks from the std-lib or search them on StackOverflow.
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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