From mboxrd@z Thu Jan 1 00:00:00 1970 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on polar.synack.me X-Spam-Level: X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00 autolearn=ham autolearn_force=no version=3.4.4 X-Google-Language: ENGLISH,ASCII-7-bit X-Google-Thread: 103376,413164aa26adbd93 X-Google-Attributes: gid103376,public X-Google-ArrivalTime: 2002-12-02 12:56:58 PST Path: archiver1.google.com!news1.google.com!newsfeed.stanford.edu!headwall.stanford.edu!newsfeed.news2me.com!newsfeed2.earthlink.net!newsfeed.earthlink.net!stamper.news.pas.earthlink.net!newsread1.prod.itd.earthlink.net.POSTED!not-for-mail Message-ID: <3DEBC909.60901@acm.org> From: Jeffrey Carter User-Agent: Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.0.0) Gecko/20020530 X-Accept-Language: en-us, en MIME-Version: 1.0 Newsgroups: comp.lang.ada Subject: Re: Int_Stack rewritten. References: Content-Type: text/plain; charset=us-ascii; format=flowed Content-Transfer-Encoding: 7bit Date: Mon, 02 Dec 2002 20:55:53 GMT NNTP-Posting-Host: 63.184.0.78 X-Complaints-To: abuse@earthlink.net X-Trace: newsread1.prod.itd.earthlink.net 1038862553 63.184.0.78 (Mon, 02 Dec 2002 12:55:53 PST) NNTP-Posting-Date: Mon, 02 Dec 2002 12:55:53 PST Organization: EarthLink Inc. -- http://www.EarthLink.net Xref: archiver1.google.com comp.lang.ada:31358 Date: 2002-12-02T20:55:53+00:00 List-Id: Stapler wrote: > > package int_stack is > > pragma Pure(int_stack); > > Stack_Overflow : Exception; > Stack_Underflow : Exception; > > -- As far as I can tell these procedures are squared away. > -- 12/02/02 > type Int_Stack(Size : Natural) is limited private; Some vertical whitespace here would make this type declaration easier to see. Also, you might find it easier to understand things if your package and type don't have the same name. I'd suggest Stack_Info for the type, and Stack for the parameters of the type (see below for more about this). > procedure Push(The_Stack : in out Int_Stack; X : in Integer); > procedure Pop(The_Stack : in out Int_Stack; X : out Integer); > > -- These functions and procedures are new and untested. > function Is_Empty(The_Stack : in Int_Stack) return Boolean; > function Is_Full(The_Stack : in Int_Stack) return Boolean; > function Length_Of(The_Stack : in Int_Stack) return Natural; > function Sum_Of(The_Stack : in Int_Stack) return Integer; > -- I will add more functions and procedures as soon as I'm sure > -- this basic package isn't hosed. > > private > > type Integer_Array is array(Positive range <>) of Integer; > > type Int_Stack(Size : Natural) is > limited record > Elements : Integer_array(1..Size); > Top : Natural := 1; It's customary to use a Top of zero to indicate an empty stack. You might find it simplifies some things. > end record; > > > end int_stack; > > > package body int_stack is > > procedure Push(The_Stack : in out Int_Stack; X : in Integer) is > > begin > if The_Stack.Top > The_Stack.Size then > raise Stack_Overflow; > end if; > > The_Stack.Elements(The_Stack.Top) := X; > > if The_Stack.Top < The_Stack.Size then > The_Stack.Top := The_Stack.Top + 1; > end if; Because of this if, you will never raise Stack_Overflow. You will just keep replacing the top element when the stack is full. > > end Push; > > procedure Pop(The_Stack : in out Int_Stack; X : out Integer) is > > begin > If The_Stack.Top < 1 then > raise Stack_Underflow; > end if; > > X := The_Stack.Elements(The_Stack.Top); > > if The_Stack.Top > 1 then > The_Stack.Top := The_Stack.Top - 1; > end if; Similar to the Push case, this never raises an exception and continally returns the same value when the stack is empty. The value 1 you compare to here is a "magic number" and would be better replaced by an attribute. This is true in other places below, too. > > > end Pop; > > function Is_Empty(The_Stack : in Int_stack) return Boolean is > > begin > > if The_Stack.Top = The_Stack.Elements'First then > return True; > else > return False; > > end if; This is better written as return The_Stack.Top = The_Stack.Elements'First; If you use Top = 0 to indicate an empty stack, then it becomes return The_Stack.Top = Empty; Naturally, you define Empty as a constant to avoid having zero be a magic number all over the place. > > end Is_Empty; > > function Is_Full(The_Stack : in Int_Stack) return Boolean is > > begin > > if The_Stack.Top = The_Stack.Elements'Last then > > return True; > else > return False; > > end if; Ditto. > > end Is_Full; > > function Length_Of(The_Stack : in Int_Stack) return Natural is > > Length : Natural; > > begin > -- This entire function seems somewhat rendundant > -- considering the "Length" attribute. But it's > -- being included as suggested > > Length := The_Stack.Elements'Length; > > return Length; This always returns the same value. What you want to return is a value related to Top (which changes). (If you use Top = zero to indicate an empty stack, this function is simpler.) > > end Length_Of; > > function Sum_Of(The_Stack : in Int_Stack) return Integer is > > The_Sum : Integer := 0; > > begin > > for J in 1..The_Stack.Top loop > > The_Sum := The_Sum + The_Stack.Elements(J); > > end loop; > > return The_Sum; This is OK. It is, however, a special case of an iterator. It might be better to provide a generic iterator and let the client decide what he wants to do with each element in turn. Then again, it might be better to make the element type of the stack be generic and let the client decide what he wants to push onto a stack. That will be your assignment when you finish this one. > > end Sum_Of; > > end int_stack; There are results from psychology that the first few characters of an identifier are the most important in distinguishing it from other identifiers. Since so many of your identifiers begin with the same 4 characters, it makes your code more difficult to read. It would be better to leave the "The_" off of identifiers. -- Jeff Carter "Hello! Smelly English K...niggets." Monty Python & the Holy Grail