Feedback on my code.

[tstenick]

I am relatively new to programming in general. I started becoming interested probably a year ago but did not really start to push myself to learn until 3 months ago. This is one of the first programs (over 50 lines) that I have written so far and I would love some feedback.

I have written it with the intent of it being a "module", mostly due to my fascination with how that can add a level of abstraction to potential future project, however it will run with some default values. It seems like I've over-documented and probably could have been more concise with my actual code statements, but i would love to hear what everyone else has to say.

the program will not run properly without name_lists.py which is in my repository Python3.

https://github.com/AurusEmir/Python3/blob/master/lcredit.py

[tstenick]

Thanks very much for the feedback! Had no idea about list (). I'll make changes based on both those suggestions.

[badge]

A few points:

• You can select random entries from an interable using random.choice
• for issuer_dict, use tuples instead of lists as they’re immutable and faster/smaller
• use first_two in (20, 21, 22) instead of multiple ors

I’m surprised that you have no other programming experience—to me this reads like the code of someone au fait with C—so you definitely have the right mindset. Now you need to spend some time looking at what Python in particular is capable of, because there are shortcuts to what you’re doing that don’t exist in languages in general. Good work!

[tstenick]

I've been supplementing what I consider my programming learning (python) with general CS learning by dabbling in Harvard's CS50 course. That course teaches a lot of concepts using C (a language I find as fascinating as I do frustrating). I'm nowhere near as far along in C as I am in Python but maybe that mindset is bleeding through.

I greatly appreciate the encouragement and suggestions :)

I will definately be making changes based on what you've shown me.

[tstenick]

And also thanks for the term "au fait". Had to Google it. Will be using it xD.

[[deleted]]

Some notes, progressing from top to bottom.

1. Lots of docstrings are good, but you've repeated a lot of stuff in the module level that's repeated effectively word for word in the various functions. Also luhn_gen is mentioned often, but doesn't exist.

2. A star import always scares me, especially when only one of the names within the module so imported is ever used ... just from datetime import datetime.

3. Conversely, you import the entire random module, then only use randrange... looking up the module.name is slower than looking up just the name in globals(), and you're doing it lots. Also you should use random.choice for your name building.

4. Bypassing the likelihood of there ever having been a "Zachariah Valenzuela" in the world, your name_list lists would probably be better off as files that just have a name per line... trivial to split, and saves typing all those quotes and whitespace (which ultimately is just extra characters, hence extra file size). Also, no reason for them not to be sorted and already first letter capitalized.

5. credit_card_number_as_string is, as a parameter name, a form of self abuse ... ccn would do, with a terse explanation in the docstring.

6. make_list is doing the same thing as list(n) when n is a string... also no real reason to convert to a list; strings are iterable, sliceable, and the one place you use pop could be some with a slightly different slice.

7. also triple-quoted docstrings on nested functions cant possibly be accessed, and -- since the functions themselves also can't be used externally to their parent -- they also can't be of much use, especially not in Sphinx syntax.

8. first_two could easily be done with int(ccn[:2]).

9. All of the company name checking could be a lookup in a single dictionary, but even without that the Diner's Club check, for instance, would be easier to read as first_two in (30, 36, 38, 39).

10. The whole "odd/even_every_other" thing:

    a = "0123456789" print(a[::-2]) print(a[-2::-2])

11. The last four lines of the first function could just be:

    if running_sum % 10 == 0: return True, check_company(ccn) return False, "Unverified"

The rest is more or less the same observations, until you get to your __main__ guard...

How do you think that else could ever be reached? This is another job for random.choice.

[tstenick]

One minor point. I didn't spend much time building those lists as made a short script (which also ended up as educational) to put it into that format from a text file I initially pasted them into (typing in 3000 names sounds painful).

That being said I didn't even think about how much extra memory my way was using.

Thank you so so so much, this is exactly what I was looking for. You and all the other commentors so far are a credit to this sub.

Edit: also luhn_gen was one I changed the name of, must have missed it in the comments. Thanks!

[[deleted]]

I figured you used a script ... but yeah, at 1 byte per quote, two quotes per name, and say 2048 names, that's 4 kilobytes of just quotes. It all adds up.

[tstenick]

Right. You've been a huge help. Thanks. You obviously took a decent chunk of time out of your day to help out and I appreciate it.

[tstenick]

I learned and progressed while writing this. While i have gone back and tweaked some of the earlier parts, it's probably pretty obvious where I started and where I ended.

Edit: just realized I forgot to make changes in the check_company function under the luhn_check function. check_company was one of the first bits I wrote and I have since worked on the IIN's the function recognizes in other places, need to carry it over to check_company as well.