current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 1 down vote favorite

I have crated the following small script. I just need to improve this complicated code if possible.

def convUnixTime(t):
    expr_date = int(t) * 86400
    fmt = '%Y-%m-%d %H:%M:%S'
    d1 = datetime.date.today().strftime('%Y-%m-%d %H:%M:%S')
    d2 = datetime.datetime.fromtimestamp(int(expr_date)).strftime('%Y-%m-%d %H:%M:%S')
    d1 = datetime.datetime.strptime(d1, fmt)
    d2 = datetime.datetime.strptime(d2, fmt)
    return (d2-d1).days
share|improve this question
1  
What is your reason to do strftime followed by strptime? – Janne Karila Jun 12 '13 at 9:54
    
new to python.. don't know much.. just created from googling.. :D – Rahul Patil Jun 12 '13 at 9:56
1  
What does this function even do? – Blender Jun 12 '13 at 18:41
    
On *nix Server it's required to Convert Unix timestamp to Readable Date/time, I used this function to check User account expiry date.. – Rahul Patil Jun 13 '13 at 2:20

1 Answer 1

up vote 5 down vote accepted

I think the main issue here is that you are trying to reuse snippets from Google without trying to understand what is doing what.

Magic numbers

This might be purely personal but I find 60*60*24 much easier to understand than 86400.

Do not repeat yourself

What is the point of having fmt = '%Y-%m-%d %H:%M:%S' if later on you rewrite strftime('%Y-%m-%d %H:%M:%S') ? You could write it : d1 = datetime.date.today().strftime(fmt) making obvious to everyone that the same format is used.

Useless conversion between strings and dates

You are converting dates to string and string to date in a convoluted and probably not required way.

Usess conversion to int

You are using int(expr_date)) but expr_date is defined as int(t) * 86400 which is an integer anyway. Also, I am not sure you would need the conversion of t as an int.

Taking into account the different comments, my resulting not-tested code looks like :

def convUnixTime(t):
        return (datetime.datetime.fromtimestamp(t*60*60*24)
              - datetime.datetime.today()).days

Edit : I was using datetime.date.today() instead of datetime.datetime.today(). Also fixed the sign issue.

share|improve this answer
    
Thanks Josay... – Rahul Patil Jun 12 '13 at 12:58
    
I have tested it is giving error: TypeError: unsupported operand type(s) for -: 'datetime.date' and 'datetime.datetime' – Rahul Patil Jun 12 '13 at 13:03
    
Arg! I'll try and think about a proper working solution then :-P – Josay Jun 12 '13 at 14:02
    
If t is already in seconds, then that is what datetime.datetime.fromtimestamp is expecting, you don't want to do any arithmetic with it. Also, the returned datetime.datetime object has no days member. If you meant the day member, it is the day of the month, not the number of days. – Dan Ross Jun 12 '13 at 22:08
    
I've fixed the code and it seems to be behaving just like yours (not sure if good thing or bad thing). – Josay Jun 12 '13 at 22:19

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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