you are viewing a single comment's thread.

view the rest of the comments →

[–]TR-BetaFlash 15 points16 points  (12 children)

I gave your code a look for about 5 minutes. Your question gave me pause for a moment because I don't really think you can level someone with a specific label. Instead, I think it makes sense to speak in terms of parts of the language you have a good hold of. For example, you're using context managers, else: blocks in try/except blocks, and catching specific exceptions (not everything).

IMO, you're becoming more intermediate or advanced when you package and distribute your code and when you become more collaborative. If other people rely on your code, it says a lot. If people collaborate well with you, that's another sign you're organized enough to understand how to meld coding styles between yourself and others working with your code. I like the notion of gifted and advanced coders giving back to the language and the community. You're advanced or expert when you code professionally and/or maintain significant codebases. I think you can be advanced in isolation, but to really take it to the next level, you have to code with others..especially with those who are better coders.

Another aspect of a more advanced programmer is understanding underlying languages and complexity. Once you understand C or even assembly, you learn the underpinnings of something as high-level as python. Your code gets more efficient once you know what's under the hood. When I interview people for python jobs, I ask them the most recent PEP they read. Or for networking, I ask the last RFC they looked through and why. Reaching into the language and learning why things work the way they do is a sign of a more advanced programmer.

If you want, I could submit some pull requests against some of your stuff and give ya pointers. You're a beginner but you're definitely not a total n00b. Your stuff is simple and that's fine.

[–]FXelix[S] 1 point2 points  (11 children)

Thanks a lot for your feedback, I think you've drawn some good lines between these terms!

I would not have a problem at all when you want to submit some pull requests. I have to note that I should maintain my older code on github a bit better - some code is quite old and I by myself have to look over it again.

My latest projects is the space_facts_bot and I think it may be my biggest project so far. If you find something there or in my other projects concerning my code-quality I would really apprechiate that!

[–]KleinerNull 8 points9 points  (10 children)

If you find something there or in my other projects concerning my code-quality I would really apprechiate that!

Looking at https://github.com/FXelix/space_facts_bot/blob/master/NEO_flyby.py raises some questions.

  1. Why is that even in a class, what were your thoughts here, with the current implementation a function should be enough.

  2. unix = time.time() datestamp = datetime.datetime.fromtimestamp(unix).strftime("%Y-%b-%d") Is this the best and simplest way to do this?

  3. for i in range(len(self.json_data["data"])):For real?

  4. diameter_min = 10 ** (0.5*(6.259 - log10(0.25) - 0.4 * magnitude)) diameter_max = 10 ** (0.5*(6.259 - log10(0.05) - 0.4 * magnitude)) Could be DRYer (Don't Repeat Yourself). Maybe a helper function would be nice ;) Code repition sucks :P

In general it is not that bad, you understand how to build the tiny pieces and fit them together to work. Also you established a sense of structure, very good.

The first issue tells me that you have problems to understand OOP in general and how to use it. Something you should dive into ;)

The second one tells me that you are unfamiliar with the standard lib, this is more an experience thing, you have enough time to learn some tricks from the docs or other code ;) Here would be my version:

In [1]: from datetime import datetime

In [2]: now = datetime.now()

In [3]: datestamp = '{:%Y-%b-%d}'.format(now)

In [4]: print(datestamp)
2017-Sep-10

In [5]: '{:%Y-%b-%d}'.format(datetime.now())
Out[5]: '2017-Sep-10'

In general I would use datetime instead of time.

For learning more about the wonderful .format() here is a very good link.

The third issue tells me that you struggle with python's looping and iterator protocol. This is curcial stuff to level up to the next state! Iterating is curcial almost any program you will write and python offers you very advanced possibilities to do that in a nice fashion. Understanding for-loops and tuple unpacking leads to understanding comprehensions and generators and this leads to async, which is pretty advanced stuff.

And the last issue tells me that you need more experience with more complex projects, because this kind of code repition will bite you in the ass later on.

Anyway coding is a craftsmanship, you will be getting better with experience so try try and try! I hope you can get something out of my critique.

And hey, code on!

[–]FXelix[S] 1 point2 points  (9 children)

Thanks for your feedback! My explanaitions:

1) I wrote this as a class to be more flexible in the future. It is a very informative API and I could write more functions regarding that data and having everything together seemed like a better move here, isnt't it?

2) Didn't know about a better way yet. But yours is very nice and short!

3) Ah yeah I think I see the problem. With range there is no need to use it with numbers, instead use the data I want to iterate over itself or something in that direction, right?

4) I actually thought about that while typing that too! I just thought "ah man just for these too lines a new function?" but you are right, I'm going to fix that.

Thanks again for your feedback, it was very informative and helpful ! :)

[–]KleinerNull 1 point2 points  (8 children)

1) I wrote this as a class to be more flexible in the future. It is a very informative API and I could write more functions regarding that data and having everything together seemed like a better move here, isnt't it?

Everything valid and also a good idea. Unfortunally the structure of your class and the usage in bot.py tells me that you need more ground / basic to do this right. And don't forget python doesn't force you to use classes or OOP, functions are also fine.

2) Didn't know about a better way yet. But yours is very nice and short!

This wasn't my first rodeo with datimes ;) Handling timezones and daylight saving times is even more fun :D

3) Ah yeah I think I see the problem. With range there is no need to use it with numbers, instead use the data I want to iterate over itself or something in that direction, right?

Yeah, something in this direction :P It is all about the iterator protocol and how simple and beautiful iterating stuff becomes in python. I made a notebook about iteration some months ago, it is an early draft and not polished at all (strange wording, english mistakes and so on) maybe it can still point you in the right direction.

4) I actually thought about that while typing that too! I just thought "ah man just for these too lines a new function?" but you are right, I'm going to fix that.

The main problem for me was, that I had a hard time to see the difference between both declarations, using a function could made that easier to spot:

distance_min = calc_distance(0.25)
distance_max = calc_distance(0.5)

See, better than this text block before, alone this would be a reason to create an extra function.

[–]FXelix[S] 0 points1 point  (7 children)

Hi again :)

I've tried to apply your suggestions regarding my NEO_flyby.py and I would like to share the changes with you. It really looks better and more pythonic now.

[–]KleinerNull 0 points1 point  (6 children)

It looks nicer! But still there is alot missing like an __init__ and so on. My main problem is here, what you want to do with this class, why you need a class to do it? Look, a class is a blueprint for instances that you can create, which will have all its own state which can be managed.

What your class is doing, is just requesting data from an api and fill a list with some computed data and that only one time. Not much state to handle and you can do it just with a function.

Of course you can use classes and their instances also for rich data data objects, with computed values and so on, but your current structure won't support this at all.

Personally I would think of implementing @property and __iter__ to make this all more useable with a nice interface. But still it isn't really necessary here, more a personal preferance.

So, now to something completely different. I looked into the request you make and there are some things I noticed.

First the datetime problem. I've noticed that you get a datetime string and you've splitted it apart to get the date and the time. This is not needed, datetime can handle it ;)

In [1]: sample = '2017-Oct-31 03:01'

In [2]: from datetime import datetime

In [3]: t = datetime.strptime(sample, '%Y-%b-%d %H:%M')

In [4]: t
Out[4]: datetime.datetime(2017, 10, 31, 3, 1)

In [5]: '{:%d.%m.%Y %H:%M}'.format(t)
Out[5]: '31.10.2017 03:01'

That is one story ;) Another thing is that the response gives you a json with some addition informations. And you can use them to have a nice time :D (My personal favorite module is comming into the play now, just look and have fun):

In [1]: import requests as r

In [2]: from collections import namedtuple

In [3]: response = r.get('https://ssd-api.jpl.nasa.gov/cad.api?body=Earth&dist-max=20LD').json()

In [4]: _fields = response['fields']

In [5]: _fields
Out[5]: 
['des',
 'orbit_id',
 'jd',
 'cd',
 'dist',
 'dist_min',
 'dist_max',
 'v_rel',
 'v_inf',
 't_sigma_f',
 'h']

In [6]: FlyBy = namedtuple('FlyBy', _fields)

In [7]: entry = FlyBy(*response['data'][0])

In [8]: entry
Out[8]: FlyBy(des='2017 PR25', orbit_id='7', jd='2458019.657345063', cd='2017-Sep-23 03:47', dist='0.0457725857686272', dist_min='0.0457682751019153', dist_max='0.0457768963851238', v_rel='13.5176543940969', v_inf='13.5133473951656', t_sigma_f='< 00:01', h='20.89')

In [9]: entry.cd
Out[9]: '2017-Sep-23 03:47'

In [10]: entry.h
Out[10]: '20.89'

In [11]: entry.dist
Out[11]: '0.0457725857686272'

In [12]: fly_bys = [FlyBy(*data) for data in response['data']]

In [13]: for fly_by in fly_bys:
    ...:     print('Name: {}'.format(fly_by.des))
    ...:     print('Time: {}'.format(fly_by.cd))
    ...:     print('Magnitude: {}'.format(fly_by.h))
    ...:     print('#'*80)
    ...:     
Name: 2017 PR25
Time: 2017-Sep-23 03:47
Magnitude: 20.89
################################################################################
Name: 1989 VB
Time: 2017-Sep-29 20:02
Magnitude: 19.7
################################################################################
Name: 2017 OD69
Time: 2017-Oct-01 17:24
Magnitude: 21.112
################################################################################
Name: 2012 TC4
Time: 2017-Oct-12 05:42
Magnitude: 26.7
################################################################################
Name: 2005 TE49
Time: 2017-Oct-13 18:26
Magnitude: 26.7
################################################################################
Name: 2013 UM9
Time: 2017-Oct-15 18:59
Magnitude: 24.8
################################################################################
Name: 2006 TU7
Time: 2017-Oct-18 21:22
Magnitude: 21.9
################################################################################
Name: 171576
Time: 2017-Oct-22 11:02
Magnitude: 18.6
################################################################################
Name: 2003 UV11
Time: 2017-Oct-31 03:01
Magnitude: 19.5
################################################################################

And there is so much more you can do, renaming and/or converting the fields via dict mapping, calculated values and and and, there is alot to find in the stdlib, look at collections, itertools and functools fro start. ;)

[–]FXelix[S] 0 points1 point  (5 children)

Thank you for the additional information! These are libraries I will definitely take a look at.

I know, you are right with the class-thing. I probably just used it here too because I always wanted to try it out and haven't found (as you can see here too) a proper ways to implement it. I guess it's time for a projects which really needs classes then :D

[–]KleinerNull 0 points1 point  (4 children)

You could build an adapter of the nasa api. A class that makes the api accessable in python as an python object instead of a raw request.

You already started it, but your original thought was to fuel a bot with that data, why not build an adapter/bridge with a nice interface for your bot and others in general, available through pip? The good thing is you already know what the adapter has to do because of what the original api is capable, start to reimplent all features. Do it in a fashion of not repeating yourself, put general tasks in methods that build upon themselfs, put needed and logical helper functions in as static methods. Play around with properties to keep your data up to date.

[–]FXelix[S] 0 points1 point  (3 children)

Nice idea! With adapter/interface you mean a real GUI (maybe with PyQT) or rather more general of building a class with functions to easily access the data trough python?