patternswiftMajor
Stack implementation using Swift
Viewed 0 times
swiftimplementationusingstack
Problem
I would like to request some code review and feedback on a stack implementation using Swift.
Disclaimer: I might have not used protocols, optional unwrapping and error and
```
import Foundation
/ Node conformance /
protocol NodeProtocol {
typealias IDType;
typealias IDNode;
func insertNode(element : IDType, nextNode : IDNode?);
}
/ Stack's conformance prototol /
protocol StackProtocol {
typealias IDType;
/ Function to push element on stack /
func push(element : IDType);
}
class IDNode : NodeProtocol
{
var element: IDType?;
var nextNode: IDNode?;
/ Sets the element and updates next pointer /
func insertNode(element: IDType, nextNode: IDNode?) {
self.element = element;
self.nextNode = nextNode;
}
}
class IDStack : StackProtocol
{
private var stackHead: IDNode?;
init() {
/ Constructor to initialize the stack /
stackHead = IDNode();
}
func push(element: IDType) {
/* If stackHead is empty - just create a new stack
* Ideally the constructor should have taken care of this.
* but in our case - because of optional. stackHead can be NULL.
* So we don't want to take any chance while pushing element to stack.
*/
if(stackHead == nil) {
stackHead = IDNode();
}
/* if stack's first element is empty - Insert as the first element
* At this point stack is guaranteed not NULL. Right?
* What if memory allocation fails from above?
*/
if(stackHead!.element == nil) {
stackHead!.insertNode(element, nextNode: nil);
}else {
/ create a new node and insert the new element at top /
var nodeToPushInStack: IDNode! = IDNode();
/* I'm assuming memory allocation always passes from above.
*
Disclaimer: I might have not used protocols, optional unwrapping and error and
nil check correctly (still trying to figure out the essence of protocols - but that's the drill I'm going through now).```
import Foundation
/ Node conformance /
protocol NodeProtocol {
typealias IDType;
typealias IDNode;
func insertNode(element : IDType, nextNode : IDNode?);
}
/ Stack's conformance prototol /
protocol StackProtocol {
typealias IDType;
/ Function to push element on stack /
func push(element : IDType);
}
class IDNode : NodeProtocol
{
var element: IDType?;
var nextNode: IDNode?;
/ Sets the element and updates next pointer /
func insertNode(element: IDType, nextNode: IDNode?) {
self.element = element;
self.nextNode = nextNode;
}
}
class IDStack : StackProtocol
{
private var stackHead: IDNode?;
init() {
/ Constructor to initialize the stack /
stackHead = IDNode();
}
func push(element: IDType) {
/* If stackHead is empty - just create a new stack
* Ideally the constructor should have taken care of this.
* but in our case - because of optional. stackHead can be NULL.
* So we don't want to take any chance while pushing element to stack.
*/
if(stackHead == nil) {
stackHead = IDNode();
}
/* if stack's first element is empty - Insert as the first element
* At this point stack is guaranteed not NULL. Right?
* What if memory allocation fails from above?
*/
if(stackHead!.element == nil) {
stackHead!.insertNode(element, nextNode: nil);
}else {
/ create a new node and insert the new element at top /
var nodeToPushInStack: IDNode! = IDNode();
/* I'm assuming memory allocation always passes from above.
*
Solution
Let's start with some general remarks:
-
Swift does not require semicolons after statements (but they are allowed). From what I have seen since Swift was introduced last year,
most people do not write semicolons in Swift.
-
Classes are reference types, and the properties of an instance of a class
can be modified even if the instance is declares as a constant with
Therefore you can replace
e.g.
-
Often an explicit type annotation is not necessary because the compiler
can infer the type of an expression automatically. E.g. the above statement can be shortened to
Now the "major" points. To simplify things, I'll consider the
implementation without protocols first, so in the first part
of this answer I'll ignore your protocol definitions and assume that
the classes are defined as
First the
and optional, but actually node elements are cannot be
are never mutated once a node has been been pushed on the stack.
You probably did that because you create "empty" nodes first and then
call the
Another reason is that you use nodes "without an element", but I'll come
back to that later.
I would suggest to make
and replace the
Which leads us to the
a bit too complicated. The reason is that you use a single node
with
a linked list of nodes for two or more elements.
It becomes much simpler if you use a linked list with one node for each element in each case, and
or is
because the optional property
is automatically initialized to
simplifies to
Popping an element becomes simpler as well. Note that the preferred
way to check an optional (here:
binding:
Remark: Your comment
is misleading: Variables cannot be uninitialized in Swift. This is
ensured by the compiler and one of the major design goals in Swift.
Finally your
quite strange to me,
a computed property
of Swift's sequence types.
Modifying
and I can see no reason to do so. Use a local variable instead which
traverses through the linked list, and again use optional binding
to check if you have hit the end of the list:
Back to the protocols: Defining a protocol for "Stack" makes only sense
if it contains all required methods:
Then you can create an instance of
that expects a
Of course you can also define protocols for both the node and the
stack type. Now the node protocol should contain all required
properties and methods:
and a concrete implementation would be
(Please don't ask me why the
-
Swift does not require semicolons after statements (but they are allowed). From what I have seen since Swift was introduced last year,
most people do not write semicolons in Swift.
-
Classes are reference types, and the properties of an instance of a class
can be modified even if the instance is declares as a constant with
let.Therefore you can replace
var by let at many places in your code,e.g.
let nodeToPushInStack: IDNode! = IDNode()-
Often an explicit type annotation is not necessary because the compiler
can infer the type of an expression automatically. E.g. the above statement can be shortened to
let nodeToPushInStack = IDNode()Now the "major" points. To simplify things, I'll consider the
implementation without protocols first, so in the first part
of this answer I'll ignore your protocol definitions and assume that
the classes are defined as
class IDNode { ... }
class IDStack { ... }First the
IDNode class: You have defined element as a variableand optional, but actually node elements are cannot be
nil and are never mutated once a node has been been pushed on the stack.
You probably did that because you create "empty" nodes first and then
call the
insertNode() method to set the element and next pointer.Another reason is that you use nodes "without an element", but I'll come
back to that later.
I would suggest to make
element a constant and non-optionaland replace the
insertNode() method by an init method:class IDNode
{
let element: IDType
let nextNode: IDNode?
init(element : IDType, before: IDNode?) {
self.element = element
self.nextNode = before
}
}Which leads us to the
IDStack class: The push and pop methods area bit too complicated. The reason is that you use a single node
with
element == nil for an empty stack, a single node withelement != nil for a stack with a single element, and thena linked list of nodes for two or more elements.
It becomes much simpler if you use a linked list with one node for each element in each case, and
stackHead points to the front node,or is
nil for an empty stack. Then you don't need an init() methodbecause the optional property
private var stackHead: IDNode?is automatically initialized to
nil, and pushing an elementsimplifies to
func push(element: IDType) {
let newNode = IDNode(element: element, before: stackHead)
stackHead = newNode
}Popping an element becomes simpler as well. Note that the preferred
way to check an optional (here:
stackHead) for nil is optionalbinding:
func popElement() -> IDType? {
if let firstNode = stackHead {
let element = firstNode.element
stackHead = firstNode.nextNode
return element
} else {
return nil
}
}Remark: Your comment
/* Stack is empty / not initialized */is misleading: Variables cannot be uninitialized in Swift. This is
ensured by the compiler and one of the major design goals in Swift.
Finally your
getLength() method: Using Double as return type seemsquite strange to me,
Int would be appropriate. And I would usea computed property
count instead, similar to the count propertyof Swift's sequence types.
Modifying
self.stackHead inside the method temporarily is a bad ideaand I can see no reason to do so. Use a local variable instead which
traverses through the linked list, and again use optional binding
to check if you have hit the end of the list:
var count : Int {
var numItems = 0
var currentNode = self.stackHead
while let node = currentNode {
numItems++
currentNode = node.nextNode
}
return numItems;
}Back to the protocols: Defining a protocol for "Stack" makes only sense
if it contains all required methods:
protocol StackProtocol {
typealias IDType
func push(element : IDType)
func popElement() -> IDType?
var count : Int { get }
}Then you can create an instance of
IDStack and pass it to a functionthat expects a
StackProtocol. Simple example:func test(stack : S) {
stack.push("foo")
stack.push("bar")
stack.push("baz")
print(stack.count)
while let element = stack.popElement() {
print(element)
}
}
test(IDStack())Of course you can also define protocols for both the node and the
stack type. Now the node protocol should contain all required
properties and methods:
protocol NodeProtocol {
typealias IDType
var element : IDType { get }
var nextNode : Self? { get }
init(element : IDType, before: Self?)
}and a concrete implementation would be
final class IDNode : NodeProtocol
{
let element: IDType
let nextNode: IDNode?
init(element : IDType, before: IDNode?) {
self.element = element
self.nextNode = before
}
}(Please don't ask me why the
final is necessary hereCode Snippets
let nodeToPushInStack: IDNode<IDType>! = IDNode<IDType>()let nodeToPushInStack = IDNode<IDType>()class IDNode<IDType> { ... }
class IDStack<IDType> { ... }class IDNode<IDType>
{
let element: IDType
let nextNode: IDNode?
init(element : IDType, before: IDNode?) {
self.element = element
self.nextNode = before
}
}private var stackHead: IDNode<IDType>?Context
StackExchange Code Review Q#97600, answer score: 28
Revisions (0)
No revisions yet.