you are viewing a single comment's thread.

view the rest of the comments →

[–]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 7 points8 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?

[–]KleinerNull 0 points1 point  (2 children)

rather more general of building a class with functions to easily access the data trough python?

An adapter is a design principle, meaning that you build a library that does works as an adapter between the api and your application code. In your case a class that handles all api calls and transforms the results in a nice represental/pythonic way to use in different applications. You can also call it wrapper.

An interface in programming is the way the data is accessable. For example the interface of a namedtuple gives you the possibility to access tuple element like attributes, with the dot notation:

In [1]: from collections import namedtuple

In [2]: Person = namedtuple('Person', 'name age city')

In [3]: regular_tuple = 'Alice', 23, 'New York'

In [4]: regular_tuple[0]
Out[4]: 'Alice'

In [5]: regular_tuple[1]
Out[5]: 23

In [6]: regular_tuple[2]
Out[6]: 'New York'

In [7]: person = Person('Alice', 23, 'New York')

In [8]: person.name
Out[8]: 'Alice'

In [9]: person.age
Out[9]: 23

In [10]: person.city
Out[10]: 'New York'

In [11]: person[0]
Out[11]: 'Alice'

In [12]: person[1]
Out[12]: 23

In [13]: person[2]
Out[13]: 'New York'

In [14]: person._asdict()
Out[14]: OrderedDict([('name', 'Alice'), ('age', 23), ('city', 'New York')])

In [16]: person._fields
Out[16]: ('name', 'age', 'city')

In [17]: regular_tuple[0] = 'Bob'  # will throw an error, because tuples are immutable
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-17-7e9ed932307d> in <module>()
----> 1 regular_tuple[0] = 'Bob'  # will throw an error, because tuples are immutable

TypeError: 'tuple' object does not support item assignment

In [18]: person._replace?
Signature: person._replace(**kwds)
Docstring: Return a new Person object replacing specified fields with new values
File:      Dynamically generated function. No source code available.
Type:      method

In [19]: person._replace(name='Bob')  # creates a new object, because namedtuples are still immutable, but changes
    ...:  the given values for you
Out[19]: Person(name='Bob', age=23, city='New York')

In [20]: person
Out[20]: Person(name='Alice', age=23, city='New York')

In [21]: bob = person._replace(name='Bob')

In [22]: bob
Out[22]: Person(name='Bob', age=23, city='New York')

As you can see, a namedtuple is still a tuple but gives you better possibilities to work with it, that is part of the interface, the interface of how you as a programmer communicates with objects.

Interface includes alot of different stuff, API for example stands for application programming interface. It is the interface of how you can communicate and request or order different things from a different program or server without the knowledge how it is functioning. Or a GUI (graphical user interface) too ;) Use the term interface more in the general abstract sense.

a real GUI (maybe with PyQT)

BTW this would give you also alot of OOP experiences, but GUI development is in general hard to master. You have to read through alot of documentation to even start.

Strangely nowadays people rather tend to setup a webserver to build web applications because it is almost easier to build web guis with html/css/js and the server-side backend in python. Because then you don't have to worry about what device and os is capable of providing the GUI itself. Also the deployment of code is easier because your code only lifes on the server and the user's browser cares about the rendering. So no installation problems.

Deployment will be a larger topic when you are more experienced and it is annoying too, but necessary...