# Tag Info

75

1. Encapsulate Writing code at the top level of a module makes it hard to test the code and hard to measure its performance. It's best to encapsulate code in a function. Accordingly, I'd write: def survivor(n): """Return the survivor of a circular firing squad of n people.""" persons = list(range(1, n + 1)) while len(persons) > 1: ...

21

The arithmetic solution to this is relatively simple as we're talking modulo here: return i % N; This should produce the same results for your adjusted index. When I ran a few calculations on the modulo operator through WolframAlpha, I got the following results: for $F(x) = x \mod 3$ $5 \mapsto 2$ $4 \mapsto 1$ $3 \mapsto 0$ \\$ 2 \mapsto 2 ...

14

This is well documented, well commented code. The docstring says: The one way in which slicing a WraparoundList differs from slicing an ordinary list is the case of using the list length as the upper limit. but this isn't quite the whole story — an ordinary list can also be sliced using a value greater than the list length, and in that case ...

13

On testing... Your tests are incomplete, certainly. Your test on isEmpty only tests two conditions: a newly instantiated queue which has never had anything added to it should return true, and after adding just one thing, it should return false. How does isEmpty do when the queue is full? How does isEmpty do when the queue has had two items added and ...

12

Stumbled upon a simpler solution here (notice the %% operator): template <typename T, int N> int betterModulo(int i, T(&)[N]) { return (i % N + N) % N; } Further searching turned up this, which helped me feel like I was not crazy for wanting to think of things as circular... :)

12

Your code looks pretty decent. My two cents: Implement Default impl Default for Fifo { fn default() -> Fifo { Fifo { size: 0, read_idx: 0, write_idx: 0, buffer: [0; FIFO_CAPACITY], } } } Then you could simplify your new to: pub fn new() -> Fifo { Fifo::default() } Simplify ...

11

there is absolutely no thread safety constructs, this means that you'll see a lot of interesting race conditions and memory inconsistencies just assuming dequeues happen fast enough does not mean they will in production you return T by value in Obtain(), instead of by reference so you can't actually write into it Obtain is a bad name, it sounds like it gets ...

11

I'm not a big algorithms/data structures guy, but the only thing that caught my attention was the delete function. Is there a reason that delete uses data to identify the node, as opposed to just taking in the Node object you want to delete? It seems like that could get hairy if two nodes have the same data (which happens in main). Besides that, there are a ...

11

Empty Lists if (list.isEmpty()) { throw new IllegalArgumentException("list cannot be empty"); } Yuck ... you should be able to handle an empty list more gracefully than that. Creating a special EmptyCircularListCursor instance perhaps. Random Access I like that you differentiate between RandomAccess and other lists. This is a good ...

11

Some basic comments As a small point to begin with, I think your code to do with some more whitespace, especially when starting a new method. The way it is currently, it's very densely packed, which makes it harder to read. Your operator= has no exception safety, and does not check for self-assignment. The copy and swap idiom is the easiest way to fix this ...

11

Naming There is one 'u' in ConcuurentCircularBuffer that wants to be a 'r' --> ConcurrentCircularBuffer There is no need to abbreviate name like size (sz --> size) I would call last to something more descriptive like lastIndex. Code Style sz, buffer and lockObject should be read-only. There is no need to skip the first array element, just start with 0. ...

11

I think std::rotate can lead to a somewhat simpler approach: get the input create a copy rotate the copy by one element Accumulate the sum of elements that match between the two Code might look something on this general order: int sum_matching(std::vector<int> const &digits) { std::vector<int> copy{digits}; std::rotate(copy.begin()...

11

Coding idiomatically Remember that #pragma once isn't standard (unlike include guards). If you only intend your class works on major compilers, you can use #pragma once (but care about defects and drawbacks) If you wish your class works on all compilers, use both #pragma once and the include guards. If you want your class to be 100% compatible with ...

10

Your program is really based only of case where you have 5 nodes. This make your code really hard to change if you want to have a "list" of node of a different size. The name of the class should represent a concept not an action, CountNodesBetweenTwoNodes could be something like CircularList or something like that. A name that would represent what the ...

10

Java style convention One of the first thing you should do when working in a new language is to look at style convention. You used C++ convention and not the Java one. Class name should be PascalCase and method name should be camelCase. Unless it's a constant, you should not use _ (except in some precise case). if(isEmpty()) { ... } else ...

10

First, a minor nitpick. public boolean isEmpty() { return(tail == head) ? true : false; } public boolean isFull() { int diff = tail - head; if(diff == -1 || diff == (capacity - 1)) { return true; } return false; } Twice we're using returning true or false based on the result of a conditional. Why not just directly return the ...

10

Rather than implement all the memory management yourself use an existing container and just implement the circular part yourself. template <typename T> class CircularBuffer { std::vector<T> buffer; // maybe there is a better one ... // I would think on this a bit. The main reason is that std::vector will not ...

10

In addition to @JanDotNet's answer: You can use "less strict" lock in your code, specifically ReaderWriterLock or ReaderWriterLockSlim. As I see from your code, you can execute Read in parallel threads but cannot execute Put and Read in parallel. These classes are exactly for this case and will improve overall performance.

10

Quick Review An API like this, dealing with thread-sensitive operations, requires time and effort to test and review rigorously. When I will find this time, I will do a thorough review. But here are some things I notice right off the bat. CommMemoryDisposedException should inherit from ObjectDisposedException. This way, consumers can handle your exception ...

9

Use assignment instead of memcpy A better implementation of Obtain can return a non-const reference which you can assign to: T& Obtain() { return storage[curWriteNum & MASK]; } Used like this: void Processor::EnqueueFutOrderbook(orders* param) { orders& storageItem = futOrdersUpdates.Obtain(); storageItem = *param; ...

9

Explicit visibility I don't like relying on standard visibility; explicit declaring takes away all doubts. Instead have this: private int[] queue; private int front; private int rear; private int currentSize; private int size; Implicit this Contrary to the above having explicit this is unneeded and takes away from the readability. You should only use ...

9

Your current solution is actually pretty good, conceptually. There's nothing wrong with the start variable. I am not sure why you want to remove it. The loop is logically a good solution, but there's a better way than that, though (better because you can make the system do it for you....). public int[] shiftLeft(int[] nums) { if (nums == null || nums....

9

Yay exceptions! Though it isn't reservered for execution-ending errors, I would avoid throwing an OutOfMemoryException, because usually it signals something very inconvient indeed. IndexOutOfRangeException also seems inappropriate: I would probably use InvalidOperationException in both Enqueue and Dequeue. The constructor could do with a check to ensure ...

8

Focusing directly on the countNodesBetweenTwoNodes private int countNodesBetweenTwoNodes(Node firstNode, Node secondNode) { int count = 0; while(firstNode.next != secondNode) { count++; firstNode = firstNode.next; } return count; } The method name is way too long... that's an AppleScript name! The arguments in a Java ...

8

You shouldn't have words in global scope. As you're just passing it to another function from main(), initialize it within main(). It should also be const since it's not being modified anywhere. const std::string words[] = { "this", "and", "that" }; Also, if you have C++11, use std::array instead. C-arrays should be avoided in C++. constexpr std::array&...

8

Unfortunately I can't answer your initial question, as the analysis would take more time than I can afford; but the second question I can give you some bullet-points on: From my experience the CUDA compiler is not as smart as the mainstream C/C++ compilers, and there's a lot of things that would be optimized out in more advanced compilers that aren't in ...

8

You can make your code much simpler if you use a LinkedList<T> instead of an array. It has also a much better performance when inserting and removing items in this case which are both O(1) operations: public class ConcurrentCircularBuffer<T> : ICircularBuffer<T> { private readonly LinkedList<T> _buffer; private int ...

8

Issues (for me) I get a couple of errors when I try and compile. This error I have a feeling is caused because you are using a more up to date compiler (I presume this is a C++17 feature). if constexpr (sizeof...(Tail) > 0) { ^^^^^^^^^ This problem causes my second error (because I remove constexpr to make it compile): rt.cpp:12:6: note: candidate ...

7

You have done the correct thing by putting a sentinel element into the list. This makes adding/removing and generally manipulating the list trivial because you don't need to handle NULL links in the list. But you have done it using a two phase create. You have to construct your list. You have to call create to initialize it. This is a very bad idea. ...

7

I agree with @User1 about RB_pop() and would like to add on to it: In order to prevent the function from returning an unexpected return value of false, you should make the function void and have a second parameter data. This will also allow you to return early if the first conditional statement is false. void RB_pop(ring_buffer* rb, int8_t* data) { if ...

Only top voted, non community-wiki answers of a minimum length are eligible