Return to Answer

replaced http://programmers.stackexchange.com/ with https://softwareengineering.stackexchange.com/

edited Apr 12, 2017 at 7:31

Overall this looks very nice. It's easy to read, and is very refined. A few notes:

You can remove the Map from the first line of your structure.

 typedef struct
 {
 Color *blocks;
 int rows;
 int columns;
 } Map;

Your function map_delete() only performs one action, freeing the memory of map. Unless you plan to expand on the map structure where you will have to free more members within the structure itself, I would get rid of it and just call free(map). If you have compiler optimizations on, I guarantee that your compiler is already doing that for you anyways, so it should slightly decrease compilation time.
You can simpilfy your NULL tests.
```
 if(!map)
```
This is more of a style issue, but since this recently led to the Apple goto security flaw, I'm going to cover it. I don't think you are writing your brace-free single statement loops and test conditionals correctly. This is completely up to you to decide since this is a style issue this is a style issue, but I prefer to do it like this:
```
 if(!map) return NULL;
```
Other's may be more strict and tell you to use braces, but it's up to you.
You need more comments. There are some lines in your code that I have trouble following. And if I have trouble following it, chances are that you will have trouble following it as well when you re-visit this project in a few months. Save yourself (and possibly others) the trouble and document your code.

Overall this looks very nice. It's easy to read, and is very refined. A few notes:

You can remove the Map from the first line of your structure.

 typedef struct
 {
 Color *blocks;
 int rows;
 int columns;
 } Map;

Your function map_delete() only performs one action, freeing the memory of map. Unless you plan to expand on the map structure where you will have to free more members within the structure itself, I would get rid of it and just call free(map). If you have compiler optimizations on, I guarantee that your compiler is already doing that for you anyways, so it should slightly decrease compilation time.
You can simpilfy your NULL tests.
```
 if(!map)
```
This is more of a style issue, but since this recently led to the Apple goto security flaw, I'm going to cover it. I don't think you are writing your brace-free single statement loops and test conditionals correctly. This is completely up to you to decide since this is a style issue, but I prefer to do it like this:
```
 if(!map) return NULL;
```
Other's may be more strict and tell you to use braces, but it's up to you.
You need more comments. There are some lines in your code that I have trouble following. And if I have trouble following it, chances are that you will have trouble following it as well when you re-visit this project in a few months. Save yourself (and possibly others) the trouble and document your code.

Overall this looks very nice. It's easy to read, and is very refined. A few notes:

You can remove the Map from the first line of your structure.

 typedef struct
 {
 Color *blocks;
 int rows;
 int columns;
 } Map;

Your function map_delete() only performs one action, freeing the memory of map. Unless you plan to expand on the map structure where you will have to free more members within the structure itself, I would get rid of it and just call free(map). If you have compiler optimizations on, I guarantee that your compiler is already doing that for you anyways, so it should slightly decrease compilation time.
You can simpilfy your NULL tests.
```
 if(!map)
```
This is more of a style issue, but since this recently led to the Apple goto security flaw, I'm going to cover it. I don't think you are writing your brace-free single statement loops and test conditionals correctly. This is completely up to you to decide since this is a style issue, but I prefer to do it like this:
```
 if(!map) return NULL;
```
Other's may be more strict and tell you to use braces, but it's up to you.
You need more comments. There are some lines in your code that I have trouble following. And if I have trouble following it, chances are that you will have trouble following it as well when you re-visit this project in a few months. Save yourself (and possibly others) the trouble and document your code.

answered Apr 21, 2014 at 17:33

answered Apr 21, 2014 at 17:33

Overall this looks very nice. It's easy to read, and is very refined. A few notes:

You can remove the Map from the first line of your structure.

 typedef struct
 {
 Color *blocks;
 int rows;
 int columns;
 } Map;

Your function map_delete() only performs one action, freeing the memory of map. Unless you plan to expand on the map structure where you will have to free more members within the structure itself, I would get rid of it and just call free(map). If you have compiler optimizations on, I guarantee that your compiler is already doing that for you anyways, so it should slightly decrease compilation time.
You can simpilfy your NULL tests.
```
 if(!map)
```
This is more of a style issue, but since this recently led to the Apple goto security flaw, I'm going to cover it. I don't think you are writing your brace-free single statement loops and test conditionals correctly. This is completely up to you to decide since this is a style issue, but I prefer to do it like this:
```
 if(!map) return NULL;
```
Other's may be more strict and tell you to use braces, but it's up to you.
You need more comments. There are some lines in your code that I have trouble following. And if I have trouble following it, chances are that you will have trouble following it as well when you re-visit this project in a few months. Save yourself (and possibly others) the trouble and document your code.

lang-c