# Tag Info

16

A few random comments on pieces of the code: class Subject: __metaclass__ = ABCMeta @abstractmethod def register_observer(observer): """Registers an observer with Subject.""" pass @abstractmethod def remove_observer(observer): """Removes an observer from Subject.""" pass @abstractmethod def ...

11

In my framework, I originally had a class whose sub classes were only supposed to contain public instance variables - kind of like a struct in C. ^^^ That, that's the source of your problem ^^^ Java is an object oriented language. Objects encapsulate their data, and the better Object designs make it impossible for other classes to directly access the ...

8

Are you reinventing the wheel? I'm sorry if this addresses just a little part of your question, but if you're looking for a way to get an event when a property is changed, have you thought about implementing INotifyPropertyChanged (msdn)? With this you could do stuff like: public class Person : INotifyPropertyChanged { ... public string ...

7

Traits First of all, we have to remove duplications caused by the implementation of the SplObject interface. We have here a great case for a trait: http://php.net/manual/en/language.oop5.traits.php. Here is a refactoring using a trait (the rest is unchanged): <?php trait Subject { private $observer = null; public function attach(SplObserver$...

7

One of the first changes I would make is to change how the set mutator on Output works. Assuming you already have an equality check for Pin (and if you don't, I would make one) you can add the following to the beginning of the mutator on Gate.OutputValue: if (_output == value) { return; } This means you won't change _output if there's nothing to ...

7

Using ContainsKey() together with the getter of the Item property of a Dictionary<TKey, TValue> should be replaced by a call to TryGetValue() which is faster because the check if the key exists is done by the Item getter too. Internally these three methods are calling the FindEntry() method to check whether a given key exists. So calling this method ...

7

Your service looks perfectly fine to me. As far as the Clock component goes, I can only recommend one thing. When getClock().subscribe(...) is executed, a reference to the subscription is being returned back. Ideally, the reference should be kept as long as the component is alive. On component destruction, the subscription should be released by invoking ...

6

Attention Bug Alert The way how the IsSetterCall() method is implemented is flawed. The method doesn't clearly distinguish between a property and a method. I could call a method like public void set_Test() { } and your method would assume this is a property which wouldn't cause any problems here, but later in the Invoke() method you use methodCall....

6

Don't use push_back on vectors, basically ever emplace_back() is almost always preferable. Slow removal Ideally, you'd want to store the observer list in an intrusive doubly-linked list, so that you can have O(1) removal. Unfortunately, this type of pattern is a bit of a blind spot in the STL, as there is no ideal container for it. (AFAIK, someone please ...

6

I'm not sure we really need a base Observable class to inherit from anymore, because with lambdas and function type erasure, this pattern has become more or less trivial. But if you want to do it that way, I think you can simplify and generalize it a bit more, by relaxing constraints in addObserver's signature (which I renamed attach because it's the ...

5

Pick a convention for your curly brackets/indentation and stick with it. You code has all of the following: Brackets included, one-lined statement Brackets omitted, one-lined statement New line after opening bracket, closing bracket on same line. New line after opening bracket, closing bracket on different line. Un-indented code inside brackets. Padding ...

5

There are a few things I'd consider changing in your code, especially since you state that you'd like to transition to modern C++. Using struct variables in C++ doesn't require prefixing them with the struct keyword as is needed in C, so your typedef struct {...} Callable2;, which I imagine you wrote so that you could get away with Callable2 foo; as ...

5

5

First of all, I think having public instance variables is a complete dead end. If you want to make sure that any time the instance variables are modified the listeners are notified, that needs to be encapsulated. It should be impossible or at least difficult to circumvent the listener notification process. Approach 1: Use a mediator From http://...

5

Design Review Ownership One of the advances of C++ over is C is ownership semantics. We understand who owns an object (and thus who is responsible for destroying the object). We do this by using a set of helper classes that explicitly state ownership or pass by reference to indicate non ownership. The trouble with pointers is that they have no ownership ...

5

1) int as time interval is ambiguous. TimeSpan is not. So I would use the latter. public int WaitTimeout { get; set; } 2) IsDisposed is a misleading name. I would not expect new HScheduler(...).IsDisposed to return true. 3) This does not look threadsafe: WaitHandle.WaitAll(waitHandles, WaitTimeout); Array.ForEach(waitHandles, (wh) => wh.Dispose()); ...

5

One of the improvements would be to use an HttpClient instead of Http for making the API calls. If you use HttpClient, you won't have to call the res.json() everytime you subscribe to the response. To use HttpClient, you'll first have to import HttpClientModule from @angular/common/http. Then you'll have to add it to the imports array of your @NgModule ...

4

You should probably bind ItemsSource to CollectionView, instead of binding to ObservableCollection. This way you don't have to recreate collection every time you want to apply your filter. May look like this: //constructor Parts = new ObservableCollection<EditPartViewModel>(allpvms); PartsCollectionView = (ListCollectionView)CollectionViewSource....

4

A Generic Observable is a neat idea. Much of the code that was initially designed for the Observer pattern in Java was then actually built using a custom interface for each 'event', and you ended up, for example, with the Listener and Event interfaces that are so common in the AWT/Swing frameworks. A more general-purpose system has merits though. What will ...

4

Here's a problem: var o = new Observable<Unit>(); var a = o.Subscribe(_ => Console.WriteLine("A")); var b = o.Subscribe(_ => Console.WriteLine("B")); a.Dispose(); o.Notify(Unit.Default); This prints A Disposing of a removed the wrong observer. I'd recommend reading Why shouldn't I implement IObservable<T>? The reason you shouldn't ...

4

Inheritance is by default private class concreteObserver : ObserverInterface ^^^^ Need public: here Mark overriden methods: void update(std::string message) // override ??? endl vs '\n' Don't use endl unless you absolutely want to flush the buffer. Probably OK here. Just wanted to note it. Don't pass pointers: void ...

4

Gate public IEnumerable<PinValue> OutputValues { get { PinValue[] result = new PinValue[0]; if (Output != null) { result = new PinValue[] { Output.Value }; } return result; } } there is no need to first create an array and later overwrite it if Output != null. You can simply use ...

4

You don't need a wasPresent variable. MutableSet.add returns true if the element was added to the set: if (observers.add(observer)) { init(observer) } Sometimes in Kotlin you don't have to declare temporary variables so that you can do something with a return value before returning it. e.g.: return ObservedProperty(initialValue, onChanged).apply { ...

4

Potential Bugs You don't define virtual destructors for Observer and Subject. This makes code such as Subject<int>* a = new ConcreteSubject<int>(3); //... delete a; invoke undefined behavior. The Single Responsibility Principle This principle, which is one of the foundations of good object oriented programming, states that each class ...

4

Design Is there a need for IEventDispatcher? Defining it as an abstract base class doesn't provide much utility if there's only ever going to be one derived class. Is there any requirement for EventDispatcher::eventListeners to be of type std::multimap<EventType, EventHandler>? It doesn't seem like the ordering of elements has much relevance, so a std:...

4

Make immutable class members const If you have a member of a class that is set once at construction time, but will never change, then make it const. For example, in Stock: class Stock { const std::string name; ... public: Stock(..., std::string stock_name, ...): name(stock_name) { ... } And once it is const, you can safely make name public ...

3

There is a bug in the "Values" property. It should return this.values (lower case "v") not this.Values (upper case "V"). Also the collection returned may not represent the way ordered in the way indexMap would have returned. See remarks section at https://msdn.microsoft.com/en-us/library/0yxt5h4s(v=vs.110).aspx.

3

RssParserService should NOT know concretely about its observers. It would be nearly the same as letting an abstract class know its derived classes. All your decoupling benefits through abstraction/dependency inversion (here the observer pattern) would be gone. On the other side your own observer implementations relies now on a dependency injection mechanism ...

3

I'm not sure how I feel about this. public void OnCompleted() { throw new NotImplementedException(); } In my opinion, if you're going to implement a contract, you should fulfill the contract, even if that means doing nothing at all. There's little more frustrating than getting an object that you believe implements an interface only to find out that it ...

3

Yes, you have made an implementation of the observer pattern. In you implementation of the notify function, you can use for(auto subscriber : subscribers){ (*subscriber)->update(message) } The name "SubjectInterface" is wrong, as you mix virtual declaration and code. It is an abstract class, so the name "AbstractSubject" should fit more. You ...

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