Dev:Code Structure

From AMSN
Jump to: navigation, search

Contents

Code cleaning

Structure:

First, we must define 4 "primary" namespaces, they will be as follow:

  •  ::amsn
    • It will contain all GUI related procs
  •  ::gui
    • It will contain all "public" procs that can be accessed externally for the ::amsn namespace
  •  ::MSN
    • It will contain all protocol related procs
  •  ::protocol
    • It will contain all "public" procs that can be accessed externally for the ::MSN namespace

Now here is an explanation of what I mean by "public". The ::amsn procs should NEVER be called from within ANY namespace (global namespace included) except from the ::gui namespace. This goes for the ::protocol namespace too.

This means that if you want to redraw the contact list, instead of calling ::amsn::cmsn_draw_online, or ::amsn::cmsn_draw_offline, you'll call ::gui::refreshContactList. and this goes for every procs. Also, if you want to send a message on an SB, instead of doing a ::MSN::WriteSB $sb $msg, you will do something like that ::protocol::SendMessage $chatid $msg

the ::protocol::SendMessage should get the sb name from the chatid

What do you think about that, should we abstract it at this level? don't use sbnames or window names, but the protocol layer will use the chatid (which, as alvaro said, was meant for this kind of abstraction) instead? I think this kind of abstraction is best.

Now, we should move all gui related procs to the ::amsn namespace and all protocol related procs to the ::MSN namespace, also create other kind of namespaces for everything that should be set alone, for example smileys :

  • create a ::smileys namespace__
  • move all gui related procs from the smileys file to the ::amsn namespace__ (example the window for adding a new custom smiley, and the smiley selector window)
  • Create new procs for putting a layer between the gui and smileys thing__ (when parsing smileys, we should do a search withing the text widget, but a search within a text string, and return what changes must occur, and then a proc from the ::amsn namespace would then change what needs to be changed)
  • move all protocol related procs to the msn namespace__ (this includes parsing of the custom smileys, etc...)
  • move everything else to the smileys namespace__, this would mean, parsing smileys from the xml config file, etc...

and this goes for all other things, I took the smileys as example since this is what I know best.

Now here are some basic rules that we must follow :

procs must be as __small__ as possible, this means, a maximum of 10 lines, the "only" exception would be for creating a window that has alot of widgets, but for that kind of thing, you also need to make it less than 10 lines if you can, I want to see the cmsn_draw_main as being a 10 line proc, all other stuff must be in other procs, something liek this :

 proc cmsn_draw_main { } {
 set w .
 drawMaimMenu $w
 drawMainWidgets $w
 createMainBinding $w
 }

Alvaro says that 10 lines is maybe two few lines, but I think that it's just perfect, for sure, you can do more than 10 lines, but just keeping in mind the "concept" of 10 lines will make the code more modular. It's better to split a big one into many procedures with clear names, than having a messy and long procedure code.

and each other proc would call out some other procs and make be less than 10 lines... this is meant to be __as MODULAR as possible__, this is the key word

Another thing, I don't want procs that call out a hundred of other procs and you get lost in it, I mean, even if you do :

 proc cmsn_draw_main { } {
 ...
 createMainBindings $w
 }
 proc createMainBindings { w } {
 ...
 createStatusBarBindings $w
 }
 proc createStatusBarBindings { w } {
 ...
 addSpecialBindings $w
 }

ok, here is a big stack trace (a little), that doesn't matter, since it is justified, but what I don't want is having something like the authentication process, something where you can get lost in it, it must always be easy to read, __modular AND linear__, which means, you don't need to go back to the first proc to see when it goes again.. anyways, just try to understand how the authentication process is done and you'll see what I mean

One important thing, when moving code, you must first understand it, so you know what you're doing, don't just say "ohh, this goes in here", you must understand it, almost always you'll need to change it (making it more modular), and try to read the code a find a better way to do things, so the code will be optimized... remove almost any unecessary canvas. many if-elsif statements change to switch statements, etc...

If you don't fully understand what a procedure does, ask on the list. Other developers can help you.

and the important thing I was talking about, since you read the procs, you need to __ADD COMMENTS__ to every procs, and to every group of lines that basically do the same thing (which should better be grouped in a separate proc, but whatever...)

Comments

Proc comments

Leave 2 blanks before the comment, not more, not less, and one blank after the comment

ok, first line will have to be a full line of ### second line should be the proc prototype, initialised values hould be identified as so with the corresponding initial value third line is empty fourth and so on should explain what the proc does, this includes this info : what are the arguments and what are their meaning what are the variables used inside the proc and why do we use them what are the possible return values (check if proc always/never returns something and see if it can cause a bug) What is the purpose of the proc How it does it's job The last line should be a single # char

ok, here's an example of the great job burger did and I'm very thankfull to him when I had to work with MSNP2P too :)

 return "${theader}${b}"
 }


 ##################################################################
 # MakeMSNSLP ( method to from branchuid cseq maxfwds contenttentype [A] [B] [C] [D] [E] [F] [G]) 
 # This function creates the appropriate MSNSLP packets
 # method :		INVITE, BYE, OK, DECLINE
 # contenttype : 	0 for application/x-msnmsgr-sessionreqbody (Starting a session)
 #			1 for application/x-msnmsgr-transreqbody (Starting transfer)
 #			1 for application/x-msnmsgr-transreqbody (Starting transfer)
 #
 # If INVITE method is chosen then A, B, C, D and/or E are used dependinf on contenttype
 # for 0 we got : "EUF-GUID", "SessionID", "AppID" and "Context" (E not used)
 # for 1 we got : "Bridges", "NetID", "Conn-Type", "UPnPNat" and "ICF"
 # for 2 we got : "Bridge", "Listening", "Nonce", "IPv4External-Addrs","IPv4External-Port"
 #		 "IPv4Internal-Addrs" and "IPv4Internal-Port"
 #
 # If OK method is chosen then A to G are used depending on contenttype
 # for 0 we got : "SessionID"
 # for 1 we got : "Bridge", "Listening", "Nonce", "IPv4External-Addrs","IPv4External-Port"
 #		 "IPv4Internal-Addrs" and "IPv4Internal-Port"
 # Returns the formated MSNSLP data
 proc MakeMSNSLP { method to from branchuid cseq uid maxfwds contenttype {A ""} {B ""} {C ""} {D ""} {E ""} {F ""} {G ""} } {
 ...


Here's how I would change he's comment to make it follow the rules I've laid, although his comments are perfect :


 return "${theader}${b}"
 }


 #################################################################
 # MakeMSNSLP ( method to from branchuid cseq maxfwds contenttentype [A] [B] [C] [D] [E] [F] [G]) 
 #
 # This function creates the appropriate MSNSLP packets
 # method :		This is the MSNSLP method to create, one of : INVITE, BYE, OK, DECLINE
 # to, from, branchuid, cseq, maxfwds  :     All these arguments are respectively the values of the "To",\
 #                                                            "From", "branch", "Cseq" and "Max-Forwards" in the MSNSLP message
 # 
 # contenttype : 	This is to specify what type of data the MSNSLP packet is supposed to handle, \
 #                         the values are as follow :
 #                         0 for application/x-msnmsgr-sessionreqbody (Starting a session)
 #			1 for application/x-msnmsgr-transreqbody (Requesting transfer)
 #			2 for application/x-msnmsgr-transreqbody (Starting transfer)
 #
 # The A, B, C, D, E, F and G arguments are initialized as an empty string
 #
 # If INVITE method is chosen then A, B, C, D and/or E are used depending on contenttype
 # for 0 we got : "EUF-GUID", "SessionID", "AppID" and "Context" (E not used)
 # for 1 we got : "Bridges", "NetID", "Conn-Type", "UPnPNat" and "ICF"
 # for 2 we got : "Bridge", "Listening", "Nonce", "IPv4External-Addrs","IPv4External-Port"
 #		 "IPv4Internal-Addrs" and "IPv4Internal-Port"
 #
 # If OK method is chosen then A to G are used depending on contenttype
 # for 0 we got : "SessionID"
 # for 1 we got : "Bridge", "Listening", "Nonce", "IPv4External-Addrs","IPv4External-Port"
 #		 "IPv4Internal-Addrs" and "IPv4Internal-Port"
 #
 # Returns the formated MSNSLP data
 #
 
 proc MakeMSNSLP { method to from branchuid cseq uid maxfwds contenttype \
                               {A ""} {B ""} {C ""} {D ""} {E ""} {F ""} {G ""} } {
 ...

ok, this was a big proc:P but it should be made smaller, something like calling those procs :

 # Check what method is used
 switch -- $method { 
 INVITE { 
 # If INVITE method is used, return the concatenation of the header and the body 
 return "[createInviteHeader $to $from $branchuid $cseq $uid $maxfwds][createInviteBody $contentype $A $B ...]" 
 }
 OK {
 return "[createOkHeader ..][createOkBody ...]" 
 }
 ...
 }

also add verifications to see if the A,B,C... args contain necessery data. and use a switch in the createInviteBody proc which would call other procs depending on contenttype

ok, so this is a huge example, but the idea should be clear, I hope so, if anyone has a better example or wants to make changes, you're welcome, this is what the wiki is all about :P but as you may have understood by now, this is kind of a "perfect" "comment writing process" to use, but you know you're not obliged to follow these rules exactly, but we should try at least to make the proc as easy to understand as possible throught the comments, even for a guy who doesn't know about tcl:p so you'll have to judge.

commands comments

Ok, this is basically simple, just add comments on every group of commands so that we know what does what, but anyways as I said before, every group of commands should be in a separate proc, and with the proc name + proc comments, we should know what it does, but still add some little text if necessary.

Personal tools