Skip to main content
use actual paragraphs, subsection headings, and bullet lists
Source Link

Strategy

Strategy
To To support (coding and) code review (especially when asking for alternatives), state (in the code) the goal pursued during coding. (Double linkage/last serves no purpose I can see.)
If

If not literally re-inventing a support on solid surfaces combining low longitudinal friction with good lateral guide, use/implement an existing "protocol"/"interface" - mutable sequence comes to mind seeing the methods you present; something like test_MutableSequence() may fall into your lap.
Explicitly

Explicitly specify everything protocol but not standard using docstrings. Sketch tests: If you don't know what to test, you don't know what to implement.

Tactics
- specify what is to happen with parameter values without "natural" meaning, e.g., index smaller zero or not smaller count.
- (as @Peilonrayz demanded:) Don’t Repeat Yourself
- provide docstrings for classes (and modules), too
- check comments and, arguably more important, docstrings for correctness - when a second error passes unit testing after thinking unit tests complete, revise unit tests

Tactics

  • specify what is to happen with parameter values without "natural" meaning, e.g., index smaller zero or not smaller count.
  • (as @Peilonrayz demanded:) Don’t Repeat Yourself
  • provide docstrings for classes (and modules), too
  • check comments and, arguably more important, docstrings for correctness
  • when a second error passes unit testing after thinking unit tests complete, revise unit tests

Observations about the code, [especially] insert, find, replace, at_index:
- insert fails to update count
- find, replace: replace (& _find) might use find. With quality satisfied with more than one node's data, both are underspecified.
- _at_index: if count/2 < index < count, walk backwards
(it may be useful to allow indices from -count (even with at_index or generally) - cf. slicing)
- delete looks non-adapted from a singly-linked list implementation (no need for last)
current.next.last should be set depending on current == tail
should use find()
- insert fails to set old_head.last
- reverse: how about transmuting to an instance of ListLinkedDoubly, with roles of next and last exchanged(&head/tail, if sticking with no sentinel node (@Peilonrayz, again))
As presented, reverse() is a costly operation - reversed() returns an iterator
- __str__: return '[]' if 0 == self.count \
    else '[-(' + ')<=>('.join(self.items()) + ')-]'

  • insert fails to update count
  • find, replace: replace (& _find) might use find. With quality satisfied with more than one node's data, both are underspecified.
  • _at_index: if count/2 < index < count, walk backwards
    (it may be useful to allow indices from -count (even with at_index or generally) - cf. slicing)
  • delete looks non-adapted from a singly-linked list implementation (no need for last)
    current.next.last should be set depending on current == tail
    should use find()
  • insert fails to set old_head.last
  • reverse: how about transmuting to an instance of ListLinkedDoubly, with roles of next and last exchanged(&head/tail, if sticking with no sentinel node (@Peilonrayz, again))
    As presented, reverse() is a costly operation - reversed() returns an iterator
  • __str__: return '[]' if 0 == self.count \
        else '[-(' + ')<=>('.join(self.items()) + ')-]'

Strategy
To support (coding and) code review (especially when asking for alternatives), state (in the code) the goal pursued during coding. (Double linkage/last serves no purpose I can see.)
If not literally re-inventing a support on solid surfaces combining low longitudinal friction with good lateral guide, use/implement an existing "protocol"/"interface" - mutable sequence comes to mind seeing the methods you present; something like test_MutableSequence() may fall into your lap.
Explicitly specify everything protocol but not standard using docstrings. Sketch tests: If you don't know what to test, you don't know what to implement.

Tactics
- specify what is to happen with parameter values without "natural" meaning, e.g., index smaller zero or not smaller count.
- (as @Peilonrayz demanded:) Don’t Repeat Yourself
- provide docstrings for classes (and modules), too
- check comments and, arguably more important, docstrings for correctness - when a second error passes unit testing after thinking unit tests complete, revise unit tests

Observations about the code, [especially] insert, find, replace, at_index:
- insert fails to update count
- find, replace: replace (& _find) might use find. With quality satisfied with more than one node's data, both are underspecified.
- _at_index: if count/2 < index < count, walk backwards
(it may be useful to allow indices from -count (even with at_index or generally) - cf. slicing)
- delete looks non-adapted from a singly-linked list implementation (no need for last)
current.next.last should be set depending on current == tail
should use find()
- insert fails to set old_head.last
- reverse: how about transmuting to an instance of ListLinkedDoubly, with roles of next and last exchanged(&head/tail, if sticking with no sentinel node (@Peilonrayz, again))
As presented, reverse() is a costly operation - reversed() returns an iterator
- __str__: return '[]' if 0 == self.count \
    else '[-(' + ')<=>('.join(self.items()) + ')-]'

Strategy

To support (coding and) code review (especially when asking for alternatives), state (in the code) the goal pursued during coding. (Double linkage/last serves no purpose I can see.)

If not literally re-inventing a support on solid surfaces combining low longitudinal friction with good lateral guide, use/implement an existing "protocol"/"interface" - mutable sequence comes to mind seeing the methods you present; something like test_MutableSequence() may fall into your lap.

Explicitly specify everything protocol but not standard using docstrings. Sketch tests: If you don't know what to test, you don't know what to implement.

Tactics

  • specify what is to happen with parameter values without "natural" meaning, e.g., index smaller zero or not smaller count.
  • (as @Peilonrayz demanded:) Don’t Repeat Yourself
  • provide docstrings for classes (and modules), too
  • check comments and, arguably more important, docstrings for correctness
  • when a second error passes unit testing after thinking unit tests complete, revise unit tests

Observations about the code, [especially] insert, find, replace, at_index:

  • insert fails to update count
  • find, replace: replace (& _find) might use find. With quality satisfied with more than one node's data, both are underspecified.
  • _at_index: if count/2 < index < count, walk backwards
    (it may be useful to allow indices from -count (even with at_index or generally) - cf. slicing)
  • delete looks non-adapted from a singly-linked list implementation (no need for last)
    current.next.last should be set depending on current == tail
    should use find()
  • insert fails to set old_head.last
  • reverse: how about transmuting to an instance of ListLinkedDoubly, with roles of next and last exchanged(&head/tail, if sticking with no sentinel node (@Peilonrayz, again))
    As presented, reverse() is a costly operation - reversed() returns an iterator
  • __str__: return '[]' if 0 == self.count \
        else '[-(' + ')<=>('.join(self.items()) + ')-]'
factoring out add() didn't look worth the bother
Source Link
greybeard
  • 7.7k
  • 3
  • 21
  • 56

Tactics:
- specify what is to happen with parameter values without "natural" meaning, e.g., index smaller zero or not smaller count.
- (as @Peilonrayz demanded:) Don’t Repeat Yourself
- provide docstrings for classes (and modules), too
- check comments and, arguably more important, docstrings for correctness - when a second error passes unit testing after thinking unit tests complete, revise unit tests

Observations about the code, [especially] insert, find, replace, at_index:
- insert fails to update count
- find, replace: replace (& _find) might use find. With quality satisfied with more than one node's data, both are underspecified.
- _at_index: if count/2 < index < count, walk backwards
(it may be useful to allow indices from -count (even with at_index or generally) - cf. slicing)
- delete looks non-adapted from a singly-linked list implementation (no need for last)
current.next.last should be set depending on current == tail
should use find()
- insert fails to set old_head.last
- reverse: how about transmuting to an instance of ListLinkedDoubly, with roles of next and last exchanged(&head/tail, if sticking with no sentinel node (@Peilonrayz, again))
As presented, reverse() is a costly operation - reversed() returns an iterator
- __str__: return '[]' if 0 == self.count \
    else '[-(' + ')<=>('.join(self.items()) + ')-]'

Trying to stay DRY implementing pre/append():

def _add(self, item):
    """Update count and return new Node,
    attached to list if empty before.
    """
    self.count += 1
    new_node = Node(item)

    if self.head is None:
        self.head = self.tail = new_node
    return new_node

def append(self, item):
    """Return self with item inserted at the tail."""
    new_node = self._add(item)

    if 1 < self.count:
        self.tail.next = new_node
        new_node.last = self.tail
        self.tail = new_node

def prepend(self, item):
    """Return self with item inserted at the head."""
    new_node = self._add(item)

    if 1 < self.count:
        self.head.last = new_node
        new_node.next = self.head
        self.head = new_node

(I "real life", I like to be able to chain calls: return self)

Tactics:
- specify what is to happen with parameter values without "natural" meaning, e.g., index smaller zero or not smaller count.
- (as @Peilonrayz demanded:) Don’t Repeat Yourself
- provide docstrings for classes (and modules), too
- check comments and, arguably more important, docstrings for correctness - when a second error passes unit testing after thinking unit tests complete, revise unit tests

Observations about the code, [especially] insert, find, replace, at_index:
- insert fails to update count
- find, replace: replace (& _find) might use find. With quality satisfied with more than one node's data, both are underspecified.
- _at_index: if count/2 < index < count, walk backwards
(it may be useful to allow indices from -count (even with at_index or generally) - cf. slicing)
- delete looks non-adapted from a singly-linked list implementation (no need for last)
current.next.last should be set depending on current == tail
should use find()
- insert fails to set old_head.last
- reverse: how about transmuting to an instance of ListLinkedDoubly, with roles of next and last exchanged(&head/tail, if sticking with no sentinel node (@Peilonrayz, again))
As presented, reverse() is a costly operation - reversed() returns an iterator
- __str__: return '[]' if 0 == self.count \
    else '[-(' + ')<=>('.join(self.items()) + ')-]'

Trying to stay DRY implementing pre/append():

def _add(self, item):
    """Update count and return new Node,
    attached to list if empty before.
    """
    self.count += 1
    new_node = Node(item)

    if self.head is None:
        self.head = self.tail = new_node
    return new_node

def append(self, item):
    """Return self with item inserted at the tail."""
    new_node = self._add(item)

    if 1 < self.count:
        self.tail.next = new_node
        new_node.last = self.tail
        self.tail = new_node

def prepend(self, item):
    """Return self with item inserted at the head."""
    new_node = self._add(item)

    if 1 < self.count:
        self.head.last = new_node
        new_node.next = self.head
        self.head = new_node

(I "real life", I like to be able to chain calls: return self)

Tactics
- specify what is to happen with parameter values without "natural" meaning, e.g., index smaller zero or not smaller count.
- (as @Peilonrayz demanded:) Don’t Repeat Yourself
- provide docstrings for classes (and modules), too
- check comments and, arguably more important, docstrings for correctness - when a second error passes unit testing after thinking unit tests complete, revise unit tests

Observations about the code, [especially] insert, find, replace, at_index:
- insert fails to update count
- find, replace: replace (& _find) might use find. With quality satisfied with more than one node's data, both are underspecified.
- _at_index: if count/2 < index < count, walk backwards
(it may be useful to allow indices from -count (even with at_index or generally) - cf. slicing)
- delete looks non-adapted from a singly-linked list implementation (no need for last)
current.next.last should be set depending on current == tail
should use find()
- insert fails to set old_head.last
- reverse: how about transmuting to an instance of ListLinkedDoubly, with roles of next and last exchanged(&head/tail, if sticking with no sentinel node (@Peilonrayz, again))
As presented, reverse() is a costly operation - reversed() returns an iterator
- __str__: return '[]' if 0 == self.count \
    else '[-(' + ')<=>('.join(self.items()) + ')-]'

from list of findings towards review
Source Link
greybeard
  • 7.7k
  • 3
  • 21
  • 56

Strategy
To support (coding and) code review (especially when asking for alternatives), state (in the code) the goal pursued during coding. (Double linkage/last serves no purpose I can see.)
If not literally re-inventing a support on solid surfaces combining low longitudinal friction with good lateral guide, use/implement an existing "protocol"/"interface" - mutable sequence comes to mind seeing the methods you present; something like test_MutableSequence() may fall into your lap.
Explicitly specify everything protocol but not standard using docstrings. Sketch tests: If you don't know what to test, you don't know what to implement.

Tactics:
- specify what is to happen with parameter values without "natural" meaning, e.g., index smaller zero or not smaller count.
- (as @Peilonrayz demanded:) Don’t Repeat Yourself
- provide docstrings for classes (and modules), too
- check comments and, arguably more important, docstrings for correctness - when a second error passes unit testing after thinking unit tests complete, revise unit tests

  • provide docstrings for classes (and modules), too

Observations about the code, [especially] insert, find, replace, at_index:
- insert fails to update count
- find, replace: replace (& _find) might use find. With quality satisfied with more than one node's data, both are underspecified.
- _at_index: if count/2 < index < count, walk backwards
(it may be useful to allow indices from -count (even with at_index or generally) - cf. slicing)
- delete looks non-adapted from a singly-linked list implementation (no need for last)
current.next.last should be set depending on current == tail
should use find()
- insert fails to set old_head.last
- reverse: how about transmuting to an instance of ListLinkedDoubly, with roles of next and last exchanged(&head/tail, if sticking with no sentinel node (@Peilonrayz, again))
As presented, reverse() is a costly operation - reversed() returns an iterator
- __str__: return '[]' if 0 == self.count \
    else '[-(' + ')<=>('.join(self.items()) + ')-]'

  • insert fails to update count
  • find, replace: replace might use _find. With quality satisfied with more than one node's data, both are underspecified.
  • _at_index: if count/2 < index < count, walk backwards
    (it may be useful to allow indices from -count (even with at_index or generally) - cf. slicing)
  • delete looks non-adapted from a singly-linked list implementation
    should use _at_index
  • insert fails to set old_head.last
  • reverse: how about transmuting to an instance of ListLinkedDoubly, with roles of next and last(&head/tail) exchanged
  • __str__: return '[]' if 0 == self.count \
        else '[-(' + ')<=>('.join(self.items()) + ')-]'

double linkage/Trying to stay DRY implementing lastpre/append() serves no purpose:

def _add(self, item):
    """Update count and return new Node,
    attached to list if empty before.
    """
    self.count += 1
    new_node = Node(item)

    if self.head is None:
        self.head = self.tail = new_node
    return new_node

def append(self, item):
    """Return self with item inserted at the tail."""
    new_node = self._add(item)

    if 1 < self.count:
        self.tail.next = new_node
        new_node.last = self.tail
        self.tail = new_node

def prepend(self, item):
    """Return self with item inserted at the head."""
    new_node = self._add(item)

    if 1 < self.count:
        self.head.last = new_node
        new_node.next = self.head
        self.head = new_node

(I "real life", I can see
can this profit fromlike to be able to chain calls: collections.abc.MutableSequence?return self)

specify what is to happen with parameter values without "natural" meaning, e.g., index smaller zero or not smaller count.

  • provide docstrings for classes (and modules), too

[especially] insert, find, replace, at_index

  • insert fails to update count
  • find, replace: replace might use _find. With quality satisfied with more than one node's data, both are underspecified.
  • _at_index: if count/2 < index < count, walk backwards
    (it may be useful to allow indices from -count (even with at_index or generally) - cf. slicing)
  • delete looks non-adapted from a singly-linked list implementation
    should use _at_index
  • insert fails to set old_head.last
  • reverse: how about transmuting to an instance of ListLinkedDoubly, with roles of next and last(&head/tail) exchanged
  • __str__: return '[]' if 0 == self.count \
        else '[-(' + ')<=>('.join(self.items()) + ')-]'

double linkage/last serves no purpose I can see
can this profit from collections.abc.MutableSequence?

Strategy
To support (coding and) code review (especially when asking for alternatives), state (in the code) the goal pursued during coding. (Double linkage/last serves no purpose I can see.)
If not literally re-inventing a support on solid surfaces combining low longitudinal friction with good lateral guide, use/implement an existing "protocol"/"interface" - mutable sequence comes to mind seeing the methods you present; something like test_MutableSequence() may fall into your lap.
Explicitly specify everything protocol but not standard using docstrings. Sketch tests: If you don't know what to test, you don't know what to implement.

Tactics:
- specify what is to happen with parameter values without "natural" meaning, e.g., index smaller zero or not smaller count.
- (as @Peilonrayz demanded:) Don’t Repeat Yourself
- provide docstrings for classes (and modules), too
- check comments and, arguably more important, docstrings for correctness - when a second error passes unit testing after thinking unit tests complete, revise unit tests

Observations about the code, [especially] insert, find, replace, at_index:
- insert fails to update count
- find, replace: replace (& _find) might use find. With quality satisfied with more than one node's data, both are underspecified.
- _at_index: if count/2 < index < count, walk backwards
(it may be useful to allow indices from -count (even with at_index or generally) - cf. slicing)
- delete looks non-adapted from a singly-linked list implementation (no need for last)
current.next.last should be set depending on current == tail
should use find()
- insert fails to set old_head.last
- reverse: how about transmuting to an instance of ListLinkedDoubly, with roles of next and last exchanged(&head/tail, if sticking with no sentinel node (@Peilonrayz, again))
As presented, reverse() is a costly operation - reversed() returns an iterator
- __str__: return '[]' if 0 == self.count \
    else '[-(' + ')<=>('.join(self.items()) + ')-]'

Trying to stay DRY implementing pre/append():

def _add(self, item):
    """Update count and return new Node,
    attached to list if empty before.
    """
    self.count += 1
    new_node = Node(item)

    if self.head is None:
        self.head = self.tail = new_node
    return new_node

def append(self, item):
    """Return self with item inserted at the tail."""
    new_node = self._add(item)

    if 1 < self.count:
        self.tail.next = new_node
        new_node.last = self.tail
        self.tail = new_node

def prepend(self, item):
    """Return self with item inserted at the head."""
    new_node = self._add(item)

    if 1 < self.count:
        self.head.last = new_node
        new_node.next = self.head
        self.head = new_node

(I "real life", I like to be able to chain calls: return self)

another observation, two musings
Source Link
greybeard
  • 7.7k
  • 3
  • 21
  • 56
Loading
Source Link
greybeard
  • 7.7k
  • 3
  • 21
  • 56
Loading