New subject: Another one

10 Nov 2003

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

[Admin] Code-Review