8

I've got a thing that you can do engine.setState(<state class>) and it will instantiate the class type you give it and start running on the new state.

In SelectFileState there is a button to go to NewFileState, and on NewFileState, there is a button to go back to SelectFileState.

Now, at the beginning of SelectFileState, I'm importing NewFileState (So I can later in the class do engine.setState(NewFileState). At the beginning of NewFileState, I'm also importing SelectFileState (So I can later go back to SelectFileState).

However, this creates a circular import, as described in some other posts. Some say that circular imports are indicators bad design, and should be refactored..

I know that I can just fix this problem by importing SelectFileState right before I need to use it, but I'd rather do things the right way and refactor it.

Now I'm wondering though.. How would you refactor that out?

Thanks.

Edit: Pydsigner suggests that I merge the two files into one, as they are both very related to each other. However, I cannot put EVERY state that has a circular dependency into one file, so there's got to be a better method for that. Any ideas?

2Edit: I'm circumventing this problem for now by not using the from x import y syntax, and instead just doing import x. This is not a preferable solution, and I'd like to know the "Pythonic" way to fix this kind of thing. Just merging files together can't be the fix forever.

The code:

SelectFileState

from states.state import State
from states.newfilestate import NewFileState

from elements.poster import Poster
from elements.label import Label
from elements.button import Button
from elements.trifader import TriFader

import glob
import os

class SelectFileState(State):
    def __init__(self, engine):
        super().__init__(engine)

    def create(self):
        self.engine.createElement((0, 0), Poster(self.engine.getImage('gui_loadsave')), 1)
        self.engine.createElement((168, 30), Label("Load a game", 40), 2)
        self.engine.createElement((400, 470), Button("New save", code=self.engine.createElement, args=((0, 0), TriFader(NewFileState, False), -240)), 3)

        ycounter = 150

        globs = glob.glob("save\\*.mcw")
        for file in globs:
            self.engine.createElement((200, ycounter), Button(os.path.basename(file)[:-4]), 2)
            ycounter += 50

NewFileState

from states.state import State
from states.selectfilestate import SelectFileState

from elements.poster import Poster
from elements.label import Label
from elements.button import Button
from elements.inputbox import InputBox
from elements.trifader import TriFader


class NewFileState(State):
    def __init__(self, engine):
        super().__init__(engine)

    def create(self):
        self.engine.createElement((0, 0), Poster(self.engine.getImage('gui_loadsave')), 1)
        self.engine.createElement((135, 30), Label("Make a new save", 40), 2)

        self.lvlname = self.engine.createElement((180, 212), InputBox(length=25, text="World name"), 2)
        self.engine.createElement((200, 240), Button(text="Ok", code=self.createSave, args=()), 2)

    def createSave(self):
        open("save\\" + self.lvlname.getText() + ".mcw", 'w')
        self.engine.createElement((0, 0), TriFader(SelectFileState), -240)
6
  • See this answer: stackoverflow.com/a/3956038/855026 which explains that it can be helpful to only import the namespace/module instead of using the from..import syntax.
    – Brian L
    Commented Nov 13, 2012 at 2:40
  • Just a general note: You really do not need the __init__() methods in either class. All you are doing is making your code more redundant and slower.
    – pydsigner
    Commented Nov 13, 2012 at 2:51
  • @pydsigner Yes I do. The element __init__ routine does important stuff like interfacing with the engine and setting some variables.
    – SuperDisk
    Commented Nov 13, 2012 at 2:53
  • 1
    Ah, but you forget that you automatically inherit your parent's methods, including __init__(). Unless you are tweaking something, you don't need to override it.
    – pydsigner
    Commented Nov 13, 2012 at 2:56
  • @pydsigner Doh! I already knew that, but it'd become habit to write it. Thanks for pointing that out.
    – SuperDisk
    Commented Nov 13, 2012 at 3:19

4 Answers 4

4

Without seeing code, what would make the most sense is to merge the two files. If they are that closely intertwined, you could probably put them together without anything really oddly out of place.

6
  • I suppose, but is this just a quick fix or the 'actual' way to do it?
    – SuperDisk
    Commented Nov 13, 2012 at 2:19
  • Again, I haven't seen your code, but this is generally an indication that the twain should become one. There really aren't any other good ways to do this. Circular import problems indicate structure problems, and such a problem is what I'm somewhat blindly trying to fix.
    – pydsigner
    Commented Nov 13, 2012 at 2:21
  • I just added it, however I'm not sure how this'll help with explaining the problem. It's just a little more murky of an explanation.
    – SuperDisk
    Commented Nov 13, 2012 at 2:28
  • Oh, that does help. From seeing that code, it would be an extremely advisable thing to merge the two modules. They aren't 1000 line monoliths, and, for instance, imports are quite redundant. Merge them.
    – pydsigner
    Commented Nov 13, 2012 at 2:31
  • I accepted your answer for this particular instance, however, I cannot merge every state into one file.. How might I go about the others that are not so intertwined?
    – SuperDisk
    Commented Nov 13, 2012 at 20:19
2

In Python imports don't have to appear at the beginning of module. In fact they can appear in functions, so in NewFileState.py you could move the import of SelectFileState into NewFileState.create and you could make a similar change to SelectFileState.py

3
  • I mentioned this (I don't want to do it) in the original post.
    – SuperDisk
    Commented Nov 13, 2012 at 2:40
  • 1
    This is the simplest fix and would be easy enough to read and maintain
    – jsbueno
    Commented Nov 13, 2012 at 2:43
  • 1
    No, imports are to be at the beginning of the file (PEP 8). Just because it works in a 20 line file doesn't make it advisable.
    – pydsigner
    Commented Nov 13, 2012 at 2:49
0

What you can do here, is since you only will need the reciprocal classes when the code is running, classes are instantiated and methods are called, is to make the class names available to the parent module - "state" and import your names from there:

state.init.py

from states.selectfilestate import SelectFileState
from states.newfilestate import NewFileState

At this point both submodules will be initialized - and even though during the execution of the body they will "see" an incomplete version of the states module, this module is not being accessed at this stage. When any of the classess is actually instantiated, states.__init__.py will have finished executing, and the module reference to states will point to the complete module object.

In state.selectfilestate:

import states
...
        self.engine.createElement((0, 0), TriFader(states.SelectFileState), -240)

And on state.newfilestare:

import states
...
        self.engine.createElement((0, 0), TriFader(SelectFileState), -240)
0

You could hide the actual class references in a dictionary mapping, and instead of passing the class reference you pass a constant value which maps to the real class. This can be kept just as a dict in a standalone module, or it could be wrapped in a separate StateManager class who handles retrieving the next state.

The issue with this approach is that you need to manually update the list of states and mappings from constants to class references.

Here's an example implementation:

states.const

# This module is states.const

(
    STATE_SELECT_FILE,
    STATE_NEW_FILE,
) = range(2) # manually update this number when you add/remove states

states.mapping

# This module is states.mapping
from states.const import *
from states.newfilestate import NewFileState
from states.selectfilestate import SelectFileState

STATE_MAPPING = {
    STATE_SELECT_FILE : SelectFileState,
    STATE_NEW_FILE : NewFileState,
}

SelectFileState

from states.const import STATE_NEW_FILE
# ... snip ...
... TriFader(STATE_NEW_FILE, False) ...

NewFileState

from states.const import STATE_SELECT_FILE
# ... snip ...
... TriFader(STATE_SELECT_FILE) ...

engine.setState()

from states.mapping import STATE_MAPPING

def setState(class_key):
    obj = STATE_MAPPING[class_key]()
    # ... do other stuff ...

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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