I have the following function:

def dnd_return(number,my_timeout):
        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"
        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:
                    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'])

                        dnd_list.append("DND status : "+j['DND_status'])
                except Exception as e:
                dnd_list.append("The number should contain 10 digits")

            number = number[:4]
                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
            return render_template('mobilenumbers.html',
                                    error = "Please enter a valid number"
        return render_template('mobilenumbers.html')

Is there a better way to do this? Can you recommend improvements to this code?

  • 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 and strnumber 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 to strnumber 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 and j 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.
  • \$\begingroup\$ I'm unable to understand your second point. Can you give me an example on how I can use functions for the transformations? Thanks! \$\endgroup\$ – user3620828 Jan 13 '15 at 8:38

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.