aleab / toastify

:mega: TOASTIFY DEVELOPMENT HAS STOPPED | Toastify adds global hotkeys and toast notifications to Spotify
https://aleab.github.io/toastify
GNU General Public License v2.0
434 stars 48 forks source link

Internal Design Overhaul: ToastView #66

Open aleab opened 6 years ago

aleab commented 6 years ago

Internal Design Overhaul: ToastView

ToastView is one of the other monsters of Toastify codebase: 1200+ LOC. This is the code-behind of the Toast view and entry point of the whole App. It currently does the following:

I still have no ideas on how to refactor this.

Vankog commented 6 years ago

I'd say: One step at a time ;-) (don't hit me ^^)

Yeah, but I myself just looked up some general principles to get started: https://softwareengineering.stackexchange.com/a/180190:

Code Complete recommends creating an interface or a wrapper between the legacy code and new code that uses it. Keep the interface clean and as you need to make changes to the legacy code, refactor or rewrite the pieces that you need to and move them forward.

If you need to make enough changes, then eventually the classes will be brought up to par with new code.

https://softwareengineering.stackexchange.com/a/180187:

Refactoring is somewhat the whole books is written about. It's hard to retell all the methods and pitfalls.

  1. Before starting refactoring write the tests for a piece you want to refactor.
  2. Start refactoring with small and clean pieces of those big functions
  3. Find the piece that can be rewritten as a function and make it a function.
  4. Run tests to be sure you've done the things right and no side effects appears
  5. After this you should have and idea what has to be done with all other (big one) pieces. When the whole thing becomes clearer then you can make a higher-level redesign.

P.S. it's just a brief sketch. About refactoring itself is better to read some books (like M. Fowler - Refactoring, etc.)

http://www.drdobbs.com/architecture-and-design/making-large-classes-small-in-5-not-so-e/230600127:

Let's go over the principal techniques. I presume in this discussion that design has been done and it's now just a matter of writing the code. Or in the less attractive case, of maintaining code.

Diminish the workload. The first technique to apply is the single responsibility principle (SRP), which states that classes should do only one thing. How big that one thing is will determine in large part how big your classes are going to be. Reduce the work of each class; then, use other classes to marshal these smaller classes correctly.

Avoid primitive obsession. This obsession refers to the temptation to use collections in their raw form. This is definitely a code smell. If you have a linked list of objects, that linked list should be in its own class, with a descriptive name. Expose only the methods that the other classes need. This prevents other classes from performing operations without your knowledge on an object they don't own. The purpose of the list is also supremely clear and this encapsulation enables you to change easily to a different data structure if the need should arise later on.

Reduce the number of class and instance variables. A profusion of instance variables is a code smell. It strongly suggests that the class is doing more than one thing. It also makes it very difficult for subsequent developers to figure out what the class does. Very often, some subset of the variables form a natural grouping. Group them into a new class. And move the operations that manipulate them directly into that class.

Subclass special-case logic. If you have a class that includes rarely used logic, determine whether that logic can be moved to a subclass or even to another class entirely. The classic example of the benefits of object orientation is polymorphism. Use it to handle special variants.

Don't repeat yourself (DRY). This suggestion appears pointlessly obvious. However, even coders who are attentive to this rule will repeat code in two methods that differ only in a single detail. In addition, they can overlook the introduction of duplicate code during maintenance. More than the other guidelines here, which are all techniques, DRY is a discipline within a discipline.

Taken together, these tools get you most of the way to small classes.

https://softwareengineering.stackexchange.com/a/276658

Before tackling a refactor, I always want to make sure that I have a firm understanding of what it is I'm changing. There are two methods I generally employ when tackling this kind of thing: logging and debugging. The latter is facilitated by the existence of unit tests (and system tests if they make sense). As you don't have any, I'd strongly urge you to write tests with full branch coverage to ensure that your changes don't create unexpected behavioural changes. Unit tests and stepping through the code help you get a low level understanding of behaviour under a controlled environment. Logging helps you get a better understanding of behaviour in practice (this is especially useful in multithreaded situations).

Once I get a better understanding of everything that's going on through debugging and logging, then I'll start reading code. At this point, I generally have a much better grasp of what's going on and sitting down and reading 2k LoC should make much more sense at that point. My intention here is more to get an end-to-end view and make sure that there isn't any sort of odd edge cases that the unit tests I've written aren't covering.

Then I'll give the design a think and draw up a new one. If backwards compatibility is of any importance, I'll make sure that the public interface doesn't change.

With a firm understanding of what's going on, tests and a fresh new design in hand, I'll put it up for review. Once the design has been vetted by at least two people (hopefully familiar with the code), I'll start implementing the changes.

So, yeah... Getting to know the code, its responsibilities, its dependencies, write tests, divide and conquer.

I my eyes this should not be done in a separate branch, but in the master. Otherwise one might be tempted to do too much at once instead of doing small steps at a time without breaking the project. Introducing temporary adapters and wrappers between new and legacy code.

aleab commented 6 years ago

I my eyes this should not be done in a separate branch, but in the master. Otherwise one might be tempted to do too much at once instead of doing small steps at a time without breaking the project. Introducing temporary adapters and wrappers between new and legacy code.

Agreed! 👍

Vankog commented 6 years ago

Have you used the Live Dependency Validation Feature of VS before? https://almvm.azurewebsites.net/labs/tfs/livedependencyvalidation/ This sounds awesome for when the project is in a good state again.

aleab commented 6 years ago

Nope, never used it. I'll take a look at it.