Revisit code structure

[mkroetzsch]

@arsylum @guenthermi Now that we have split the code into smaller files, it would be good to revisit the structure and rethink some of the choices. Things I noticed:

1. The distribution of code across modules app, util, and query is rather arbitrary. Is there any reason to have more than one module in the first place? Do modules define the directory structure or are the two unrelated?
2. The controllers for each view are currently not easy to distinguish from other code. It would maybe be good to adopt some naming convention for the source files of controllers.
3. The file src/js/app/app.js contains code that is specific to the browse view. It seems this should be moved somewhere else.

[arsylum]

1. Most angular related docs preach modularity. Hence it may be even preferable to also create modules browse and view for example. I just thought that'd take it to far in the first step. Modules are not linked to directory structure (it's more a convention).
2. That's right and also currently goes against the recommendation of "one component per file". My reasoning here was that when a view logic basically consists of a service and a controller (like in view.js or query.js) they are closely related and fit together in one file. Hence I also gave up the fooController naming convention since it's not just the controller after all. Seperating further down to fooController.js and fooFactory.js (or fooService.js) etc. is of course a valid option and especially sensible when those files grow further.
3. You mean the Arguments factory? I first moved that into browse.js but then noticed it has other concerns as well (not sure if I understood that right though). So essentially I just didn't know what to do with it for the moment.

[mkroetzsch]

1. I am agnostic here. I don't understand what difference this makes anyway. When we load code, we mainly address it by its -- what is this anyway? -- class? factory?, such as for example "Classes" or "i18n". This is also how code is distributed in files now. What changes if we have one module for each of these?
2. Keeping factory and controller in one file seems fine for now, but it would still make sense to name the files so that one can tell where the controller for a given view is.
3. Yes, I mean the Arguments. Maybe @guenthermi can help here.
4. Should we use upper case or lower case in factory arguments ("Classes" vs. "sparql")? It would be nice to have this uniform as well.

[arsylum]

1. I'm not better off unfortunately. I've learned angular during this project and while I started to note a few differences from the usually advertised best practices I cannot say that I can see all the pros and cons clearly. I imagine the main benefit of strong modularity having a network of parts with clearly defined responsibilities which also are pluggable (e.g. making it possible to ship a lightweight version with reduced feature set) other benefits are in testability, but since we don't do unit test that's not really relevant [yet?]. In angular there is basically two concepts of linking parts together. A module defines it's dependencies which have to be modules themselves (such as ngAnimate, pascalprecht.translate or utilities). Those modules' components can be injected into our modules own components (such as foo.factory(['someServiceFromUtilities', function($sSFU) {}])) That reveals another flaw in the current implementation: Both queryInterface and utilities modules define no dependencies but inject components from various modules into their own components. I assume this only works because they are themselves dependencies of classBrowserApp.
2. I thought that something like browse.js would be the obvious place to look for a controller that is in charge of the page at location #/browse. But it's obviously a biased assumption.
4. Since all controllers currently use UpperCamelCaseConvention I'd propose to have lowerCamelCaseServices for easy distinction.

I explored a further structure draft embracing strong separation which I would like to put up for discussion. It might be pushing it and still has some gaps, but should we generally want to move more into this direction, might as well do it now while we're at it.

[guenthermi]

1. When I created the arguments factory, I thought that it is useful to catch the query string parameter and manage the configuration associated with it in one place. But as it seems the only point where it make sense to share configuration is the lang parameter which is stored in the i18n module. So probably it makes more sense to put arguments in the browse.js file.
2. In the most angularjs style guidelines controllers are named in upper case and factories in lower case. https://github.com/mgechev/angularjs-style-guide https://github.com/johnpapa/angular-styleguide/blob/master/a1/README.md
3. I like the draft, but I would rather separate the actual pages with there related controllers and services, other modules which contain shared code and html templates. I am not sure if it make sense to create separate modules for the different views, because if there is code that is useful to include somewhere else it should be in a separate module and making lightweight versions without one of the view does not really make sense. If there is something like an extension it could be separated as well. Furthermore I would not name the views .view.html cause pagename.html is expressive enough.

[arsylum]

(3) I'm not sure if I understand you correctly. But I didn't think separate modules for different views but for different features / functionality. E.g. displaying information about a class or property is a feature. Since the view.html happens to be part of that feature it seemed sensible to place it next to the code of that module (while sqidCompile and sqidStatementTable directives remained part of util in the draft, though they are only used in #/view as of now but might get reused at some point?)

And on dropping the .view. naming - I just experimented here and basically wanted to make a differentiation from .tpl.html , but I fully agree that we can drop it. Not least to avoid atrocities like view.view.html...

[guenthermi]

At the moment we have three big controllers for view, browse and query which have a strong relatedness to there respective page. Probably it would be good to put them in one module, because they are quite specific for our application. So what I would separate from the other modules is core, browse, view and query.

[arsylum]

I disagree. What about the StatController for example? Sure it's not a big controller but it's also has strong relatedness to its respective page. But than which criteria exactly make a controller eligible to be part of the 'big controllers'? And I would dare to claim that currently most of our applications code is quite specific to the application.

Also if we pack a lot of logic into one main module we will run into similar circularity problems as we have now. Example: classBrowserApp generally depends on utilities while utilities depends on classBrowserApp for Classes and Properties. Like mentioned earlier it actually somehow works out, but it's a bit dirty and a recipe for future headache.

[guenthermi]

Sorry for the late response. Even in your draft the pages are part of a folder and every folder has exactly one module definition thus you have to decide to put the controller into this module or not. But you convinced me that it is better to have more smaller modules.

[arsylum]

what began as an experiment to create an autonomous i18n module led to go about implementing this, only to realize it's a massive effort. So far it seems like it can work out, but it starts to feel quite a bit over engineered. Progress is in the branch refactwo but still a big mess, with most features in a yet non-working state.

[mkroetzsch]

There does not seem to be so much i18n code in the (huge) pile of changes in refactwo. Maybe I am not seeing the right parts. Can you explain what your goal is and why this is more effort than expected? We should be careful not to spend too much time on this, considering that i18n is basically working very well in the current structure, with the only small glitch that not all views are setting the language from the URL.

[arsylum]

But views aren't supposed to set languages from URLs! (imho, sorry) They should merely adhere one governing language service. Sharing this responsibility over all controllers was a bad design choice that makes it about impossible to evade such 'glitches'. Like up until recently the browse view would store it's language setting in it's own arguments service to the effect that the UI language would switch back and forth when navigating between different views under certain conditions. It's also undesirable that the view controller always resets to 'en' in absence of an url lang parameter.

The i18n module got severely sidetracked by all the other changes - my goal is actually to create structure for things like this. Why couldn't this go into good old classBrowserApp? It could easily. I just thought having it in a single place would be more structured than having it spread across that and utilities. Speaking of which, is there a reason why the i18n needs to manage it's own language setting and is not interested in how things were set up with the $translateProvider? Was it designed the other way around? Or because we can retrieve labels in many more languages than we have translations of the UI? I don't think it should redefine the already specified default language in any case.

The refactwo branch now seems to be back at fully functional from what I can tell. Some cleanup left to do but at least it lacks the inexplicable issues I couldn't resolve in the first refactoring. I think it already brings many more benefits than just an lang param url watcher but my judgment is at best clouded after moving around this amount of code. You might as well place a veto if you think the pile of changes is actually too huge (also since I skipped the detailed documentation of changes this time). Otherwise I'd like to merge in the latest changes to the browse view and maybe revisit the i18n service before making a pull request. (I wont bother changing anything about the services API though, since reworking through all existing i18n'ed code doesn't sound fun)

Sorry for the sidetracking (Hello #65) and rant. This whole refactoring thing turned into kind of an ordering frenzy for me and I'm probably just getting concerned whether it's still justified what I do. Once this is over I'll happily return to feature work.

^{One last thing: It just occured to me that the proper name for the i18n module should rather be l10n, since that is what it does. i18n is more like the usage of it. But I don't think anyone should bother to care}

[mkroetzsch]

I think the discussion of refactoring file locations and the rewriting of i18n should be in different pull requests. How deeply entangled is this now? It would be useful to merge the huge file changes in this branch as early as possible since they will otherwise create a lot of merge conflicts. Then one could redesign i18n (or l10n or whatever) without that hurry.

Re: Why do we store our own language rather than using the one set in angular translate? Because we need to distinguish the case "no language set, defaulting to English" from the case "language set to English by the user", and I did not know if/how this could be done. Having more languages in labels than in message files should not be a problem (angular translate falls back to whatever is configured in this case). A weaker reason is that the i18n service should be notified of language changes (to invalidate the caches), and it was easy to implement it so that it would also notify angular translate -- the opposite (making angular translate notify our code) does not seem so obvious, but maybe this is solved by how things work now (I guess some code now does the setting from the URL, so this code could notify all components).

Re: cost vs. benefit. Design principles are just guidelines that aim at lowering future maintenance cost and/or reducing programming errors. There are many ways to achieve these goals, so one has to consider how much time is spend in each case. Especially complex solutions (you mentioned "over-engineered") tend to increase maintenance costs rather than reducing them. It helps to realise that software projects have a limited life time and that only a few things are being done within this time at all. In fact, it helps even more to realise that the same is true for humans. At least this makes me reconsider before I go on a refactoring spree these days. ;-)

Anyway, if we have a fully functional, future-proof localisation system ready now, this should be a good thing. However, I would like to understand how it works (also for my own future maintenance work). I don't know if it is realistic to separate this from the refactoring merge request. If not, can you at least give me some hints which files to look at and how information flows now?

[arsylum]

See #65 for the i18n part. In hindsight I agree that work on the i18n should be in a separate pull request. However I would still like to at least commit some cleanup of the ?lang param handling that is now done by the i18n module. I think I just need to remove it from the view and browse controllers but I'm always slightly hesitant to fiddle around in code that is not my own without full understanding.

As for over engineering: I mostly meant that by combining angular with requirejs we now have a twofold dependency system which gives room for confusion. On the lower level we have the requirejs modules which are basically javascript files that use a define() wrapping call. Those are not to be confused with angular modules which are defined by angular.module(). requirejs modules can depend on other requirejs modules (require js basically takes care of the right loading and execution order) and angular modules can depend on other angular modules. The basic structure of our javascript files now is:

define([            // wrapping requirejs module define call 

    'foo/bar',      // that starts with an array listing dependencies in the form of
    'boo/far.fur',  // for 'boo/far.fur.js' file (just trim the .js ending) or
    'somelibrary'   // an identifier defined in the "paths" section of main.js

], function() {     // and then the module execution function

angular.module('i18n').component('name',['dep', function(dep) {
    // component logic

}]); // closing component() call

}); // closing define() call

A further error hazard is that this dependency management is non strict in a sense. If you forget to list a module that you need, chances are it will work anyway because, simply put, a lot of things depend on a lot of things so it might already be available. That's the kind of situation where in the future seemingly unrelated changes could suddenly take down other parts of code. But we already had that situation actually.

But that aside I hope that the improvements I can see in this are not only a result of my subjectivity. That modules now actually can be switched out off and on without breaking the overall application is a feat of maintainability. And while the explicit listings of statements require some extra attention they also are somewhat implicitly documenting, like a summary of what is using what.

To sum it up, this is roughly the loading chain when starting the application: index.html loads requirejs.js -> requirejs.js loads main.js -> main.js loads squid.module.js -> squid.module.js draws in its dependencies ['layout/layout.directives', core/core.config', 'i18n/i18n.config', 'meta/meta.config', 'browse/browse.config', 'view/view.config', 'query/sparqly.config'] (they draw in subsequent deps and so on...) and then once squid.module.js is loaded main.js calls angular.bootstrap( document, ['sqid'], { strictDi: true } ) starting the angular magic to happen.

Or a bit shorter - everything is loaded, then the application is started ;-)

[mkroetzsch]

Ok, thanks for the explanation. I was confused by the dependency management, so this helps a lot. The long chain of loading is eliminated when using the minified version, right, so there is no such overhead there?

I am still a bit puzzled regarding when we can merge the huge refactortwo branch. It seems that this is blocking other developments since we cannot hope to merge changes easily into this completely different file structure. What's needed there now? How much work remains? Can we maybe have a partial i18n rewrite for now, merge the big refactoring, and continue i18n in a new branch?

[arsylum]

Yes I'm also concerned about blocking progress. Luckily it is actually ready as far as I can tell. I just want to merge/resolve the latest changes to the browse view into the refactwo branch so it can be ff without conflict. I think I will have this done and create the pull request by the end of the day.

[arsylum]

Merged in the changes so we can finally work on without causing all those conflicts.

If someone should yet run into a serious issue with this, report ASAP. Once we have shoveled a number of commits on top of this, a rollback would be overly painful. The way it looks to me we should be fine though.

I propose to still leave this issue open for that matter and close it by the end of the week.

[mkroetzsch]

I propose to still leave this issue open for that matter and close it by the end of the week.

Agreed. I have now updated the online version to also use the current master code.