Problem
I have the following function:
def dnd_return(number,my_timeout):
try:
r =requests.get("https://dndcheck.p.mashape.com/index.php?mobilenos= {}".format(number), timeout =my_timeout)
except (requests.exceptions.Timeout, requests.exceptions.ConnectionError) as err:
return "Error fetching DND info"
else:
return r.text
Then I have the following route:
@app.route('/track_mobile_numbers', methods=['POST','GET'])
def mobile():
if request.method == 'POST':
if request.form.get("number", "") != "":
number = request.form['number']
number = number.replace(" ","")
number = number.replace('+91', '')
number = number.strip()
strnumber = number
dnd_list = []
if len(number) == 10:
try:
dnd_dump = dnd_return(number,9)
json_dump = json.loads(dnd_dump)
j = json_dump[0]
if j['DND_status'] != "deactivated" and j['DND_status'] != "off":
dnd_list.append("DND status : "+j['DND_status'])
dnd_list.append("Date of Activation : "+j['activation_date'])
dnd_list.append("Preference : "+j['current_preference'])
else:
dnd_list.append("DND status : "+j['DND_status'])
except Exception as e:
dnd_list.append(e)
else:
dnd_list.append("The number should contain 10 digits")
number = number[:4]
try:
number = int(number)
num = mobileNumbers.query.filter_by(number = number).first()
return render_template('mobilenumbers.html',
strnumber = "Mobile number : " + strnumber,
operator = "Operator : " + num.operator,
state = "State : " + num.state,
dnd = dnd_list
)
except Exception as err:
return render_template('mobilenumbers.html',
error = err
)
else:
return render_template('mobilenumbers.html',
error = "Please enter a valid number"
)
else:
return render_template('mobilenumbers.html')
Is there a better way to do this? Can you recommend improvements to this code?
Solution
- You may want to use a different template for error messages. You can still reuse the overall design by inheriting from a base template.
-
You replace variable values a lot, making it hard to follow what the meaning of variables like
num
,number
andstrnumber
are throughout the code. Instead write functions which create the various transformations of the input that you need, making your variables unchanged throughout the code. For example:def format_number(number): return number.replace(" ","").replace('+91', '').strip()
Now you can get from
number
tostrnumber
without ever changing the former. - In some error cases you render the template with
error
set, but in at least one you use a different error message delivery:dnd_list.append("The number should contain 10 digits")
. This seems inconsistent. - The various large branches of code should be moved into separate functions.
- Single letter variables like
e
,r
andj
make this code hard to maintain. Yes, it’s kinda obvious from context, but the less context you have to keep in your head while developing, the better.