|
Separation of ConcernsSeparate dispatch and handlingExample: the handling of resize bars in the splitwin module needs to do something on mouseDown, mouseMove, and mouseUp these predicates to handle these things should be grouped together and should then be called from the splitwin event handler. Split functionality earlyDon’t make a predicate that is used for two different things where you dispatch on some argument value or worse a database fact in the leaf of call tree. In your code you use the same predicate to draw 2D and 3D resize bars. Looking at the code very little is reused and it makes the code more complicated to build it that way instead make two predicates and check at the top level whether you should call split_draw2Dbars or split_draw3Dbars. Copy-PasteDon’t use copy paste! If you have the same code in several cases carefully consider whether it would make sense to put it in a common predicate.
Predicate namesThere is no one rule as to how you pick good names for your predicates. So I’m only going to come with a few suggestions here. Use "Is" if you have a predicate that test for something. That way you signal what holds if the predicate succeeds so don’t name the predicate "splitwin_CheckLegalBarPos" but "splitwin_ IsLegalBarPos". The predicate name should reflect the meaning of the predicate not the way it is used. So don’t name the predicates "splitwin_MouseInBar", but "splitwin_pnt_IsInBar". Sometimes it is very difficult to come up with good names Use direct recursion instead of accumulating argumentsThat is, unless there is a purpose with the accumulating argument like in reverse. You write: splitwin_DeleteFlags([],OutFlags,OutFlags) :- !. splitwin_DeleteFlags([F|Flags],OutFlags,FL) :- F = wsf_ClipChildren, !, splitwin_DeleteFlags(Flags,OutFlags,FL). splitwin_DeleteFlags([F|Flags],OutFlags,FL) :- F = wsf_Visible,!, splitwin_DeleteFlags(Flags,OutFlags,FL). splitwin_DeleteFlags([F|Flags],OutFlags,FL) :- !, splitwin_DeleteFlags(Flags,[F|OutFlags],FL). splitwin_DeleteFlags(_,_,[]) :- errorexit(). There is no good reason for using an accumulating argument here it only complicates matters and reverse the list of flags. Furthermore testing for each flag you want to delete in separate clauses is very bad programming style instead you should use member or make a separate predicates that checks whether this is one of the flags you want to delete. I would write splitwin_DeleteFlags([],[]). splitwin_DeleteFlags([F|Fs],DFs) :- member(F,[wsf_ClipChildren,wsf_Visible]), !, splitwin_DeleteFlags(Fs,DFs). splitwin_DeleteFlags([F|Fs],[F|DFs]) :- splitwin_DeleteFlags(Fs,DFs). splitwin_DeleteFlags(_,_):- errorExit(splitwin_internal_error). % not needed in new version. Or maybe make a predicate called list_diff which took the difference between two lists in general and call it list_diff(Flags,[wsf_ClipChildren,wsf_Visible],NewFlags). list_diff([],_,[]):- !. list_diff([X|Xs],Ys,Zs) :- member(X,Ys), !, list_diff(Xs,Ys,Zs). list_diff([X|Xs],Ys,[X|Zs]) :- list_diff(Xs,Ys,Zs). The last solution is preferable because list_diff has a clear meaning independent of its usage. Keep things logically togetherIn you code you handle the resize bars on mouse_Down, mouse_Move, and mouse_Up. You have sensibly enough made a separate predicate for mouse_Move, but you haven’t made separate predicates for the resize bar handling on mouse_Down and mouse_Úp. You should make handling predicates for all three events, and call them in the event handler. |