Re: how to refractor this ?

On 19/03/2014, at 5:07 AM, Roelof Wobben wrote:

> I have this piece: 
> 
> siblings(Child, Sibling) :-
>   same_father(Child, Sibling),
>   Child \= Sibling.
> siblings(Child, Sibling) :-
>   same_mother(Child, Sibling),
>   Child \= Sibling.
> 
> display_siblings(X,Y):-
>     setof(X-Y, (siblings(X,Y), X @< Y), Sibs),
>     write(Sibs).
> 
> brother_child(Child, Brother) :- 
>     siblings(Child, Brother), 
>     male(Brother).
> 
> display_brother(X,Y) :-
>      setof(X-Y, (brother_child(X,Y), X @< Y),Brother),
>      write(Brother).
> 
> sister_child(Child, Sister) :- 
>     siblings(Child, Sister), 
>     female(Sister).
> 
> display_sister(X,Y) :-
>      setof(X-Y, (sister_child(X,Y), X @< Y),Sister),
>      write(Sister).
> 
> As you can see the display are almost the same except the calling rule and the name of the variable.
> 
> Is there a way I can make one big display function. 
> and can I refractor more here ?

(1) The word is 'refactor' = 're-' + 'factor'.

(2) Before considering refactoring, you should ask whether
    the code in fact works.

    If you want to display the sisters of a Person,

	display_sisters(Person) :-
	    person_sister(Person, Sister),
	    writeln(Sister),
	    fail ; true.

    will do the job.  In particular, if a Person has no
    sisters, it will quietly write nothing.

    However, in that case, your

	display_system(Person, _)

    will FAIL.

    The reason for this is that setof(Template, Goal, Set)
    will backtrack over bindings for variables in Goal
    that do not occur in Template.  (If it did not do this
    nested setof calls would not work.)  When there is no
    binding for such variables, the query fails.  This is
    not a bug or an accident but an *essential* feature
    of the setof/3 interface if it is to work nested.

    When all of the variables in Goal *happen* to be
    included in Template, that is NOT treated as a special
    case.

    So
	setof(X-Y, (sister_child(X,Y), X @< Y), Sister),
	write(Sister)
    is very nearly equivalent to
	sister_child(X, Y), X @< Y, Sister = X-Y,
	write(Sister).

    Start by asking:
	is display_sister(X, Y) supposed to be called
	with X and Y both known?
	what is it supposed to do then?
	in particular, what if sister(X,Y) is false,
	or sister(X,Y) is true but X @>= Y?

	is display_system(X, Y) supposed to be called
	with X known and Y still a variable?
	what is it supposed to do then?
	why is it considered a good idea that Y should
	still be an unbound variable afterwards?
	what is supposed to happen if there is no
	value for Y making sister(X, Y) true?

	the same, but with Y known and X a variable.

	is display_system(X, Y) supposed to be called
	with X and Y still both variables?
	what is it supposed to do then?
	why is it considered a good idea that X and Y
	should still be unbound variables afterwards?
	what is supposed to happen if there are no sisters?

(3) We come back to your continued failure to write any
    COMMENTS.  How are we supposed to know your intentions
    when you don't TELL us what your intentions are?
    We can't infer them from the code because the code
    looks *wrong*, as in "I can't think of any plausible
    use for code that looks like that".  Certainly such
    a change in behaviour (from *succeed* when there are
    no such relatives to *fail* when there are no such
    relatives) should most definitely have been signalled
    by a comment if that was what you meant.

    The very first lesson in becoming a good programmer in
    any language is "before you write any procedure,
    WRITE A COMMENT stating your intentions".  It does not
    have to be a big comment, or follow the heavyweight
    markup conventions of some stupid tool like JavaDoc.
    It has to give a human being a clue or two about what
    is supposed to happen.  And the first person to be
    helped is *YOU*.

    If you are following rigorous naming conventions, as
    recommended by Bertrand Myer in OOSC2, for example,
    you can omit a comment whose content is predictable
    from the procedure's name.  You can even do this if
    you are following a project-local or even file-local
    convention as long as that convention is itself
    DOCUMENTED.

    Let me give a compelling example of the need for comments
    from your own code:

> siblings(Child, Sibling) :-
>   same_father(Child, Sibling),
>   Child \= Sibling.
> siblings(Child, Sibling) :-
>   same_mother(Child, Sibling),
>   Child \= Sibling.

    If Child and Sibling are full siblings (same father and
    same mother but not same father) then
    siblings(Child, Sibling)
    will succeed twice, since there are two different ways
    to prove it.  (SQL agrees with Prolog here.)

    MAYBE THAT'S WHAT YOU WANT, though it seems extremely
    unlikely.  We cannot tell whether I've just noticed a
    bug in your code or whether you just have strange
    intentions, because *you* knew what you wanted this
    to do but decided to hide your intentions from us.

    Now, since I posted a definition

	full_or_half_siblings(Child, Sibling) :-
	    same_father(Child, Sibling),
	    Child \== Sibling.
	full_or_half_siblings(Child, Sibling) :-
	    same_mother(Child, Sibling),
	    \+ same_father(Child, Sibling).

   this siblings/2 looks like a garbled copy of that
   full_or_half_siblings/2.  So I am pretty sure it
   is a "sloppy copying" bug.  But I can't be CERTAIN
   because YOU DID NOT SAY WHAT YOU MEANT TO HAPPEN.

   Note that using full_or_half_siblings/2, which takes
   simple care NOT to repeat solutions, means that there
   isn't any point in using setof/3 in the other predicates.

   There are a number of great slogans about programming.
   "If it isn't tested it doesn't work" is one.
   Here's another: "If it has no useful comments it's wrong."

(4) I thought the following code had already been posted
    in this thread.

	:- meta_predicate display_answers(1).

	display_answers(Property) :-
	    call(Predicate, Answer),
	    writeln(Answer),
	    fail ; true.

(5) By the way, I don't really like output where many
    different commands have output that can't be
    distinguished.  I would much rather see

	display_sisters(Person) :-
	    write('% sisters of '), write(Person), nl,
	    (   sister(Person, Sister),
	        portray_clause(sister(Person, Sister)),
	        fail
            ;   true
            ),
	    write('% end'), nl.

    portray_clause/1 is a way of writing a something so
    that it looks like a fact that you could paste into
    Prolog source code.