Skip to main content
added 150 characters in body
Source Link
coderodde
  • 32.3k
  • 15
  • 79
  • 205

I am not a professional C++ programmer, yet I can provide some minor stylistic advices.

Intro Actually, you have implemented a binary heap that runs Insert, Update, ExtractMinimum in all \$\mathcal{O}(\log N)\$ time. Nice work!

Advice 1 - Rename class parameter names

You have two main type parameters: K and V. K as the data key and V as the "value". Actually, V is used as the priority type; thus, I suggest you rename V to Priority and K to Key.

Advice 2 - Use header guards

I would place the entire header file content into:

#ifndef COM_CODEREVIEW_STACKEXCHANGE_JZHU379_UTIL_BINARY_HEAP_H
#define COM_CODEREVIEW_STACKEXCHANGE_JZHU379_UTIL_BINARY_HEAP_H

// Your entire binary heap code.

#endif // COM_CODEREVIEW_STACKEXCHANGE_JZHU379_UTIL_BINARY_HEAP_H

Advice 3 - Wrap your heap implementation in a namespace Along the Advice 2, I would also place your actual code into a namespace:

namespace com::stackexchange::codereview::jzhu379::util {
    // Code here.
} // end of namespace ::stackexchange::codereview::jzhu379::util

Advice 4 - Wrong default priority comparator You have:

class Compare = std::less<Priority> // Note Priority against V.

Unfortunately, that yields a max heap. If you want the default priority order be such that the lower priority key, the higher the actual priority, you must do:

class Compare = std::greater<Priority>
You have:

class Compare = std::less<Priority> // Note Priority against V.

Unfortunately, that yields a max heap. If you want the default priority order be such that the lower priority key, the higher the actual priority, you must do:

class Compare = std::greater<Priority>

Advice 5 - A minor neat pick You have:

bool containsKey(const Key& key) const { return keyIdx_.count(key); }

I understand that you implicitly convert size_t to bool, but why not do instead:

bool containsKey(const Key& key) const { return keyIdx_.count(key) > 0; }
                                                                   ^^^^

Advice 6 - Additional advice You use two functions: moveUpwards and moveDownwards. What comes to implementing binary heaps, the usual and customary names for those are siftUp and siftDown.

Advice 7 - pop returns silently Your pop returns silently when the heap is empty. The std::priority_queue throws an exception.

I am not a professional C++ programmer, yet I can provide some minor stylistic advices.

Intro Actually, you have implemented a binary heap that runs Insert, Update, ExtractMinimum in all \$\mathcal{O}(\log N)\$ time. Nice work!

Advice 1 - Rename class parameter names

You have two main type parameters: K and V. K as the data key and V as the "value". Actually, V is used as the priority type; thus, I suggest you rename V to Priority and K to Key.

Advice 2 - Use header guards

I would place the entire header file content into:

#ifndef COM_CODEREVIEW_STACKEXCHANGE_JZHU379_UTIL_BINARY_HEAP_H
#define COM_CODEREVIEW_STACKEXCHANGE_JZHU379_UTIL_BINARY_HEAP_H

// Your entire binary heap code.

#endif // COM_CODEREVIEW_STACKEXCHANGE_JZHU379_UTIL_BINARY_HEAP_H

Advice 3 - Wrap your heap implementation in a namespace Along the Advice 2, I would also place your actual code into a namespace:

namespace com::stackexchange::codereview::jzhu379::util {
    // Code here.
} // end of namespace ::stackexchange::codereview::jzhu379::util

Advice 4 - Wrong default priority comparator You have:

class Compare = std::less<Priority> // Note Priority against V.

Unfortunately, that yields a max heap. If you want the default priority order be such that the lower priority key, the higher the actual priority, you must do:

class Compare = std::greater<Priority>

Advice 5 - A minor neat pick You have:

bool containsKey(const Key& key) const { return keyIdx_.count(key); }

I understand that you implicitly convert size_t to bool, but why not do instead:

bool containsKey(const Key& key) const { return keyIdx_.count(key) > 0; }
                                                                   ^^^^

Advice 6 - Additional advice You use two functions: moveUpwards and moveDownwards. What comes to implementing binary heaps, the usual and customary names for those are siftUp and siftDown.

I am not a professional C++ programmer, yet I can provide some minor stylistic advices.

Intro Actually, you have implemented a binary heap that runs Insert, Update, ExtractMinimum in all \$\mathcal{O}(\log N)\$ time. Nice work!

Advice 1 - Rename class parameter names

You have two main type parameters: K and V. K as the data key and V as the "value". Actually, V is used as the priority type; thus, I suggest you rename V to Priority and K to Key.

Advice 2 - Use header guards

I would place the entire header file content into:

#ifndef COM_CODEREVIEW_STACKEXCHANGE_JZHU379_UTIL_BINARY_HEAP_H
#define COM_CODEREVIEW_STACKEXCHANGE_JZHU379_UTIL_BINARY_HEAP_H

// Your entire binary heap code.

#endif // COM_CODEREVIEW_STACKEXCHANGE_JZHU379_UTIL_BINARY_HEAP_H

Advice 3 - Wrap your heap implementation in a namespace Along the Advice 2, I would also place your actual code into a namespace:

namespace com::stackexchange::codereview::jzhu379::util {
    // Code here.
} // end of namespace ::stackexchange::codereview::jzhu379::util

Advice 4 - Wrong default priority comparator You have:

class Compare = std::less<Priority> // Note Priority against V.

Unfortunately, that yields a max heap. If you want the default priority order be such that the lower priority key, the higher the actual priority, you must do:

class Compare = std::greater<Priority>

Advice 5 - A minor neat pick You have:

bool containsKey(const Key& key) const { return keyIdx_.count(key); }

I understand that you implicitly convert size_t to bool, but why not do instead:

bool containsKey(const Key& key) const { return keyIdx_.count(key) > 0; }
                                                                   ^^^^

Advice 6 - Additional advice You use two functions: moveUpwards and moveDownwards. What comes to implementing binary heaps, the usual and customary names for those are siftUp and siftDown.

Advice 7 - pop returns silently Your pop returns silently when the heap is empty. The std::priority_queue throws an exception.

added 3 characters in body
Source Link
coderodde
  • 32.3k
  • 15
  • 79
  • 205

I am not a professional C++ programmer, yet I can provide some minor stylistic advices.

Intro Actually, you have implemented a binary heap that runs Insert, Update, ExtractMinimum in all \$\mathcal{O}(\log N)\$ time. Nice work!

Advice 1 - Rename class parameter names

You have two main type parameters: K and V. K as the data key and V as the "value". Actually, V is used as the priority type; thus, I suggest you rename V to Priority and K to Key.

Advice 2 - Use header guards

I would place the entire header file content into:

#ifndef COM_CODEREVIEW_STACKEXCHANGE_JZHU379_UTIL_BINARY_HEAP_H
#define COM_CODEREVIEW_STACKEXCHANGE_JZHU379_UTIL_BINARY_HEAP_H

// Your entire binary heap code.

#endif // COM_CODEREVIEW_STACKEXCHANGE_JZHU379_UTIL_BINARY_HEAP_H

Advice 3 - Wrap your heap impelemntationimplementation in a namespace Along the Advice 2, I would also place your actual code into a namespace:

namespace com::stackexchange::codereview::jzhu379::util {
    // Code here.
} // end of namespace ::stackexchange::codereview::jzhu379::util

Advice 4 - Wrong default priority comparator You have:

class Compare = std::less<Priority> // Note Priority against V.

Unfortunately, that yields a max heap. If you want the default priority order be such that the lower priority key, the higher the actual priority, you must do:

class Compare = std::greater<Priority>

Advice 5 - A minor neat pick You have:

bool containsKey(const Key& key) const { return keyIdx_.count(key); }

I understand that you implicitly convert size_t to bool, but why not do instead:

bool containsKey(const Key& key) const { return keyIdx_.count(key) > 0; }
                                                                   ^^^^^^^^^

Advice 6 - Additional advice You use two functions: moveUpwards and moveDownwards. What comes to implementing binary heaps, the usual and customary names for those are siftUp and siftDown.

I am not a professional C++ programmer, yet I can provide some minor stylistic advices.

Intro Actually, you have implemented a binary heap that runs Insert, Update, ExtractMinimum in all \$\mathcal{O}(\log N)\$ time. Nice work!

Advice 1 - Rename class parameter names

You have two main type parameters: K and V. K as the data key and V as the "value". Actually, V is used as the priority type; thus, I suggest you rename V to Priority and K to Key.

Advice 2 - Use header guards

I would place the entire header file content into:

#ifndef COM_CODEREVIEW_STACKEXCHANGE_JZHU379_UTIL_BINARY_HEAP_H
#define COM_CODEREVIEW_STACKEXCHANGE_JZHU379_UTIL_BINARY_HEAP_H

// Your entire binary heap code.

#endif // COM_CODEREVIEW_STACKEXCHANGE_JZHU379_UTIL_BINARY_HEAP_H

Advice 3 - Wrap your heap impelemntation in a namespace Along the Advice 2, I would also place your actual code into a namespace:

namespace com::stackexchange::codereview::jzhu379::util {
    // Code here.
} // end of namespace ::stackexchange::codereview::jzhu379::util

Advice 4 - Wrong default priority comparator You have:

class Compare = std::less<Priority> // Note Priority against V.

Unfortunately, that yields a max heap. If you want the default priority order be such that the lower priority key, the higher the actual priority, you must do:

class Compare = std::greater<Priority>

Advice 5 - A minor neat pick You have:

bool containsKey(const Key& key) const { return keyIdx_.count(key); }

I understand that you implicitly convert size_t to bool, but not do instead:

bool containsKey(const Key& key) const { return keyIdx_.count(key) > 0; }
                                                                   ^^^^^

Advice 6 - Additional advice You use two functions: moveUpwards and moveDownwards. What comes to implementing binary heaps, the usual and customary names for those are siftUp and siftDown.

I am not a professional C++ programmer, yet I can provide some minor stylistic advices.

Intro Actually, you have implemented a binary heap that runs Insert, Update, ExtractMinimum in all \$\mathcal{O}(\log N)\$ time. Nice work!

Advice 1 - Rename class parameter names

You have two main type parameters: K and V. K as the data key and V as the "value". Actually, V is used as the priority type; thus, I suggest you rename V to Priority and K to Key.

Advice 2 - Use header guards

I would place the entire header file content into:

#ifndef COM_CODEREVIEW_STACKEXCHANGE_JZHU379_UTIL_BINARY_HEAP_H
#define COM_CODEREVIEW_STACKEXCHANGE_JZHU379_UTIL_BINARY_HEAP_H

// Your entire binary heap code.

#endif // COM_CODEREVIEW_STACKEXCHANGE_JZHU379_UTIL_BINARY_HEAP_H

Advice 3 - Wrap your heap implementation in a namespace Along the Advice 2, I would also place your actual code into a namespace:

namespace com::stackexchange::codereview::jzhu379::util {
    // Code here.
} // end of namespace ::stackexchange::codereview::jzhu379::util

Advice 4 - Wrong default priority comparator You have:

class Compare = std::less<Priority> // Note Priority against V.

Unfortunately, that yields a max heap. If you want the default priority order be such that the lower priority key, the higher the actual priority, you must do:

class Compare = std::greater<Priority>

Advice 5 - A minor neat pick You have:

bool containsKey(const Key& key) const { return keyIdx_.count(key); }

I understand that you implicitly convert size_t to bool, but why not do instead:

bool containsKey(const Key& key) const { return keyIdx_.count(key) > 0; }
                                                                   ^^^^

Advice 6 - Additional advice You use two functions: moveUpwards and moveDownwards. What comes to implementing binary heaps, the usual and customary names for those are siftUp and siftDown.

Source Link
coderodde
  • 32.3k
  • 15
  • 79
  • 205

I am not a professional C++ programmer, yet I can provide some minor stylistic advices.

Intro Actually, you have implemented a binary heap that runs Insert, Update, ExtractMinimum in all \$\mathcal{O}(\log N)\$ time. Nice work!

Advice 1 - Rename class parameter names

You have two main type parameters: K and V. K as the data key and V as the "value". Actually, V is used as the priority type; thus, I suggest you rename V to Priority and K to Key.

Advice 2 - Use header guards

I would place the entire header file content into:

#ifndef COM_CODEREVIEW_STACKEXCHANGE_JZHU379_UTIL_BINARY_HEAP_H
#define COM_CODEREVIEW_STACKEXCHANGE_JZHU379_UTIL_BINARY_HEAP_H

// Your entire binary heap code.

#endif // COM_CODEREVIEW_STACKEXCHANGE_JZHU379_UTIL_BINARY_HEAP_H

Advice 3 - Wrap your heap impelemntation in a namespace Along the Advice 2, I would also place your actual code into a namespace:

namespace com::stackexchange::codereview::jzhu379::util {
    // Code here.
} // end of namespace ::stackexchange::codereview::jzhu379::util

Advice 4 - Wrong default priority comparator You have:

class Compare = std::less<Priority> // Note Priority against V.

Unfortunately, that yields a max heap. If you want the default priority order be such that the lower priority key, the higher the actual priority, you must do:

class Compare = std::greater<Priority>

Advice 5 - A minor neat pick You have:

bool containsKey(const Key& key) const { return keyIdx_.count(key); }

I understand that you implicitly convert size_t to bool, but not do instead:

bool containsKey(const Key& key) const { return keyIdx_.count(key) > 0; }
                                                                   ^^^^^

Advice 6 - Additional advice You use two functions: moveUpwards and moveDownwards. What comes to implementing binary heaps, the usual and customary names for those are siftUp and siftDown.