patterncMinor
Dijkstra's algorithm in C99
Viewed 0 times
algorithmdijkstrac99
Problem
I just implement Dijkstra's algorithm in C99. Can you review my code please? I'm looking for any mistake, performance improvement or coding style errors.
main.c
pathfinder.h
pathfinder.c
map.h
```
#ifndef MAP_H
#define MAP_H
static const char CHAR_WALL = '#';
static const char CHAR_NOTWALL = ' ';
static const char CHAR_PATH = '.';
static const char CHAR_BEGIN = '*';
static const char CHAR_END = '*';
static const char CHAR_VERTICAL_BORDER = '|';
static const char CHAR_HORIZONTAL_BORDER = '_';
typedef struct {
int x;
int y;
} tuple_2;
typedef struct {
tuple_2 coordinateFather;
int isChecked;
int distance;
int i
main.c
#include "src/map.h"
#include "src/pathfinder.h"
int main() {
map *map = init(createTuple_2(11, 11));
addBegin(map, createTuple_2(1, 1));
addEnd(map, createTuple_2(10, 10));
addWall(map, createTuple_2(5, 5));
addWall(map, createTuple_2(3, 5));
addWall(map, createTuple_2(4, 5));
addWall(map, createTuple_2(2, 5));
addWall(map, createTuple_2(6, 5));
addWall(map, createTuple_2(7, 5));
addWall(map, createTuple_2(8, 5));
addWall(map, createTuple_2(9, 5));
addWall(map, createTuple_2(2, 5));
addWall(map, createTuple_2(1, 5));
addWall(map, createTuple_2(0, 5));
dijkstra(map);
print(map, 1);
}pathfinder.h
#ifndef PATHFINDER_H
#define PATHFINDER_H
#include "map.h"
void dijkstra(map *map);
#endif // PATHFINDER_Hpathfinder.c
#include "pathfinder.h"
#include
void dijkstra(map *map) {
map->coord[getIndex(map->length.x, map->begin)].distance = 0;
while(!isAllVisited(map)) {
tuple_2 u = smallestDistance(map);
if(u.x == -1 || u.y == -1) {
printf("No path found\n");
return;
}
map->coord[getIndex(map->length.x, u)].isChecked = 1;
updateNeighborhood(map, u);
}
tuple_2 path = map->end;
while(path.x != map->begin.x || path.y != map->begin.y) {
path = map->coord[getIndex(map->length.x, path)].coordinateFather;
map->coord[getIndex(map->length.x, path)].isInThePath = 1;
}
}map.h
```
#ifndef MAP_H
#define MAP_H
static const char CHAR_WALL = '#';
static const char CHAR_NOTWALL = ' ';
static const char CHAR_PATH = '.';
static const char CHAR_BEGIN = '*';
static const char CHAR_END = '*';
static const char CHAR_VERTICAL_BORDER = '|';
static const char CHAR_HORIZONTAL_BORDER = '_';
typedef struct {
int x;
int y;
} tuple_2;
typedef struct {
tuple_2 coordinateFather;
int isChecked;
int distance;
int i
Solution
Generally the variable and function naming is clear and makes the code easier to read. If this program is for a first
or second year computer science class it's really very good. If you are new to C programming it is also very good.
Having modular programs is always good because it allows reuse of the code and minimizes side effects.
Module Partitioning
There is a software design principle called The Single Responsibility Principle.
This principle applies to functions and structs in C and functions and classes in C++, Java and other programming
languages. The principle states that a function or struct should be designed to do one thing and only one
thing. This makes it easier
to read, write, test and debug programs.
Another important software design principle is to reduce coupling
between modules and data structures. Coupling should be reduced to make each module of a program as independent as possible to reduce side effects. Decoupling modules allow the addition of features or bug fixing in one module without
breaking code in another module. This is why global variables are generally frowned upon, they create tighter
coupling.
Here are some general guides to software design.
The way the files are currently partitioned creates a tight coupling between
main.c and map.h. The main() function really only needs to know about the functions in pathfinder.h.
The map variable only needs to be available in the pathfind.c file,
The map header file contains 7 character constants. This creates the static variables in every file that
includes it. The static constants should only be included in the files that use them. The map.c file is
the only file that uses these constants so all the static variables should be declared there. Currently
these static variables are being defined in all files. If they were not staticly defined the linker would be
reporting multiply defined errors.
The function inBounds() should be declared as
in map.c since it isn't ever called externally.
It might be good if there was a tuple.h, and a tuple.c to manipulate the tuples, and a node.h and a node.c to
handle the nodes. This would allow the reuse of these modules at a later time.
More Meaningful Function and Variable Names
A better name for the
does, it would be a constructor in C++. There could then be an
One thought about the above code, it might be good to create an array of tuples called Wall and have the addWall()
function use the array to create the wall.
The main() function
The main() function is fine in this program, however, if the program was more complex all the main() function
should do is
Error Handling
The function
it is used. Witin the
NULL when
There are a number of ways to handle issues like this, in a program such as this one it would be best
to report the error and quite the program.
Some changes I would reccommend for main.c are:
There are a number of printf() calls in the code that seem to be reporting errors.
There is a special file pointer for reporting errors call stderr that is defined in stdio.h. Anything printed
to stderr (standard error) will be immediately output to the console. On some operating systems output printed
to stderr appears in read and is easier to see. I would change the printf() calls to fprintf(stderr, "...");.
The functions that have these pr
or second year computer science class it's really very good. If you are new to C programming it is also very good.
Having modular programs is always good because it allows reuse of the code and minimizes side effects.
Module Partitioning
There is a software design principle called The Single Responsibility Principle.
This principle applies to functions and structs in C and functions and classes in C++, Java and other programming
languages. The principle states that a function or struct should be designed to do one thing and only one
thing. This makes it easier
to read, write, test and debug programs.
Another important software design principle is to reduce coupling
between modules and data structures. Coupling should be reduced to make each module of a program as independent as possible to reduce side effects. Decoupling modules allow the addition of features or bug fixing in one module without
breaking code in another module. This is why global variables are generally frowned upon, they create tighter
coupling.
Here are some general guides to software design.
The way the files are currently partitioned creates a tight coupling between
main.c and map.h. The main() function really only needs to know about the functions in pathfinder.h.
The map variable only needs to be available in the pathfind.c file,
The map header file contains 7 character constants. This creates the static variables in every file that
includes it. The static constants should only be included in the files that use them. The map.c file is
the only file that uses these constants so all the static variables should be declared there. Currently
these static variables are being defined in all files. If they were not staticly defined the linker would be
reporting multiply defined errors.
The function inBounds() should be declared as
static int inBounds(map *map, tuple_2 coordinate)
{
....
}in map.c since it isn't ever called externally.
It might be good if there was a tuple.h, and a tuple.c to manipulate the tuples, and a node.h and a node.c to
handle the nodes. This would allow the reuse of these modules at a later time.
More Meaningful Function and Variable Names
A better name for the
init() function might be CreateMap() or ConstructMap(). That is what the functiondoes, it would be a constructor in C++. There could then be an
initMap() function that does all the following:addBegin(map, createTuple_2(1, 1));
addEnd(map, createTuple_2(10, 10));
addWall(map, createTuple_2(5, 5));
addWall(map, createTuple_2(3, 5));
addWall(map, createTuple_2(4, 5));
addWall(map, createTuple_2(2, 5));
addWall(map, createTuple_2(6, 5));
addWall(map, createTuple_2(7, 5));
addWall(map, createTuple_2(8, 5));
addWall(map, createTuple_2(9, 5));
addWall(map, createTuple_2(2, 5));
addWall(map, createTuple_2(1, 5));
addWall(map, createTuple_2(0, 5));One thought about the above code, it might be good to create an array of tuples called Wall and have the addWall()
function use the array to create the wall.
The main() function
The main() function is fine in this program, however, if the program was more complex all the main() function
should do is
- Process any arguments (argc and argv).
- Set up the environment. This includes setting up any error handling.
- Call a function to execute the program.
- Handle any errors.
Error Handling
The function
init() returns allocated memory. The pointer is never checked to see if it has value beforeit is used. Witin the
init() function there are 2 calls to malloc(). The malloc() function returnsNULL when
malloc() fails, but the return value is never tested.There are a number of ways to handle issues like this, in a program such as this one it would be best
to report the error and quite the program.
Some changes I would reccommend for main.c are:
#include
#include // Defines EXIT_SUCCESS and EXIT_FAILURE
#include "pathfinder.h"
int main(int argc, char *argv[])
{
int status = EXIT_SUCCESS;
map *map = init(createTuple(11,11));
if (map == NULL)
{
fprintf(stderr, "Unable to initialize map, exiting program\n");
return EXIT_FAILURE;
}
...
int status = dijkstra(map);
return status;
}There are a number of printf() calls in the code that seem to be reporting errors.
printf("Begin out of bounds\n");
printf("Invalid length\n");
printf("End out of bounds\n");
printf("Wall out of bounds");There is a special file pointer for reporting errors call stderr that is defined in stdio.h. Anything printed
to stderr (standard error) will be immediately output to the console. On some operating systems output printed
to stderr appears in read and is easier to see. I would change the printf() calls to fprintf(stderr, "...");.
The functions that have these pr
Code Snippets
static int inBounds(map *map, tuple_2 coordinate)
{
....
}addBegin(map, createTuple_2(1, 1));
addEnd(map, createTuple_2(10, 10));
addWall(map, createTuple_2(5, 5));
addWall(map, createTuple_2(3, 5));
addWall(map, createTuple_2(4, 5));
addWall(map, createTuple_2(2, 5));
addWall(map, createTuple_2(6, 5));
addWall(map, createTuple_2(7, 5));
addWall(map, createTuple_2(8, 5));
addWall(map, createTuple_2(9, 5));
addWall(map, createTuple_2(2, 5));
addWall(map, createTuple_2(1, 5));
addWall(map, createTuple_2(0, 5));#include <stdio.h>
#include <stdlib.h> // Defines EXIT_SUCCESS and EXIT_FAILURE
#include "pathfinder.h"
int main(int argc, char *argv[])
{
int status = EXIT_SUCCESS;
map *map = init(createTuple(11,11));
if (map == NULL)
{
fprintf(stderr, "Unable to initialize map, exiting program\n");
return EXIT_FAILURE;
}
...
int status = dijkstra(map);
return status;
}printf("Begin out of bounds\n");
printf("Invalid length\n");
printf("End out of bounds\n");
printf("Wall out of bounds");int addBegin(map *map, tuple_2 begin) {
if(!inBounds(map, begin))
{
printf("Begin out of bounds\n");
return EXIT_FAILURE;
}
map->begin = begin;
return EXIT_SUCCESS;
}Context
StackExchange Code Review Q#138154, answer score: 7
Revisions (0)
No revisions yet.