Skip to main content
2 extracting/simplification proposals
Source Link
mpasko256
  • 381
  • 1
  • 6

I don't see much to improve from algorithmic point of view. So I will focus on Object Oriented design.

  1. How about doing it similarly to Sort method? I mean instead of making abstract method, try just injecting Comparator into generic Heap implementation.

  2. People are generally scary of long and complex code. It would be nicer to try to split it into smaller parts:

  • First (most general) concept is to extract each huge operation (like Find, Downheap etc) into separate class with appropriate Single Responsibility. They usually accept in constructor original method parameters, hold its internal state as fields instead of local variables.

    They can also accept in constructor reference to original object but it would be even better design practice to extract internal Heap state into even smaller object and avoid circular dependency.

    Another approach is to make those classes stateless (just moving methods into separate classes and injecting Heap internal state)
  • Second idea would be to separate defensive programming from actual algorithms to make them cleaner and more readable (I would suggest to move them into a class mentioned in point 3. or into separate Facade responsible for validating user input)
  • You can also extract single steps of DownHeap and UpHeap routines into separate methods.
  1. You mentioned, that you struggled a little bit with collections.generic interfaces. My proposed solution would be not to pollute actual implementation but instead use Adapter Design Pattern

4. Another thing that comes into mind (beside OO) is to calculate numberOfLevels from either Count or capacity (using ceiled logarithm)

As #harold mentioned in comment it is probably unused

  1. You can simplify main body of the find routine:

     var rightResult = FindInternal(item, IndexToRightChildNode(index));
     var leftResult = FindInternal(item, IndexToLeftChildNode(index));
     return rightResult >= 0 ? rightResult : leftResult;
    

Please notice that in your case when neither rightResult nor leftResult has value greather than 0, the function will return -1 anyway.

  1. (Optionally, as applied in example of 5) You can add a an arrow functionIndexToLeftChildNode for completeness or to increase readability when you decide to inline some variables.

  2. You can extract common (utility) subroutine from DownHeap:

     private int UpdateLargestIndex(int currentLargestIndex, int childIndex) {
       if (childIndex < Count && CloserToRoot(innerT[childIndex].CompareTo(innerT[currentLargestIndex]))) {
         return childIndex;
       }
       return currentLargestIndex;
     }
    

I don't see much to improve from algorithmic point of view. So I will focus on Object Oriented design.

  1. How about doing it similarly to Sort method? I mean instead of making abstract method, try just injecting Comparator into generic Heap implementation.

  2. People are generally scary of long and complex code. It would be nicer to try to split it into smaller parts:

  • First (most general) concept is to extract each huge operation (like Find, Downheap etc) into separate class with appropriate Single Responsibility. They usually accept in constructor original method parameters, hold its internal state as fields instead of local variables.

    They can also accept in constructor reference to original object but it would be even better design practice to extract internal Heap state into even smaller object and avoid circular dependency.

    Another approach is to make those classes stateless (just moving methods into separate classes and injecting Heap internal state)
  • Second idea would be to separate defensive programming from actual algorithms to make them cleaner and more readable (I would suggest to move them into a class mentioned in point 3. or into separate Facade responsible for validating user input)
  1. You mentioned, that you struggled a little bit with collections.generic interfaces. My proposed solution would be not to pollute actual implementation but instead use Adapter Design Pattern

4. Another thing that comes into mind (beside OO) is to calculate numberOfLevels from either Count or capacity (using ceiled logarithm)

As #harold mentioned in comment it is probably unused

I don't see much to improve from algorithmic point of view. So I will focus on Object Oriented design.

  1. How about doing it similarly to Sort method? I mean instead of making abstract method, try just injecting Comparator into generic Heap implementation.

  2. People are generally scary of long and complex code. It would be nicer to try to split it into smaller parts:

  • First (most general) concept is to extract each huge operation (like Find, Downheap etc) into separate class with appropriate Single Responsibility. They usually accept in constructor original method parameters, hold its internal state as fields instead of local variables.

    They can also accept in constructor reference to original object but it would be even better design practice to extract internal Heap state into even smaller object and avoid circular dependency.

    Another approach is to make those classes stateless (just moving methods into separate classes and injecting Heap internal state)
  • Second idea would be to separate defensive programming from actual algorithms to make them cleaner and more readable (I would suggest to move them into a class mentioned in point 3. or into separate Facade responsible for validating user input)
  • You can also extract single steps of DownHeap and UpHeap routines into separate methods.
  1. You mentioned, that you struggled a little bit with collections.generic interfaces. My proposed solution would be not to pollute actual implementation but instead use Adapter Design Pattern

4. Another thing that comes into mind (beside OO) is to calculate numberOfLevels from either Count or capacity (using ceiled logarithm)

As #harold mentioned in comment it is probably unused

  1. You can simplify main body of the find routine:

     var rightResult = FindInternal(item, IndexToRightChildNode(index));
     var leftResult = FindInternal(item, IndexToLeftChildNode(index));
     return rightResult >= 0 ? rightResult : leftResult;
    

Please notice that in your case when neither rightResult nor leftResult has value greather than 0, the function will return -1 anyway.

  1. (Optionally, as applied in example of 5) You can add a an arrow functionIndexToLeftChildNode for completeness or to increase readability when you decide to inline some variables.

  2. You can extract common (utility) subroutine from DownHeap:

     private int UpdateLargestIndex(int currentLargestIndex, int childIndex) {
       if (childIndex < Count && CloserToRoot(innerT[childIndex].CompareTo(innerT[currentLargestIndex]))) {
         return childIndex;
       }
       return currentLargestIndex;
     }
    
added 147 characters in body
Source Link
mpasko256
  • 381
  • 1
  • 6

I don't see much to improve from algorithmic point of view. So I will focus on Object Oriented design.

  1. How about doing it similarly to Sort method? I mean instead of making abstract method, try just injecting Comparator into generic Heap implementation.

  2. People are generally scary of long and complex code. It would be nicer to try to split it into smaller parts:

  • First (most general) concept is to extract each huge operation (like Find, Downheap etc) into separate class with appropriate Single Responsibility. They usually accept in constructor original method parameters, hold its internal state as fields instead of local variables.

    They can also accept in constructor reference to original object but it would be even better design practice to extract internal Heap state into even smaller object and avoid circular dependency.

    Another approach is to make those classes stateless (just moving methods into separate classes and injecting Heap internal state)
  • Second idea would be to separate defensive programming from actual algorithms to make them cleaner and more readable (I would suggest to move them into a class mentioned in point 3. or into separate Facade responsible for validating user input)
  1. You mentioned, that you struggled a little bit with collections.generic interfaces. My proposed solution would be not to pollute actual implementation but instead use Adapter Design Pattern

4. Another thing that comes into mind (beside OO) is to calculate numberOfLevels from either Count or capacity (using ceiled logarithm)

As #harold mentionmentioned in comment it is probably unused

I don't see much to improve from algorithmic point of view. So I will focus on Object Oriented design.

  1. How about doing it similarly to Sort method? I mean instead of making abstract method, try just injecting Comparator into generic Heap implementation.

  2. People are generally scary of long and complex code. It would be nicer to try to split it into smaller parts:

  • First (most general) concept is to extract each huge operation (like Find, Downheap etc) into separate class with appropriate Single Responsibility. They usually accept in constructor original method parameters, hold its internal state as fields instead of local variables.

    They can also accept in constructor reference to original object but it would be even better design practice to extract internal Heap state into even smaller object and avoid circular dependency.
  • Second idea would be to separate defensive programming from actual algorithms to make them cleaner and more readable (I would suggest to move them into a class mentioned in point 3. or into separate Facade responsible for validating user input)
  1. You mentioned, that you struggled a little bit with collections.generic interfaces. My proposed solution would be not to pollute actual implementation but instead use Adapter Design Pattern

4. Another thing that comes into mind (beside OO) is to calculate numberOfLevels from either Count or capacity (using ceiled logarithm)

As #harold mention in comment it is probably unused

I don't see much to improve from algorithmic point of view. So I will focus on Object Oriented design.

  1. How about doing it similarly to Sort method? I mean instead of making abstract method, try just injecting Comparator into generic Heap implementation.

  2. People are generally scary of long and complex code. It would be nicer to try to split it into smaller parts:

  • First (most general) concept is to extract each huge operation (like Find, Downheap etc) into separate class with appropriate Single Responsibility. They usually accept in constructor original method parameters, hold its internal state as fields instead of local variables.

    They can also accept in constructor reference to original object but it would be even better design practice to extract internal Heap state into even smaller object and avoid circular dependency.

    Another approach is to make those classes stateless (just moving methods into separate classes and injecting Heap internal state)
  • Second idea would be to separate defensive programming from actual algorithms to make them cleaner and more readable (I would suggest to move them into a class mentioned in point 3. or into separate Facade responsible for validating user input)
  1. You mentioned, that you struggled a little bit with collections.generic interfaces. My proposed solution would be not to pollute actual implementation but instead use Adapter Design Pattern

4. Another thing that comes into mind (beside OO) is to calculate numberOfLevels from either Count or capacity (using ceiled logarithm)

As #harold mentioned in comment it is probably unused

Source Link
mpasko256
  • 381
  • 1
  • 6

I don't see much to improve from algorithmic point of view. So I will focus on Object Oriented design.

  1. How about doing it similarly to Sort method? I mean instead of making abstract method, try just injecting Comparator into generic Heap implementation.

  2. People are generally scary of long and complex code. It would be nicer to try to split it into smaller parts:

  • First (most general) concept is to extract each huge operation (like Find, Downheap etc) into separate class with appropriate Single Responsibility. They usually accept in constructor original method parameters, hold its internal state as fields instead of local variables.

    They can also accept in constructor reference to original object but it would be even better design practice to extract internal Heap state into even smaller object and avoid circular dependency.
  • Second idea would be to separate defensive programming from actual algorithms to make them cleaner and more readable (I would suggest to move them into a class mentioned in point 3. or into separate Facade responsible for validating user input)
  1. You mentioned, that you struggled a little bit with collections.generic interfaces. My proposed solution would be not to pollute actual implementation but instead use Adapter Design Pattern

4. Another thing that comes into mind (beside OO) is to calculate numberOfLevels from either Count or capacity (using ceiled logarithm)

As #harold mention in comment it is probably unused