Code review/architecture feedback please…

[clean_coder_student]

Hi,

Reading about ‘programming against interfaces’, single responsibility, ISP, clean code, etc  I created a (very) small c# (winforms) test project which I hope somebody can have a look at and give me feedback.
The objective was to create reusable code that is as loosely coupled as possible (implementing the knowledge gained so far)

To make it concrete I build a simple login form (you know the one where you are asked for a username/password).
Code I now wrote about 10 time for multiple applications, every time thinking ‘oh, well lets write it again, is faster than digging up the previous one’

I created a test project with a form and 1 button to start the real thing (the login form)
I separated the login view from the login ‘handler’ (in order to prevent confusion regarding MVC & MVP, I called it handler)
Both have their own interfaces so only know each other’s interfaces.

And yes I had a look at MVC & MVP for windows forms, but what I saw so far I didn’t like (in one, the default forms designer couldn’t be used. All buttons/labels/etc are created manually which is not my idea of productivity, and in the other there is a coupling between the two). Other things  where mainly focusing on ASP.
If anybody has some good links regarding MVC/MVP for c# winForms, I would be grateful.

The idea is that we can test the handler without the view and both can change independently (as long as the interfaces don’t change).
So main objectives:
-testable code (test handler without testing the view)
-reusable code (loosely coupled, swap one view for another with the same handler, or swap handlers for same view, etc)

But the point is  that most of the changes I can imagine at this stage, do need the interface to change, meaning that older code will have to implement the new interface changes which on its turn makes me ask if this is useful at al.

Let say we would like to add a label that warns if the caps locks key is down, this means we need a label on the form, we could handle the logic in the view as well but not sure that is nice. And if we change the interface in order for the handler to handle it, we are not backward compatible.

Code can be downloaded via this link: 

https://www.dropbox.com/s/vih7q03ulyepoba/loginTester_v2.zip

Feedback on code & mainly on architecture are appreciated,

Regards,

[Dave Schinkel]

This isn't to answer your question but I took  look at the code.  Any reason why you're adding an unnecessary extra space between a method and ()?  it looks weird/minor annoyance to me.  Same thing with even method params: Application.Run (new Form1 ());

any reason you need to do this?  doesn't make it any more readable IMO.

[Dave Schinkel]

sorry I feel like a bit of code review.

Do you use ReSharper?

Any reason not to use a using statement instead of all the reduntant System.Windows.Forms to fully qualify types?

private System.Windows.Forms.TextBox txtUserName;

sorry not to knock, these things just bug me.  :)

On Saturday, July 5, 2014 6:33:39 AM UTC-5, clean_coder_student wrote:

[Dave Schinkel]

Hehe on more thing.  Don't use Hungarian notation.

On Saturday, July 5, 2014 6:33:39 AM UTC-5, clean_coder_student wrote:

[Dave Schinkel]

When I use ReSharper I see hints to use a using statement.  I assume since you are a student (or your username says student so I assume) you probably haven't invested in this tool??  If not you can learn a lot from it and it can save you a TON of time

The C# version is around $150 but well worth it.

On Saturday, July 5, 2014 6:33:39 AM UTC-5, clean_coder_student wrote:

[Dave Schinkel]

I should have bundled up my code review in ONE reply, so I know, it's annoying.

 but also you can take out all those unecessary "this." in front of your types.  They're unnecessary meaning clutter and get int the way of overall readability IMO.  Only qualify types with "this." if you are absolutely forced by the compiler to do so (name conflicts), otherwise 95% of the time you don't need to use that, C# is smart enough to figure out those are its own members.

[Dave Schinkel]

So you are a student right?  What school?  How far are you  (freshman, etc)?  and what's the curriculum there, are you in Computer Science?  If so is the focus right now on just C# Win Forms?  What version of .NET?  Doing any JavaScript in your classes yet?

[clean_coder_student]

Hi Dave/others,

Thanks for looking into the code, really appreciated.
Thanks for the tip, I will look into ReSharper.
Sorry for the forum name confusion, not really a student but I think that in our field of work we are eternal students. There is so much new & interesting tec/software/edi/etc coming on the market every day… For now I’m catching up with clean code which I like a lot (hence the name).
I use c# for windows, java for android and still c for embedded systems.

What did you think of the design? I’m very interested in your take on that.
In case you wanted to write something like this (a dialog box, view & control) would you use a 3th object to link the 2 via there interfaces or do it differently, put it all in one or split it out in classes, etc?
And do you use MVC/MVP? Any tips on that?
All I could find are these two but as mentioned not my preference
mvp       http://www.codeproject.com/Articles/522809/WinForms-MVP-An-MVP-Framework-for-WinForms
mvc       http://www.codeproject.com/Articles/383153/The-Model-View-Controller-MVC-Pattern-with-Csharp

Regards,

[Łukasz Duda]

Hi,
I've added simple example to show how I implement presenter: https://github.com/lukasz-duda/PresenterDemo
As for code review:

• Use recommended capitalization conventions: http://msdn.microsoft.com/en-us/library/ms229043%28v=vs.110%29.aspx and naming conventions http://msdn.microsoft.com/en-us/library/ms229002%28v=vs.110%29.aspx (methods and classes PascalCase, don't use Hungarian notation)
  
• Name Form1 doesn't tell what's inside
• Don't use unnecessary comments like this //**************************************************************************
• Your indentation is inconsistent - for example clsLoginViewHandlerSimple comparing to Form1
• As for the presentation architecture: both ILoginView and ILoginViewHandler depend on System.Windows.Forms. I prefer when interfaces don't impose implementation. Then they're more reusable and you don't have to change them so frequently.

Why did you say that you can't use form designer?

I hope my example will help.

Greetings, Lucas

[clean_coder_student]

Hi Lukasz/others,

Thanks that is great.

> As for the presentation architecture: both ILoginView and ILoginViewHandler depend on System.Windows.Forms

I see your point… Will give it some more thought. Its cleaner if the handler code is completely loose from System.Windows.Forms but it would mean that types/parameters like FormClosingEventArgs need to be replaced by something hand made. Question would be if that is useful since we may want to use features now in FormClosingEventArgs later (ok I smell a YAGNI)

> Why did you say that you can't use form designer?

Visual studio (2013) gives an error as soon as you open the (sample) form. (mvp codeproject sample in previous post)


Ok let me see if I get this right…
You have a view (orderview) which implements an interface (IOrderView)
We have a Presenter (orderPresenter) which receives an IOrderView so it can use the view.
You wire this up in main()

Than we have tree other classes, ShowOrderUseCase, ShowOrderRequest and Order which I assume are the model part of MVP and are in a separate class library (SalesApplication)
ShowOrderRequest is simply some data, same for Order.

The View & Presenter are nice loosely coupled though the interface.
Also the model layer defines the Presenter Interface (IShowOrderResporder) which is implemented on the GUI side  by the presenter (orderPresenter)
So Model knows the presenter (loosely coupled via interface) and presenter knows View (also loosely coupled via interface)

For those who don’t have visual studio, see quick class diagram here.
https://www.dropbox.com/s/7rfcwkejhyr76jl/ClassDiagram.png

Ok, hope you are willing to answer some questions.
-What is the difference between the two classes Order & ShowOrderRequest? They both seem simple data classes. What is the functional difference?
-here communication is only from model via presenter to view. For input going back from Form(view) to presenter, would you give the presenter object to the view?, or add a new (presenter)interface and hand that to the view?
-let say I converted my login sample to this MVP pattern and I wanted to add a message in the form warning customers  that caps lock was on, that would be code added in the presenter correct?
-same but now for passwords rules (you know, must be >=8 characters, must contain at least 1 number, etc) that would be put in the model layer?

Thanks again & regards,

[Łukasz Duda]

Hi,
Here are some answers:

1. ShowOrderRequest and Order both are data transfer objects, but ShowOrderRequest is the use case's input and Order is the output - part of the responder. Don't pay attention to the components and namespaces partitioning in this example.
   
2. I let the view know concrete presenter type. I've commited an update. Now the presenter could work with different view implementations, but the view is created for specified presenter.
3. I think I would add some property for the password field control, to show warning when caps lock is on. But it could be in presenter as well. It depends whether I consider it critical and want to be tested, which I usually don't in such case ;-)
   
4. I put rules for setting new password in the use case implementation or it's dependencies (interactors layer). I could repeat some of the rules in the view controls properties or presenter to let user know before he confirms all changes.

ok, I've reached timeout exception ;-P maybe I continue later

greetings

[clean_coder_student]

Hi Lukasz,

Thanks for the reply, I had a look at the update, ok so view knows the presenter.
To be honest I don’t really get the(functional/design) difference between view & presenter, it seems presenter is only some form of relay between model & view(form).
Why not let model talk to view directly via its interface?
I can see from both testing point & design that we want to separate the view/form & model layer but I have to look more into this 3 way split to find its use.

I like the UseCase naming, it kind of makes you focus on useCases and what users want.
Can you recommend any good books (or other  resources) regarding MVP? I can’t find any at amazon.

With kind regards.

[Łukasz Duda]

Hi,

>> As for the presentation architecture: both ILoginView and ILoginViewHandler depend on System.Windows.Forms
>I see your point… Will give it some more thought. Its cleaner if the handler code is completely loose from System.Windows.Forms but it would mean that types/parameters like FormClosingEventArgs need to be replaced by something hand made. Question would be if that is useful since we may want to use features now in FormClosingEventArgs later (ok I smell a YAGNI)

YAGNI, as I understand, tells about implementing requirements not needed at the time. There aren't any new requirements implemented. The design has changed, to isolate dependencies and make abstract code more readable and reusable. Architectural decisions can't violate YAGNI in my opinion.

>Ok let me see if I get this right…
>You have a view (orderview) which implements an interface (IOrderView)
>We have a Presenter (orderPresenter) which receives an IOrderView so it can use the view.
>You wire this up in main()

Yes

>Than we have tree other classes, ShowOrderUseCase, ShowOrderRequest and Order which I assume are the model part of MVP and are in a separate class library (SalesApplication)
>ShowOrderRequest is simply some data, same for Order.

ShowOrderRequest and Order are transfer objects for use case request and response. The project doesn't show entities (business objects), which is another type of objects manipulated by the use case, about which presentation doesn't know.

>Thanks for the reply, I had a look at the update, ok so view knows the presenter.
>To be honest I don’t really get the(functional/design) difference between view & presenter, it seems presenter is only some form of relay between model & view(form).
>Why not let model talk to view directly via its interface?
>I can see from both testing point & design that we want to separate the view/form & model layer but I have to look more into this 3 way split to find its use.

Here are some of the reasons:
1. To avoid big, complicated, hard to understand classes.
2. To avoid business logic dependency on view, separate it from GUI. Then you have the possibility to treat presentation as plug-in, and you can try to use the same logic with 2 clients, for example mobile and thin client.
3. Business logic without such dependency is much more readable and testable.
4. To keep changes local. You don't need to update business logic if requirements didn't change and only presentation needs to be updated.

>Can you recommend any good books (or other  resources) regarding MVP? I can’t find any at amazon.

Unfortunately not much:
* end of this book: http://www.amazon.com/Agile-Principles-Patterns-Practices-C-ebook/dp/B0051TM4GI
* this online course: http://pluralsight.com/training/courses/TableOfContents?courseName=patterns-library&highlight=david-starr_pattern-mvp#pattern-mvp
* Martin Fowler's page (Passive View, Supervising Controller, Presentation Model instead of MVP) http://martinfowler.com/eaaDev/ModelViewPresenter.html, http://martinfowler.com/eaaDev/SupervisingPresenter.html, http://martinfowler.com/eaaDev/PassiveScreen.html, http://martinfowler.com/eaaDev/PresentationModel.html

Regards
Łukasz

[clean_coder_student]

Thanks Łukasz,

I’m about half way now in the PPP book so not sure if I can jump to the end, it seems he builds up that payroll system so I may take it 1 step at the time but a good motive ;-)
Plural I think I’ll subscribe after I come back from holidays, thanks for that link.

From what I learned so far I get the feeling (mvp) is a bit like the old (Microsoft c++ 6.0) document view architecture but now we split the document into presenter & model.
And I wonder, if you stick to clean code, DRY & the single responsibility principle you kind of naturally already end up this way (more or less).
Eg. if I brainstorm about my login sample, the next thing you probably want is a separate class that validates the password, another class that, in case of a password change, validates the password rules and one programmer simply follows single responsibility (to create these loose objects) and another may explicitly put these in a ‘model layer’ (but basically same thing).
But I don’t know mvp enough yet to see if that is true, and if MVP would be a helpful guideline/pattern…

At this point I’m not sure if I like the fact that the view knows the presenter directly. From a testing point of view it doesn’t matter, you want to test the model and that is loose from the view. You don’t want to test the view so it won’t matter if it knows the presenter. I guess it just depends if you think you want to use the view with different presenters if you can use a direct coupling or need an interface.

I found this, haven’t watched it yet so no idea of the quality (some video’s are quite good, others not so much ;-)  http://vimeo.com/10631785  (use the download button for offline watching)
Video of the lecture from CS4963, University of Utah, week 11, class covering UI Development Design Patterns. Guest lecturer Joe McBride (xamlcoder.com/blog) covers Model-View-Controller (MVC), Model-View-Presenter (MVP), and Model-View-ViewModel (MVVM).

Thanks for all Lukasz, I’m going back to the books & drawing board perhaps I’ll get back to you later but for now I have some more studying to do ;-)

Regards,

[clean_coder_student]

I run into some links/articles which may be interesting:

Martin Fowler
http://martinfowler.com/eaaDev/ModelViewPresenter.html      Retirement note for Model View Presenter Pattern
http://martinfowler.com/eaaDev/SupervisingPresenter.html     Supervising Controller
http://martinfowler.com/eaaDev/PassiveScreen.html              Passive View
http://martinfowler.com/eaaDev/uiArchs.html                        GUI Architectures


Martin Fowler refers to some article by Jeremy miller
http://codebetter.com/jeremymiller/2007/07/26/the-build-your-own-cab-series-table-of-contents/      online resource
http://www.amazon.ca/Presentation-Patterns-Designing-Maintainable-Desktop/dp/032161853X      a book (by Jeremy miller) that is supposed to be available from may 2013 but not yet… ?

later on moved to:
http://jeremydmiller.com/

[Łukasz Duda]

Thanks for the links, they look interesting.
Yeah, I think most patterns are a consequence of following fundamental rules like single responsibility, divide and win, and so on.

Good luck with books!