Re: [Admin] Code-Review

Hi David, as there was no SmallWiki meeting today I am giving the code-review I prepared using e-mail else it gets out-dated. If you have any questions, please ask. - Get rid of #isKindOf: - Don't emit html-tags manually using "html text: '<b>Role:</b>'" as this does not allow to render to another output format. Furthermore the b-tag is depreciated and won't validate for HTML4 or XHTML. Either use "html strong: 'Role:'" or much better "html spanClass: 'label' with: 'Role:'" instead. - Don't emit html-characters manually using "html text: '&nsbsp;'" as this does not allow to render to another output format. Use "html space" instead. - It is much simpler to use "html cssClass: 'label'" instead of "html attributeAt: 'class' put: 'label'". - Please don't use JavaScript, as it does not work on my machine even-tough I have enabled it. It is easily possible to replace it with server-side logic. - Don't return associations of strings in AdminAction class>>defaultMenu with tile and the url-postfix, but the actions classes. All actions in SmallWiki are able to return their title and know much better how to build their action-url than your code does. - The class name of AdminAttachments doesn't seem to match what the action is actually doing. - AdminAttachments class>>generate_id: should be renamed to AdminAttachments class>>generateId: as the underscore is not portable to other Smalltalk dialects. - Furthermore AdminAttachments class>>generate_id: should be moved to Structure as it has nothing to do with the action-class and basically works on a structure. - Last but not least, you shouldn't need to use AdminAttachments class>>generate_id: at all. As far as I understand you are building and parsing this id to identify structures within a wiki. If you use hidden-fields it is possible to pass objects from one action to the other and you do not have to deal with strings, this is much simpler and cleaner. - I haven't implemented the read-accessor of User>>password intentionally, because it is planned to store the password as a hash-value instead of plain-text later on. This is a security issue to have the password accessible like that and to render it when editing users. It is the responsibility of the user-class to validate the user-password using the message #validatePassword: and it should not be accessible form outside. - The following sections are some things that are basically reported by SmallLint or things that I think could have been written simpler. I've not tested the code I am suggesting, but it should work more or less: -=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=- Structure>>rolesIncludes: aRole "check if a role is contained in a roles collection" roles notNil ifTrue: [^roles includes: aRole]. ^false ==> Structure>>rolesIncludes: aRole ^roles notNil and: [ roles includes: aRole ] -=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=- Structure>>roleWithName: aRoleName "returns a role with certain name" self rolesDo: [:role | role name = aRoleName ifTrue: [^role]]. ^nil ==> Structure>>roleWithName: aString self roles detect: [ :each | each name = aString ] ifNone: [ nil ] -=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=- Structure>>selfAndAllChildren "return all children and their children.... as collection - at this point: just return self in a collection " | collection | (collection := OrderedCollection new) add: self. ^collection ==> Structure>>withChildren ^OrderedCollection with: self -=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=- X>>foo X bar ==> X>>foo self class bar -=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=- AdminAction class>>defaultSeparator "return the default string separator - this seperator is used to generate the input fields of the form" ^String new writeStream tab contents ==> AdminAction class>>defaultSeparator ^String with: Character tab -=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=- AdminAction>>cutStructure: aStructureToDelete ofSelfAndChildrenOf: aStructure "cut the indicated structure out of the tree, if the paste destination was not in its subtree" | errorStream myParents | errorStream := String new writeStream. myParents := aStructure parents copy. (myParents includes: aStructureToDelete) ifFalse: [self deleteStructure: aStructureToDelete OfSelfAndChildrenOf: self server root] ifTrue: [errorStream nextPutAll: '--> Paste of '''; nextPutAll: aStructureToDelete title; nextPutAll: ''' was ok, but can not cut: destination is sub-structure of the element to be pasted!'. self error: errorStream contents] ==> AdminAction>>cutStructure: aStructureToDelete ofSelfAndChildrenOf: aStructure ... self error: (String streamContents: [ :stream | stream nextPutAll: ' ... ]) -=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=- CharacterArray>>asArrayOfSubstrings "split the string at its seperators and return array with substrings" | first last collection | collection := OrderedCollection new. last := 0. [first := self findFirst: [:char | char isSeparator not] startingAt: last + 1. first ~= 0] whileTrue: [last := (self findFirst: [:char | char isSeparator] startingAt: first) - 1. last < 0 ifTrue: [last := self size]. collection add: (self copyFrom: first to: last)]. ^collection asArray ==> CharacterArray>>asArrayOfSubstrings ^self runsSatisfying: [ :each | each isSeparator ] -=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=- Cheers, Lukas -- Lukas Renggli http://renggli.freezope.org