1
\$\begingroup\$

I'd like to know if I can make this code more efficient in terms of line saving, or any other way:

def missingDoor(trapdoor,roomwidth,roomheight,step):        
    safezone = []
    hazardflr = givenSteps(roomwidth,step,True)
    safetiles = []

    for i,m in enumerate(hazardflr):
        safetiles.append((m,step))
        while i < len(safetiles):
            nextSafe = safetiles[i]
            ns0 = nextSafe[0]
            if knownSafe(roomwidth, roomheight, ns0, nextSafe[1]):
                if trapdoor[ns0/roomwidth][ns0 % roomwidth] is "0":
                    if ns0 not in safezone:
                        safezone.append(ns0)
                    for e in givenSteps(roomwidth,ns0,True):
                        if knownSafe(roomwidth, roomheight, e, ns0):
                            if trapdoor[e/roomwidth][e%roomwidth] is "0" and (e, ns0) not in safetiles:
                                safetiles.append((e, ns0))
            i += 1  
    return sorted(safezone)

a community member suggest to fix the this code like this:

def missingDoor(trapdoor, roomwidth, roomheight, step):
    safezone, safetiles = [], []
    check = lambda a, b, c, l: knownSafe(roomwidth, roomheight, a, b) and       trapdoor[a/roomwidth][a%roomwidth] == '0' and c not in l

    for i, m in enumerate(givenSteps(roomwidth, step, True)):
        safetiles.append((m, step))
        for _ in range(i, len(safetiles)):
            nextSafe = safetiles[i]
            ns0 = nextSafe[0]

            if check(ns0, nextSafe[1], ns0, safezone):
                safezone.append(ns0)
                safetiles.extend([ (e, ns0) for e in givenSteps(roomwidth,ns0,True) if     check(e, ns0, (e, ns0), safetiles) ])
    return sorted(safezone)

the only issue I had with this though is when I run the code it now runs into this:

TypeError: givenSteps() takes exactly 4 arguments (3 given)

How can I make this code more efficient? I initially started to use while loops and found that for loops are better when given a known list. I've been trying all sorts of things, given that I am new to programming.

\$\endgroup\$

closed as off-topic by yuri, Sᴀᴍ Onᴇᴌᴀ, IEatBagels, t3chb0t, Ludisposed Aug 16 '18 at 9:30

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Code not implemented or not working as intended: Code Review is a community where programmers peer-review your working code to address issues such as security, maintainability, performance, and scalability. We require that the code be working correctly, to the best of the author's knowledge, before proceeding with a review." – yuri, Sᴀᴍ Onᴇᴌᴀ, IEatBagels, t3chb0t, Ludisposed
If this question can be reworded to fit the rules in the help center, please edit the question.

  • \$\begingroup\$ Welcome to CodeReview.SE! It's a bit hard to tell whether things can be done in a more efficient way without telling us what you are trying to achieve. Also, if I were you I'd keep only your final version of the code in your question as it is a bit confusing to have two very similar pieces of code. \$\endgroup\$ – SylvainD Jul 11 '14 at 15:09
  • \$\begingroup\$ Also, your indentation seems wrong for the line safetiles.append((m,step)). Can you check everything is ok ? Finally, it would be quite interesting to provide us the whole code (if it is not too much). Cheers \$\endgroup\$ – SylvainD Jul 11 '14 at 15:13
  • \$\begingroup\$ Please don't remove large sections of your question once they are posted. It can potentially invalidate existing answers or make a trail of comments unclear to a later reader. If you have any updated revision that is not drastically different from the original content, it is best to append to the existing comment indicating as such. \$\endgroup\$ – unholysampler Jul 11 '14 at 15:42
5
\$\begingroup\$

First things first, stop caring about the number of lines. Unless you are trying to make a code golf competition entry, minimizing the number of lines is the last thing you want to optimize for. This code is hard to read and follow because it is deeply nested and there is very little context to describe what is being done on each line. This code should be broken down into sub functions that encapsulate sections of work.

Compare this line

if knownSafe(roomwidth, roomheight, e, ns0):

to this line

if trapdoor[ns0/roomwidth][ns0 % roomwidth] is "0":

Without knowing anything about the code, I know that the if block is executed if the given arguments describe a location that is known to be safe. Meanwhile, I have no idea why you are indexing into this nested list structure or what the "0" represents.


is does an identity comparison. == does an equivalence comparison. The fact that Two string containing just the character 0 might not always be represented by the same object. The fact that they are is an implementation detail of the Python interpreter you are currently using. Do not use is when you are checking if two values represent the same thing.


for i,m in enumerate(hazardflr):
safetiles.append((m,step))

This is not valid Python. It is likely an issue with posting the question, but as of now, the code in the question does not compile.


Use the Python variable name conventions. Currently, you have a mix of all lower case and camel case. The Python convention is underscore_spaced.

Single letters are rarely good variable names. i is a common convention for index. However, I have no idea what m of e stands for. You also use ns0. From that code, I can see that it is the first element in the nextSafe list. However, that doesn't tell me anything. Why is nextSafe[0] different fromnextSafe[1]? If it is just the current safe location as you iterate over all of the locations, make the variable name curr_safe_location.


You need to pick better data types to represent your state. Lists and strings are great, but they aren't the best way to represent all things. A large portion of your question is describing how your code is structured and what value represent what. This is an indication that the code is hard to read and can be written better. Even if it not required, sometimes making a class is very helpful because it will give value clear descriptive names.

If strings like "X" and "0" are going to have special meanings, at the very least, you should define them as constants. This will give your code more context. Then, if the representation changes, you don't have to look for all instances of "0" and hope every occurrence means the same thing.

\$\endgroup\$

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