Displaying member information from a CSV file

Question

The code is about displaying membership information of all clients from database. I was just wondering what I could do to improve/change it.

def optionA():
 global DoAnother
 while DoAnother == True:
 print("")
 member = input("Please enter the member number: ")
 if len(member) !=4:
 print ("")
 print("The member number is not valid")
 DoAnother = True
 else:
 global memberfile
 memberfile = open("members.txt","r+")
 searchMem = True
 for line in memberfile.readlines():
 listed = line.split(",")
 if member in listed:
 print("")
 print("Team Code: {0}".format(listed[0]))
 print("Member number: {0}".format(listed[2]))
 print("Date of joining: {0}".format(listed[1]))
 print("Membership type: {0}".format(listed[3]))
 print("Amount paid: {0}".format(listed[5]))
 searchMem = False
 nextchoice()
 if searchMem == True:
 optionA()
def optionB():
 print("")
 print("Team Code Member No Fee Amount Paid Amount Outstanding")
 print("----------------------------------------------------------------------------------------")
 global memberfile
 memberfile=open("members.txt","r+")
 amountOut = 0
 for line in memberfile.readlines():
 listed = line.split(",")
 if listed[3] =="F" and listed[5].strip('\n')<listed[4]:
 difference = (int(listed[4])-int(listed[5]))
 amountOut = (amountOut + difference)
 print("{0} {1} £{2} £{3} £{4}".format(listed[0], listed[2], listed[4], listed[5].strip('\n'), difference))
 print("")
 print(" Total Outstanding: £{0}".format(amountOut))
 nextchoice()

• user103507: What does your member.txt file look like?
• martineau: choiseunghwan: You've left out a lot of code. What's the main loop look like? What does next_choice() do? How/where isDoAnother initialized? Also what version of Python are you using?
• Grajdeanu Alex: Please don't let us guess how you handle these functions. If you want a proper review, give us some proper code to be reviewed

Answer 1

There are a number of issues with this code; the first two items below are seriously bad habits.

• Inappropriate use of functions. Functions are not goto labels. Having optionA call itself or nextchoice() makes no sense. This must be corrected — it is a fundamental misunderstanding of how to write programs.
• Use of global variables. If you design your functions properly, there should be no need for any global variables.
• Lack of abstraction. There is nothing in common between these two functions, other than the fact that they both read from members.txt. It would be a good idea to have a function to read, interpret, search the file. Furthermore, since the data are in a csv format, you should use the Python csv module.

• Using string comparison for numbers. This is a bug:

  listed[5].strip('\n')<listed[4]
  

  ... because you are comparing numbers ASCIIbetically.

• Lack of documentation. optionA and optionB are both mysterious names for functions. It's not immediately clear what the members.txt file looks like, or why optionB ignores rows whose membership type column is not F.
• File descriptor leak. If you call open() without a corresponding call to close(), you might leak file descriptors. In Python, you almost always want to call open() using a with block.

• Member search by all attributes. You ask the user to enter a member number. However, when you do

  listed = line.split(",")
  if member in listed:
   ...
  

  ... you are actually searching all fields, not just the third column.

• Python style guide. PEP 8, the official Python style guide, says that variable names should be lower_case_with_underscores unless you have a good reason to deviate from the norm.

Suggested solution

import csv
from decimal import Decimal
MEMBERS_TXT_FIELDS = [
 ('Team Code', str),
 ('Date of joining', str),
 ('Member number', str),
 ('Membership type', str),
 ('Fee', Decimal),
 ('Amount paid', Decimal),
]
def members(member_number=None):
 with open('members.txt') as member_file:
 csv_file = csv.reader(member_file)
 for row in csv_file:
 member = {
 field_name: conv(val)
 for (field_name, conv), val in zip(MEMBERS_TXT_FIELDS, row)
 }
 if member_number is None or member_number == member['Member number']:
 yield member
def show_single_member(member_number):
 for member in members(member_number):
 print("""
Team Code: {Team Code}
Member number: {Member number}
Date of joining: {Date of joining}
Membership type: {Membership type}
Amount paid: {Amount paid}""".format(**member))
 return True
 return False
def show_owing_members():
 def value_fmt(field_def, member):
 field_name, width = field_def
 value = member[field_name]
 if isinstance(value, Decimal):
 return "£{0}".format(value.quantize(Decimal('0.01'))).rjust(width - 1) + ' '
 else:
 return str(member[field_name]).ljust(width)
 TABLE_FIELDS = [
 # Field name, width
 ('Team Code', 11),
 ('Member number', 20),
 ('Fee', 9),
 ('Amount paid', 17),
 ('Amount outstanding', 31),
 ]
 # Print headings
 print(''.join(field.ljust(width) for field, width in TABLE_FIELDS))
 print(''.join('-' * width for field, width in TABLE_FIELDS))
 total_outstanding = Decimal('0.00')
 for member in members():
 if member['Membership type'] == 'F' and member['Amount paid'] < member['Fee']:
 member['Amount outstanding'] = member['Fee'] - member['Amount paid']
 print(''.join(value_fmt(field_def, member) for field_def in TABLE_FIELDS))
 total_outstanding += member['Amount outstanding']
 print()
 w = sum(width for field, width in TABLE_FIELDS if field != 'Amount outstanding')
 print("Total outstanding: ".rjust(w) + "£{0}".format(total_outstanding))
def menu():
 while True:
 print("A: Member lookup")
 print("B: Outstanding fees")
 print("Q: Quit")
 choice = input("Choice: ").upper()
 if choice == 'A':
 ok = show_single_member(input("Please enter the member number: "))
 if not ok:
 print("No such member")
 elif choice == 'B':
 show_owing_members()
 elif choice == 'Q':
 break
 print()

• Alan Kavanagh: Hopefully he signs up and checks out this fineee solution !

Answer 2

I would implement something more functional, that way if you go on to create a larger business program you could debug, and section your needs better.

#seperating the formatting/print function from the lookup would be alot cleaner
#and allow for long-term changes without having to mess with the parts that work
def getMemberInfo():
 #you also don't need loop values, rather keep testing bad input
 #as the looping mechanism
 member = input("Please enter the member number: ")
 while len(member) != 4:
 member = input("\nPlease enter a valid member number: ")
 #cleaner way to open files
 with open("members.txt", "r+") as memberFile:
 for line in memberFile:
 listed = line.split(",")
 if member in listed:
 #append the difference value, to avoid anything other
 #than formatting in the second function
 listed.append(int(listed[5]) - int(listed[4]))
 return listed
 #else: read next line
 return(None) # member wasn't found in file 
def printMemberInfo( listed ): 
 print("")
 print("")
 print("Team Code: {0}".format(listed[0]))
 print("Member number: {0}".format(listed[2]))
 print("Date of joining: {0}".format(listed[1]))
 print("Membership type: {0}".format(listed[3]))
 print("Amount paid: {0}".format(listed[4]))
 #you can play around with the formatting
 #exceeding col: 80 was irking me
 print("{0} {1} £{2} £{3} £{4}".format( \
 listed[0], listed[2], listed[4], listed[5].strip('\n'), listed[6]))
 print("")
 print(" Total Outstanding: £{0}".format(listed[6]))

With this code, you could have something that determines what the user wants to do, maybe getMemberInfo(), updateMemberInfo(), deleteMember(), and then can call those respective functions inside a main(): eventloop!