Skip to main content
Commonmark migration
Source Link

This code is very easy to read, and very straightforward in its implementation. Nice work! Overall, it looks really good. I would say that it does follow abstraction and encapsulation fairly well. Below are some things that I think could be improved.

Functions Should Do 1 Thing

#Functions Should Do 1 Thing YourYour createList() function does 3 possible things:

  1. Creates a new empty list with a set capacity
  2. Creates a new list with double the capacity of an existing list, then copies the existing list to it
  3. Creates a new list with half the capacity of an existing list, then copies the existing list to it

That's a little confusing. Also the name flag for the second parameter is not very helpful. If I were to keep the function as-is, I'd rename it to operation, or something like that to make it more clear. Usually a flag is a Boolean value of some sort. (It may be an int but is usually on or off.)

So I'd break createList() into 2 or 3 functions like this:

List* createList();
size_t setListSize(List* ptr, const size_t newSize);

Or you could break the second one into 2 functions, like increaseSize() and decreaseSize(), but I don't think that's necessary.

The new createList() function would just do what's inside of the if (flag == NEW_LIST) portion of your original function. For setListSize(), I'd pass in the new desired size. You can check inside the function if it's greater or less than the current size and do the appropriate thing. And the return value should be the size it was set to, which leads to my next point…

Handle Errors

#Handle Errors NoneNone of your functions handle error cases. What if you attempt to increase the size of the list and you're out of memory? What if you want to reduce the size of the list, but can't allocate the smaller-sized array before doing the copy? With your current createList() function, it will return NULL, but none of the callers check that return value, so they could crash. Even worse, you allocate a List on the first line, then assume it worked and allocate the array. Then you assume that worked and start setting or copying to it. If any one of those steps fails, the next one will probably crash it.

If you return the size from setListSize(), the caller can check to make sure it worked and react appropriately if it didn't.

Modify the Existing List

#Modify the Existing List SinceSince your InsertItem() and DeleteItem() functions modify your list, the setListSize() function should too. Rather than create a new list, it would create the new array (either larger or smaller than the current one), do the copy, then free the existing one and replace it with the new one.

This code is very easy to read, and very straightforward in its implementation. Nice work! Overall, it looks really good. I would say that it does follow abstraction and encapsulation fairly well. Below are some things that I think could be improved.

#Functions Should Do 1 Thing Your createList() function does 3 possible things:

  1. Creates a new empty list with a set capacity
  2. Creates a new list with double the capacity of an existing list, then copies the existing list to it
  3. Creates a new list with half the capacity of an existing list, then copies the existing list to it

That's a little confusing. Also the name flag for the second parameter is not very helpful. If I were to keep the function as-is, I'd rename it to operation, or something like that to make it more clear. Usually a flag is a Boolean value of some sort. (It may be an int but is usually on or off.)

So I'd break createList() into 2 or 3 functions like this:

List* createList();
size_t setListSize(List* ptr, const size_t newSize);

Or you could break the second one into 2 functions, like increaseSize() and decreaseSize(), but I don't think that's necessary.

The new createList() function would just do what's inside of the if (flag == NEW_LIST) portion of your original function. For setListSize(), I'd pass in the new desired size. You can check inside the function if it's greater or less than the current size and do the appropriate thing. And the return value should be the size it was set to, which leads to my next point…

#Handle Errors None of your functions handle error cases. What if you attempt to increase the size of the list and you're out of memory? What if you want to reduce the size of the list, but can't allocate the smaller-sized array before doing the copy? With your current createList() function, it will return NULL, but none of the callers check that return value, so they could crash. Even worse, you allocate a List on the first line, then assume it worked and allocate the array. Then you assume that worked and start setting or copying to it. If any one of those steps fails, the next one will probably crash it.

If you return the size from setListSize(), the caller can check to make sure it worked and react appropriately if it didn't.

#Modify the Existing List Since your InsertItem() and DeleteItem() functions modify your list, the setListSize() function should too. Rather than create a new list, it would create the new array (either larger or smaller than the current one), do the copy, then free the existing one and replace it with the new one.

This code is very easy to read, and very straightforward in its implementation. Nice work! Overall, it looks really good. I would say that it does follow abstraction and encapsulation fairly well. Below are some things that I think could be improved.

Functions Should Do 1 Thing

Your createList() function does 3 possible things:

  1. Creates a new empty list with a set capacity
  2. Creates a new list with double the capacity of an existing list, then copies the existing list to it
  3. Creates a new list with half the capacity of an existing list, then copies the existing list to it

That's a little confusing. Also the name flag for the second parameter is not very helpful. If I were to keep the function as-is, I'd rename it to operation, or something like that to make it more clear. Usually a flag is a Boolean value of some sort. (It may be an int but is usually on or off.)

So I'd break createList() into 2 or 3 functions like this:

List* createList();
size_t setListSize(List* ptr, const size_t newSize);

Or you could break the second one into 2 functions, like increaseSize() and decreaseSize(), but I don't think that's necessary.

The new createList() function would just do what's inside of the if (flag == NEW_LIST) portion of your original function. For setListSize(), I'd pass in the new desired size. You can check inside the function if it's greater or less than the current size and do the appropriate thing. And the return value should be the size it was set to, which leads to my next point…

Handle Errors

None of your functions handle error cases. What if you attempt to increase the size of the list and you're out of memory? What if you want to reduce the size of the list, but can't allocate the smaller-sized array before doing the copy? With your current createList() function, it will return NULL, but none of the callers check that return value, so they could crash. Even worse, you allocate a List on the first line, then assume it worked and allocate the array. Then you assume that worked and start setting or copying to it. If any one of those steps fails, the next one will probably crash it.

If you return the size from setListSize(), the caller can check to make sure it worked and react appropriately if it didn't.

Modify the Existing List

Since your InsertItem() and DeleteItem() functions modify your list, the setListSize() function should too. Rather than create a new list, it would create the new array (either larger or smaller than the current one), do the copy, then free the existing one and replace it with the new one.

Source Link
user1118321
  • 11.8k
  • 1
  • 19
  • 46

This code is very easy to read, and very straightforward in its implementation. Nice work! Overall, it looks really good. I would say that it does follow abstraction and encapsulation fairly well. Below are some things that I think could be improved.

#Functions Should Do 1 Thing Your createList() function does 3 possible things:

  1. Creates a new empty list with a set capacity
  2. Creates a new list with double the capacity of an existing list, then copies the existing list to it
  3. Creates a new list with half the capacity of an existing list, then copies the existing list to it

That's a little confusing. Also the name flag for the second parameter is not very helpful. If I were to keep the function as-is, I'd rename it to operation, or something like that to make it more clear. Usually a flag is a Boolean value of some sort. (It may be an int but is usually on or off.)

So I'd break createList() into 2 or 3 functions like this:

List* createList();
size_t setListSize(List* ptr, const size_t newSize);

Or you could break the second one into 2 functions, like increaseSize() and decreaseSize(), but I don't think that's necessary.

The new createList() function would just do what's inside of the if (flag == NEW_LIST) portion of your original function. For setListSize(), I'd pass in the new desired size. You can check inside the function if it's greater or less than the current size and do the appropriate thing. And the return value should be the size it was set to, which leads to my next point…

#Handle Errors None of your functions handle error cases. What if you attempt to increase the size of the list and you're out of memory? What if you want to reduce the size of the list, but can't allocate the smaller-sized array before doing the copy? With your current createList() function, it will return NULL, but none of the callers check that return value, so they could crash. Even worse, you allocate a List on the first line, then assume it worked and allocate the array. Then you assume that worked and start setting or copying to it. If any one of those steps fails, the next one will probably crash it.

If you return the size from setListSize(), the caller can check to make sure it worked and react appropriately if it didn't.

#Modify the Existing List Since your InsertItem() and DeleteItem() functions modify your list, the setListSize() function should too. Rather than create a new list, it would create the new array (either larger or smaller than the current one), do the copy, then free the existing one and replace it with the new one.