![]() |
|
#1
|
||||
|
||||
clean up suggestionsHey everyone. I've created a program that takes an equation in infix notation and converts it to postfix. The program is done and works fine. The driver for the program is a bit bloated. I'm going to be presenting this code to a group of people who do not know the Shunting Yard Algorithm (that is the algorithm used to convert). With my comments as they are, do you feel that the code is readable? Does anyone have any suggestions on how to clean it up some? Thanks for all you help
CPP / C++ / C Code:
__________________
"To argue with a person who has renounced the use of reason is like administering medicine to the dead." -Thomas Paine www.sullivan-county.com/deism.htm |
||||
|
#2
|
|||
|
|||
Re: clean up suggestionsQuote:
I have a few comments. Point: We don't know your class definition or implementation for PostFix, we can only derive some operations from it based on those used inside of "main." Since the initialization of pClass (bad variable name) in main takes no arguments, we can only assume that the PostFix default ctor is doing some kind of magic that puts the object into a usable state (acceptable), but what or how that state is meaningful is completely hidden from us. This is generally considered to be a good thing, as the general idea is to "hide" the implementation from the user such that the inner-workings of the class are unimportant except as a result of the published interface contract. Since we can't see that contract (it isn't published, only used in your "driver" code), we can't tell if your interfaces "make sense" or if they are being used in a manner that suggests quality class design. We can assume that both are being done properly and move on. ...but then we come to: pClass.DisplayOpening(); A no-args "display" operation seems like a rather poor design choice. One would think that at a minimum a streams-based object would be required. However, I strongly suspect that inside of pClass: Such an implementation tightly binds the display method with the functional component of whatever work the PostFix class does internally. So, we go a bit further into the code: pClass.GetSize("input") ...okay, so it probably returns an int, but how is that relative to the string constant input? We do finally see that at least one operation exposed by the PostFix class takes at least one argument. tempS = pClass.LookValue("input"); //Look at the value on the queue For whatever reason, we've needed to give the PostFix class instance with the string constant twice, where one would think that perhaps we would simply be able to "seed" it with "input" if somehow "input" was meaningful to it through the use of a single operation taking the string literal. Of course, there is no indication of why the word "input" is meaningful to the object, so a comment might be useful, especially since the class operations involving the string constant do not seem to describe WHY or WHAT is being done in relationship to the value passed into them. I mean, does GetSize return the length of the word "input" in number of chars? Okay, so who cares, let's keep thinking about what we're doing: pClass.MoveList("input", "final"); ...so are we supposed to believe that input and final string constants are someway of configuring an internal list member of the PostFix class? Is the operation putting "input" at the "final" position in the "list" being "moved?" Of course, we already know that "input" is the name of a queue used by the PostFix class, right? ...because: tempS = pClass.LookValue("input"); //Look at the value on the queue ...popped it off of the queue? or just peeked at it? Okay, so here is what I see happening. You're using string literals and inside of PostFix you're comparing string values to decide what to do in LookValue and other operations. This is usually a relatively poor implementation choice. We usually use constants, but they tend to be integral values rather than string literals. Here is a hypothetical example: PostFix::QueueType::Input ...then we'd expect to see usage such as: tempS = pClass.PeekValue(PostFix::QueueType::Input); ...or: tempS = pClass.PopValue(PostFix::QueueType::Input); (Some might prefer dequeue to pop.) ...and that might lead to: pClass.GetSize(PostFix::QueueType::Operator) You may see how this use of constants give us a better understanding of what we're doing and therefore how to properly use the class from the arguments used in the operations. I'm guessing that pClass.GetEquation(); queried the user for some input and therefore setup the input queue so that it has something other than a zero size. If so, then this operation again tightly couples the class design to the input method(s). Better is probably a "SetEquation" such that the instance is initialized with an equation and a GetEquation that returns the value of the equation as seeded by the set operation. This decouples the class from the I/O subsystem, since we can probably accurately assume that there are at least one instance of std::cin inside of your GetEquation operation. Therefore, the "driver" would be responsible for getting user input (or input from whatever other source) and simply "provide" it to the instance through the exposed setter interface. CPP / C++ / C Code:
...okay, in this little block of code, we can see how useful it would be to have the arguments as integral types rather than as string literals. We could simply blindly add all of the operators to the container and then sort the container to get ascending/descending results. Here we are spending effort in managing the innerworkings of the PostFix instance from outside of it. That is generally a very poor design decision, because our interfaces are basically portals to our internal data containers such that the usage of the interfaces is directly tied to the type of container used. While not terribly obvious, it says that for whatever internal container we've selected, that those operations will need to manage it whether or not we learn of a more efficient way of doing things or not. Also, in this case, if we're going to be processing operators based on precedence, and we know that we'll always use the highest order precendence operator to the lowest order operator, then why should we need to manage the order outside of the instance? Shouldn't the design of the class just know how to organize the operators in its internal data structures based on the input "equation" that we seeded it with? CPP / C++ / C Code:
...any time that you see the same operation, it may suggest to you that it may be a candidate for a do...while. Here you've "primed" the tempI variable with a read and then you read it again at the end of your while such as possibly this? CPP / C++ / C Code:
You seem to use an abundance of whitespace until you get to: CPP / C++ / C Code:
...is there some reason that you now feel compelled to jam everything so tightly together? I don't know that these are what you're asking for in terms of your initial request, but I hope that they will offer some value to you. :davis: |
|
#3
|
||||
|
||||
Re: clean up suggestionsThanks for taking the time to read through my code. I understand most of what you said. I'm sorry I should have mentioned the thought process behind my PostFix class. I developed a linked list class, a stack class, and a queue class. I then used the PostFix class to combine them all together to use with the shunting yard program. Here is the PostFix header file:
CPP / C++ / C Code:
couple of questions: 1)Is pClass a bad name because it contains the word Class? 2)Why is a no-args display function bad? I've never been told to pass a stream object to a display function before. 3)Is it my understanding that it would be better to have the driver get the equation, and then have the PostFix class do all the operations that are in my current driver? Once again, thanks for the suggestions (don't know why I didn't think of the do...while loops) __________________
"To argue with a person who has renounced the use of reason is like administering medicine to the dead." -Thomas Paine www.sullivan-county.com/deism.htm |
|
#4
|
||||
|
||||
Re: clean up suggestionsQuote:
How is pClass descriptive? Good variable names should be descriptive. Also, since it is NOT a pointer to a "class," (object) or even a pointer type, it is going to be confusing to some who would find a leading lowercase "p" to mean pointer. Also, since using "pClass" suggests "pointer to a class" you can't even have a pointer to a "class." We have pointers to objects (or POD). Quote:
It isn't so much that a no-args function of any kind is bad, rather that here you are saying that your class design is going to spew output to the console. It will do that and nothing else. However, there is nothing intrinsicly "console-oriented" about what your class is supposed to accomplish. In other words, it is not intended to be some kind of tool for formatting output to the console or interacting with the console in a meaningful way other than its limited I/O usage for very basic data and display needs. What this tells me is that it should have NO interaction with the console. Rather, the "driver" should I/O with the user in whatever way is appropriate (whether console, GUI or via mind-meld) and interact with the instantiation via its interfaces for data manipulation. Let's say that we were going to write a very simple function that returns the sum of two supplied integers. CPP / C++ / C Code:
...here is your implementation with no args: CPP / C++ / C Code:
...while your "display" function returns nothing, my point should be clear. What should "add" know about console I/O? How does console I/O "help" it add two integers together? Why should it ever be involved with getting data when we can so easily pass it all of the data that it needs in the form of arguments? Not only that, but now we've bound it inextricably to asking (or in your case, telling) the user something in a very static kind of way. At least if we were passing in a stream interface, we could overload the type and get the data I/Oed however we choose, but here, we can do nothing but wait for the user to input something meaningful. In the case of the return type shown in the no-args add, I've also "decided" that I will silently blow-up if the user inputs junk that doesn't convert to integer on stdin: CPP / C++ / C Code:
Output: Code:
I'm willing to bet that something similar is possible with your GetEquation operation, though it is not my intent to point-the-finger or be negative, when the lines of responsibility are crossed, all bets are off even though they need not necessarily be so (2nd "add" could have handled errorneous input), but they often tend to be so whenever you see it in practice. We can see how the first "add" function promises us that it will return an int if supplied two ints. This is called a contract between us (the user of the operation/object's public interface) and the interface. The 2nd add function tells us that it will return an int to us with nothing supplied by us, yet it does not always live up to its promises, does it? Imagine a situation where the first add function could take "adfk" as either of the required integers used in the input? We'd get a compilation error, huh? Let's look at your header comment: CPP / C++ / C Code:
The "class definition" has nothing to do with "what the program does." The class definition is not part of any program. The class definition defines the class operations and attributes. It is and should be a behavoiral specification for the more important question, the answer to which is the implementation of the class. What is a PostFix? The public interfaces answer the question, How do I use it? Naturally, nobody using a PostFix instance need call DisplayOpening. But if it is useful only to a single program type, then it probably doesn't belong in this definition because there is really no question answered by DisplayOpening that asks "How is this a part of a PostFix or what it is supposed to do?" You will want to be very conscientious of "blending" or "crossing" the roles of responsibilities in your class definitions and program designs. Try to ask yourself, what does a PostFix need to make it a PostFix and leave everything else out of it. What the "user" needs to know in order to use it should be very clear from the public interfaces exposed by the class definition. If you were to consider a "help" interface, this would probably be better handled by an external class definition that is used by your other classes to put forth meaningful details about it in a common manner. This is often implemented using interface inheritance or by aggregation of a type that "knows" how to interact with the various possible stream types. Quote:
The first "add" function is probably the preferred choice for "doing the work" desired by the name, right? We separate the roles and responsibilities of the design into "segments" that make sense for the kind of functionality encapsulated in our class design. What should PostFix know about the display? Should it know that we're a GUI or console based app? Probably not. Like that "add" function, it should probably just be happy to take data in it operation arguments and return data as part of its operation return types. Not only that, but what happens when we suddenly see another situation where an instance of PostFix could be useful? ...will we want it to respond to user I/O? In the first "add" function, I can happily add it to a library and use it wherever I could need that functionality. In the 2nd one, I'm literally limited to what it can and does do for me based on how it does it. In C++ we want to hide the "how" behind the interface and just focus on what it does for me. That being said, an operation that takes no data (arguments) and returns nothing is very likely not very useful. Quote:
Remember that do/while MAY NOT be what you want or need. It will always execute the block at least once. Your original code MAY be just what you need. What I was trying to say is to consider a do/while whenever you see that you are calling an operation once and then again in a loop. You may be able to restate your conditional such that the do/while works and saves code and possibly execution cycles. :davis: |
|
#5
|
||||
|
||||
Re: clean up suggestionsOnce again, thanks for the input. I've got some food for thought now. I wish they would teach us stuff like this in school. What you've said makes perfect sence once I think about it, but it stuff they never mention in school (or really inforce for that matter).
__________________
"To argue with a person who has renounced the use of reason is like administering medicine to the dead." -Thomas Paine www.sullivan-county.com/deism.htm |
Recent GIDBlog
Once again, no time for hobbies by crystalattice
| Thread Tools | Search this Thread |
| Rate This Thread | |
|
|
Similar Threads
|
||||
| Thread | Thread Starter | Forum | Replies | Last Post |
| Need suggestions on PHP/MySQL directory scripts | Div | MySQL / PHP Forum | 0 | 02-Oct-2004 14:21 |
| Namecounty suggestions? | Waterlogged | Websites Reviewed Forum | 3 | 20-Sep-2004 06:44 |
| Link to our site suggestions / ideas | jrobbio | Advertising & Affiliates Forum | 2 | 02-Apr-2003 03:37 |
Network Sites: GIDNetwork · GIDWebHosts · GIDSearch · Learning Journal by J de Silva, The