Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

I have a Python script that generates a dictionary of all possible 3 digit password combinations. It essentially works by just nesting for loops. This is fine, but say I wanted to change it so it could generate 4, 5, 6, ... digit combinations. This would require me to keep adding more nested loops and changing the print statement.

How can I make my current code more efficient and apply the DRY principle? This way, if I wanted to change it to generate 4-digit codes, I could simply change a single variable rather then add an entire for loop.

Current code:

f=open('New Text Document.txt','w')
def getCombos(digits, caps, numbers):
    if(caps == True):
        if(numbers == True):
            for x in range(33, 126):
               for y in range(33, 126):
                   for z in range(33, 126):
                       print(chr(x) + "" + chr(y) + "" + chr(z))
                       f.write(chr(x) + "" + chr(y) + "" + chr(z) + "\n")

getCombos(3, True, True) 

If I wanted to add a fourth digit:

f=open('New Text Document.txt','w')
def getCombos(digits, caps, numbers):
    if(caps == True):
        if(numbers == True):
            for x in range(33, 126):
               for y in range(33, 126):
                   for z in range(33, 126):
                       for m in range(33, 126):
                       print(chr(x) + "" + chr(y) + "" + chr(z) + "" + char(m))


getCombos(3, True, True)
share|improve this question
up vote 15 down vote accepted

Please take a look at itertools.product. It will simplify your code greatly.

def getCombos(digits, caps, numbers):
    # Build set of choices appropriately based on caps and numbers
    choices = list(range(33, 126))
    for vals in itertools.product(choices, repeat=digits):
        print ''.join(map(chr, vals))

getCombos(3, True, True)
share|improve this answer
    
You will probably want to apply chr to choices beforehand, especially because in my experience, map is really slow... – Cilyan Feb 8 at 21:58
    
Ok. I figured there was some peebuilt class or function for it. This helps also, I was looking for a slightly different approach. +1 though for the info it helps – Ashwin Gupta Feb 8 at 23:37

Instead of building characters using integers and chr, you can rely on the pre-built ones in the string module:

import string

def getCombos(digits, caps, numbers):
    symbols = string.ascii_lowercase
    if caps:
        symbols += string.ascii_uppercase
    if numbers:
        symbols += string.digits

    # process symbols to do what you aim for
share|improve this answer
    
This I did not know. Python continues to suprise me in lots of great ways. Its power and shortcuts are incredible. I'm so used to Java where I have to do everything manually, no quick libs or functions (except g2d). – Ashwin Gupta Feb 9 at 14:47

In addition to @SjoerdJobPostmus's super tip, you can also use recursion, for example:

def print_combos(digits, prefix):
    if digits == 0:
        print(prefix)
        return
    for x in range(10):
        print_combos(digits - 1, '{},{}'.format(prefix, x))

print_combos(4, '')

There are some other issues in your code.

Instead of if(caps == True): you can write simply if caps:

You opened a file for writing but you forgot to close it. It's recommended to use a context manager, like this:

def getCombos(digits, caps, numbers):
    if caps:
        if numbers:
            # ...

with open('New Text Document.txt','w') as f:
    getCombos(3, True, True) 
share|improve this answer
    
+1 for all the things I missed in my haste, especially using with. But, make sure the with is around the calling of getCombos, and not just the definition of it (because then, the file will be closed again by the time you call the function). – Sjoerd Job Postmus Feb 8 at 21:42
    
@SjoerdJobPostmus uggh, thanks, I totally screwed that up, fixed now – janos Feb 8 at 21:46
    
This is very helpful. Didn't think of recursion. +1. When i get home I'll read this more carefully. – Ashwin Gupta Feb 8 at 23:38

My immediate reaction would be to just count with normal integers, and convert each to a string before use. Not entirely sure how that would work out for performance, but it seems like it would at least be simple and avoid repetition.

As far as example code goes...well, my knowledge of Python isn't quite to the point that I consider it exemplary (and that's putting it nicely) so I hope you'll forgive me to writing something in C++ instead.

I'd write code something on this order:

std::string cvt(int in, int lower, int range) {
    std::string ret;
    while (in) {
        ret.push_back(in % range + lower);
        in /= range;
    }
    return ret;
}

void gen_combos(int digits) {
    int lower = 33;
    int upper = 126;
    int range = upper - lower;

    int max = std::pow(range, digits);

    for (int i = 0; i < max; ++i)
        std::cout << cvt(i, lower, range) << "\n";
}

Do note that I'm not particularly advocating C++ for this task--it's almost certain to be I/O bound with any reasonable language; the only advantage to C++ (for me) is that I know it better than I do Python.

Looking at your code itself, I think the style is open to improvement. Right now, you have 33 and 126 sprinkled throughout the code; I'd try to give them more meaningful names.

I'd also avoid doing comparisons like if (x == True) and instead just use if (x)1.

If you're going to test for both x and y being true before you do something, I'd prefer to use a logical and instead of nesting the if statements like:

if (x):
    if (y):
        // ...

It seems to me this is really generating things more than it's getting those things, so I'd at least consider changing the name to gen_combos (as above) or generate_combos or something on that general order.


1. Your code doesn't seem to do anything unless both numbers and caps are both true. In the code above, I just skipped these checks, at least for the moment. I think the real intent is that they probably control what characters can appear in the result string, so (for example) if caps is false, it's probably not supposed to use any capital letters--but that's just a guess, so I left it alone for now.

share|improve this answer
    
Could you provide example code? I don't exactly get what you mean. – Ashwin Gupta Feb 8 at 23:39
    
A quick question since I don't really know any C++. What is std:: ? Is that like a static reference in java? (btw, your subnote 1 is correct) – Ashwin Gupta Feb 9 at 4:54
    
@AshwinGupta it's a namespace, like a package in Java (or Python). – Boris the Spider Feb 9 at 14:47
    
+1. (Forgot to tell you yesterday), helpful addition. The logical structure is very organized here. Also, couldnt you do something like import std or include std or whatever the cpp equivalent is? (@BoristheSpider thanks) – Ashwin Gupta Feb 9 at 14:50
    
@AshwinGupta Yes, one could - but would be very bad practice. – Boris the Spider Feb 9 at 14:51

@SjoerdJobPostmus made a good answer, but it's not very fast. Well it's really slow.
@Cilyan in his comments said that map is slow, while this isn't strictly true, it's correct.
Currently rather than the algorithm being \$O(n)\$ to change the range to characters, you and Sjoerd have a speed of \$O(kn^k)\$. To get \$O(n)\$ is easy, change it at creation.
And so I would encourage the following:

def get_combos(digits, caps, numbers):
    choices = map(chr, range(33, 126))
    for vals in itertools.product(choices, repeat=digits):
        print ''.join(vals)

You should split creation and display into two functions, to follow the single responsibility principle. So you should change it to:

def get_combos(digits):
    return (''.join(vals) vals in itertools.product(map(chr, range(33, 126)), repeat=digits))

I think the comprehension here is ugly, and can be better expressed by map. I also think the ~35% increase in speed is also quite nice. And so I would change it to:

def get_combos(digits):
    return map(''.join, itertools.product(map(chr, range(33, 126)), repeat=digits))

And usage would now be:

print('\n'.join(get_combos(3)))
share|improve this answer
    
Intresting. Normally I dont worry about speed, of course for a program like this speed is critical. An excellent addition here to the previous answer +1. – Ashwin Gupta Feb 10 at 0:15
    
@ashwingupta I like to take speed into account in most things, just be careful of premature optimisations. (also the single print should be faster than tons of prints) – Joe Wallis Feb 10 at 0:37
    
Yeah. Speed is always important of course in the real world. However, for me normally I just program for fun so I don't really mind since my things don't use much memory or resources so they run relatively quickly anyways without needing me to omptimize them for speed. – Ashwin Gupta Feb 10 at 1:27
1  
You should also integrate @Mathias Ettinger's answer about cleanly generating the symbols. It won't be a oneliner anymore, but it removes the magic numbers 33 & 126. – oliverpool Feb 10 at 7:36

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.