Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 1 down vote favorite

I wrote a code that queries a data structure that contains various triples - 3-item tuples in the form subject-predicate-object. I wrote this code based on an exercise from Programming the Semantic Web textbook.

def query(clauses):
    bindings = None
    for clause in clauses:
        bpos = {}
        qc = []
        for pos, x in enumerate(clause):
            if x.startswith('?'):
                qc.append(None)
                bpos[x] = pos
            else:
                qc.append(x)
        rows = list(triples(tuple(qc)))
        if bindings == None:
            # 1st pass
            bindings = [{var: row[pos] for var, pos in bpos.items()}
                        for row in rows]
        else:
            # >2 pass, eliminate wrong bindings
            for binding in bindings:
                for row in rows:
                    for var, pos in bpos.items():
                        if var in binding:
                            if binding[var] != row[pos]:
                                bindings.remove(binding)
                                continue
                        else:
                            binding[var] = row[pos]
    return bindings

The function invocation looks like:

bg.query([('?person','lives','Chiapas'),
          ('?person','advocates','Zapatism')])

The function triples inside it accepts 3-tuples and returns list of 3-tuples. It can be found at Refactor deeply nested if-else

The function query loops over each clause, tracks variables (strings that start with '?'), replaces them with None, invokes triples, receives rows, tries to fit values to existing bindings.

How can i simplify this code? How can i make it more functional-style, without nested for-loops, continue keyword and so on?

share|improve this question

1 Answer

up vote 3 down vote accepted
  1. Your code would be helped by splitting parts of it out into functions.
  2. Your continue statement does nothing
  3. Rather then having a special case for the first time through, initialize bindings to [{}] for the same effect.

My version:

def clause_to_query(clause):
    return tuple(None if term.startswith('?') else term for term in clause)

def compatible_bindings(clause, triples):
     for triple in triples:
        yield { clause_term : fact_term
            for clause_term, fact_term in zip(clause, row) if clause_term.startswith('?')
        }

def merge_bindings(left, right):
    shared_keys = set(left.keys()).intersection(right.keys)

    if all(left[key] == right[key] for key in shared_keys):
        bindings = left.copy()
        bindings.update(right)
        return bindings
    else:
        return None # indicates that a binding cannot be merged


def merge_binding_lists(bindings_left, bindings_right):
    new_bindings = []
    for left, right in itertools.product(bindings_left, bindings_right):
        merged_binding = merge_bindings(left, right)
        if merged_binding is not None:
            new_bindings.append( merged_binding )

    return new_bindings


def query(clauses):
    bindings = [{}]
    for clause in clauses:
        query = clause_to_query(clause)
        new_bindings = compatible_bindings(clause, triples(query))
        bindings = merge_binding_lists(bindings, new_bindings)

    return bindings
share|improve this answer

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.