FeedTheCube / CogDoc

0 stars 0 forks source link

Class Error Handling #7

Open hyndgrinder opened 6 years ago

hyndgrinder commented 6 years ago

@SinclairC I haven't provided any error handling when building the classes. I will probably need you to take a crack at a class like Query which we may want to provide some break-points for. If you could at least provide me an example of how/why we'd do things like try/catch and how we gracefully capture the errors to report to the user. It would be especially useful if we could do this as or follow-up with a working session to help me understand. In this sample, the Query may or may not have a child called detailedFilters. If it doesn't, perhaps we could provide some sort of feedback to the user stating as much.

SinclairC commented 6 years ago

I'm thinking it might make sense to restructure the classes a bit before moving further. I'd like to know your reasoning behind some things.

For example, you've got your DetailFilter class, which is being used as both a static class and as an object. When you use DetailFilter.getDetailFilters(), you're using the class to make objects based on itself which also contain the method being used. It's kind of a circular setup, and there doesn't seem to be any reason for a bunch of objects to contain a method that is only ever used in a static way. I could be wrong, though. Is there a reason for that? I don't know how my experience level with Python compares to yours. Maybe this is just how things are done.

In any case, have you considered putting the static methods into a more central location and just using the classes for class-related variables and methods? What I mean is that maybe something like DataItem doesn't need to do anything, just store values and be passed around. I would save class methods for things that manipulate the object's variables or pass things to/from the object itself. What are your thoughts?

hyndgrinder commented 6 years ago

These objects will have different actions at some point.

For instance, this sample is only one report, and the user may be trying to document only report, but it's more likely that this gets run against the entire content store, which will have many reports. With that, I may be want to categorize my classes in any number ways (e.g. All Queries in this report, All Queries in the content store, All DataItems in this report by Query, All DataItems in this report, All DataItems in the content store). With that, I felt it was better to objectify the elements, and preserve the original xml so it doesn't have to be remanufactured in any way.

Also, by preserving the xml element we can wrap that elements (possibly many elements like dataItems, or filters selected from a checklist) in a parent element called an RSClipboardItem. That way the Documenter can also be a librarian for the entire Cognos solution. If I can go to a library of report objects and copy them into my report at will, I can save myself a fair deal of hassle if those things I want span multiple queries or reports. There are some rules that come with pasting into a report, though, so it's a set of behaviors we'll have to manage... I forgot to get a snippet of the wrapper, but will do so next week.

There may be more things we can do with the classified objects downstream. I don't intend to classify everything, but I do intend to classify the major things that are worth copy/pasting from report to report.

You'll see that I have created a few enhancements for the classes that I already know I want to associate behaviors with.

Is this the right approach?

SinclairC commented 6 years ago

I agree that it makes sense to have objects for each important element (Queries, DataItems, etc.).

I would organize it something like this:

-class Query (containing constructor and some get methods for checking the query's relationship to other items) -class DataItem (containing constructor, get/set methods and maybe something for serializing data) -class Util (used as a static class containing multi-use methods for grabbing/creating collections of the above objects)

So something like getDataItems() would become Util.getDataItems() instead. The classes other than Util would only contain methods that manipulate their own data or data that instantiated objects receive.

By doing it this way, we limit the chances of problems occurring across multiple files and we don't have to duplicate code as much (ex. if two "getWhateverItems()" type functions are basically the same, we could potentially make a single more generic one). We also avoid having a ton of objects using memory to store functions they will never use, while still retaining the same sort of hierarchical structure of the XML. If you need to organize objects in various ways, that's still possible this way.

On an unrelated note, do you know if you need namespaces for anything in this situation? If they're just adding extra complexity to the code and don't have any benefit, we could strip the namespace from the original XML string and avoid having to deal with it every time we grab info from the element tree.

SinclairC commented 6 years ago

Oh yeah. One other question: Why are the classes in different folders? Is that a thing that Visual Studio is doing or is it just to keep the XML-style hierarchy intact? I can see the thought behind it, in that it shows that a DataItem will be accessed through a Query, for example, but it also seems to imply that DataItem inherits from Query (since it's a sub-folder). How do you feel about a single Classes folder?

hyndgrinder commented 6 years ago

-class Query (containing constructor and some get methods for checking the query's relationship to other items) -class DataItem (containing constructor, get/set methods and maybe something for serializing data) -class Util (used as a static class containing multi-use methods for grabbing/creating collections of the above objects)

So something like getDataItems() would become Util.getDataItems() instead. The classes other than Util would only contain methods that manipulate their own data or data that instantiated objects receive.

Makes sense to me.

On an unrelated note, do you know if you need namespaces for anything in this situation? If they're just adding extra complexity to the code and don't have any benefit, we could strip the namespace from the original XML string and avoid having to deal with it every time we grab info from the element tree.

They're just a nuisance, basically. Though we will have to account for them changing between Cognos Environments. The namespaces reflect in some way the version of Cognos which produced them. Very little in the spec seems to change from version to version, however.

SinclairC commented 6 years ago

I've pushed a branch to the repo that you can check out (see SincTests). This one has Util implemented with some minor tweaks.

hyndgrinder commented 6 years ago

Oh yeah. One other question: Why are the classes in different folders? Is that a thing that Visual Studio is doing or is it just to keep the XML-style hierarchy intact? I can see the thought behind it, in that it shows that a DataItem will be accessed through a Query, for example, but it also seems to imply that DataItem inherits from Query (since it's a sub-folder). How do you feel about a single Classes folder?

Yes, I think a single classes package is a better idea. Refactoring the nesting of packages seems painful this way. Not that I did myself any favours by starting in the middle of the hierarchy, but there are objects near the top node that I don't fully understand, so I was just trying to stick with what I know.

And besides, while these object are heirarchical in nature, the etree let's us parse them with any variant of ancestor, so, enforcing it in packages seems right at first, but in the end is more trouble than it's worth.

hyndgrinder commented 6 years ago

Ok, so Issue #10 will carry the debate regarding the unification of classes and the use of the Util class. This thread will go back to the discussion on Class Error Handling

SinclairC commented 6 years ago

I'm thinking this issue can be delayed until we have more code in place. The thing about try/catch blocks is that you don't necessarily want to use them if you don't absolutely have to. During development, especially, it seems to make sense to let exceptions run their course and print out a stack trace. We want to see what problems come up.

As some people say, "Exceptions are for exceptional things." I don't think we need try/catch for most of what we're doing so far, since the worst we'll run into is a missing XML file or value, but maybe we'll need them in the future.

hyndgrinder commented 6 years ago

Perfect. Ok, I will leave the task, maybe make a new column for "parked" items, perhaps one for "pinned" items too.

The reason I don't want to close it, is it's still a legitimate task.

SinclairC commented 6 years ago

Agreed