1st Dec 2021, 10:09 AM
Suraj Patil
Suraj Patil - avatar
7 odpowiedzi
+ 6
Well, since you asked so nicely, I'll give you some criticism. Don't use global variables, use a class and move all variables/functions into the class. ( or just move the variables into a struct in the case of C, and then take a pointer to that struct for each function to access the variables ) Use the same naming convention for all functions. isEmpty and push_var use camelCase and snake_case respectively. Pick one and stick with that so either is_empty and push_var or isEmpty and pushVar. if (top == -1) { return true; } else { return false; } Use 'return top == -1;' instead. It's much shorter and more readable. In functions like push_var, pop_var, peek and display, they don't return after the isEmpty check so you run the chance of accessing the array at an index of -1 which is undefined behavior. Either return or throw something. ( and then remove the cout and handle that elsewhere with the catch ) You're shadowing the 'size' variable in main. The global size just happens to be initialized to 0 because it's global so isFull doesn't function properly. Don't use #define to define a constant variable ( that's C ), use constexpr in modern C++. Or a const variable if that is not available. Lastly, try not to use 'using namespace std;' in real code. It's fine for practice though. In this case you're getting an error because std::size exists.
1st Dec 2021, 11:19 AM
Dennis
Dennis - avatar
+ 3
This place is not for advertising your code, it's only for programming related discussion. Have a look at the forum guidelines https://www.sololearn.com/discuss/1316935/?ref=app
1st Dec 2021, 10:40 AM
Rishi
Rishi - avatar
+ 1
Implementation of stack using linked list https://code.sololearn.com/cl9wCnD8NKms/?ref=app
3rd Dec 2021, 7:55 AM
Suraj Patil
Suraj Patil - avatar
+ 1
That looks much better. However there are still some issues. The big ones are the global variables. You really don't want them or need them. I'm assuming C here instead of real C++. You can remove the #define size 100 because you're not using it anyway. Use 'const int Size = 100;' instead if you do need it ( note the capitalization, it's a good idea for constants ) if you really want to use a #define make it all caps like '#define SIZE 100' to make it obvious that you're using a macro instead of, say, a variable. int value; can be moved into main, it has no reason to be global. Currently, your functions only operate on 1 instance of struct node. What if you needed more than 1 stack? Your code doesn't support that. Move struct node* top = NULL; to main, then modify the functions to take a pointer to the node as the first argument. Take double pointers whenever you need to modify the actual pointer itself in a function. ( whenever you need to assign something to top itself ) void push( int value ) becomes void push( struct node** top, int value ) void pop() becomes void pop( struct node** top ) but something like int peek() becomes int peek( struct node* top ) Then add a pointer to whatever used top before in the functions that now take a double pointer. For example 'newnode->link = top;' becomes 'newnode->link = *top;' Then call those functions with push( &top, value ); ( add & for the functions that require a double pointer ) and peek( top ); Another issue is that you're code will crash if the user enters a 3. That's because peek doesn't return or throw if top == NULL. Resulting in a NULL dereference in the line after. The rest looks fine to me. :)
3rd Dec 2021, 8:29 AM
Dennis
Dennis - avatar
+ 1
Dennis It works fine
3rd Dec 2021, 8:36 AM
Suraj Patil
Suraj Patil - avatar
+ 1
Yes, it does work, and good work with that. I'm merely providing information on how you could improve it for next time because code can always be improved.
3rd Dec 2021, 8:39 AM
Dennis
Dennis - avatar
0
Dennis Thank you for suggestions i will improve code
3rd Dec 2021, 8:41 AM
Suraj Patil
Suraj Patil - avatar