bridgesuncc / bridges-cxx Goto Github PK

In Element.h's getLinkVisualizer() function, if a bad_alloc occurs it returns null without actually fixing the error, potentially leading to a subsequent call giving undefined behavior.

Shouldn't return null, instead, set the value to null and return like normal.

In Bridges.h several setDataStructure() functions unnecessary cast their input as themselves.
Ex:
function(SLelement* head)
{
...stuff...
root = (SLelement*) head;
}

This is redundant and should be excluded.

Edge.h's getEdge() function just returns a pointer to itself, which can easily be aquired without this method.
This method should be enhanced or removed.

setVisualizerType is documented to throw an Exception, when it does not.

Validation.h's constructor just initializes the member variables to the same value each time, and none of the member functions behave any differently for different Validation objects.

The Validation class should be replaced by a Validation namespace with the same(now global) functions.

This will also remove the need for a static variable to store the current Validation object and it's getter function.

In Bridges.h several functions contain try/catch blocks for a manual throw. With the if block already determining if the error is present, it seems redundant to place it all in an additional try/catch block. Anything that you have after the throw could just be put in an else block instead.

The try/catch blocks should be removed, with the catch statement(s) put in the if block and with the remainder of the try statement(s) put in the else block.

C++ variables of built-in types (bool,char,int,float,double, ect.) as well as pointers are not guaranteed a default value, leaving the compiler to choose wt to do with them up to and including making demons come out of your nose. Therefore variables left uninitialized have undefined behavior that will be problematic for users as well as to test.

To prevent this, these variables should be given a default sentinel value (presumably their default constructor).
ex:
int a; should be changed to int a = 0; or int a = int();
or for a template class,
E a; should be changed to E a = E();

Uninitialized variables are contained in,
ADTVisualizer.h
Bridges.h
BSTElement.h
Edge.h
Element.h
ElementVisualizer (key)
LinkVisualizer (weight)

As far as I can tell it looks like the structure of bridges implies that only one Bridges object need/should be created at a time.

That being said Bridges.h should be made static using a namespace instead of a class, having the constructor parameters be settable global variables.

This would eliminate the need to get the current Bridges object, and fall more in line with the logic of the class.

The adjacency List in GraphAdjList.h uses Edge and instead should be using Edge.

There is a commented out function in TreeElement.h.
It should probably be removed or implemented.

All of the validate functions in Validation.h throw strings, but none are documented.

Validation.h's validate methods' throws should be documented.

Connector.h only holds one member variable and none of it's functions rely on this variable. In fact the only place this variable is used is in Bridges.h's visualize() function.

Meaning that Connector.h could be made "static", while the member variable server_url could be moved into Bridges.h along with its setter and getter functions. Or better yet made to be an internal constant, as the setter function is never used.

By making Connector.h static, I mean that it's current functions could be made global, without the use of the object class Connector, and to maintain the same style of structure a new namespace could be defined as Connector, that holds these global functions.

In LinkVisualizer.h, setWeight() takes in an int, while getWeight() returns a double.

setWeight() should be changed to take in a double.

Made it a namespace
Removed color validation, as a color object is always valid.
Added validation of an rgba channel for color creation
Slimmed down named colors, as no user would use so many obscure names. Instead kept short list of common names.

holds void pointer that casts to proper pointer
Made bridges static(ish)
Switched templated class to templated functions, user must use the same parameters for setDataStruct, and visualize
Added nested class that holds variables. Bridges holds nested object as static to prevent messy static declarations.
Added enum to represent visualizer type
Combined with connector
Holds assignment number as double instead of two ints

GraphAdjMatrix.h owns pointers and is lacking a destructor to delete them.

With a successful curl post in Connector.h, curl prints directly to std::out, bypassing cout. This leads to an unavoidable info dump into the standard output, likely disgruntling users.

To prevent this cluttering of std::out, CURLOPT_WRITEDATA should be set in some way.

Connector.h includes Element.h and ADTVisualizer.h though neither is needed.
These should be removed.

Several functions in Bridges.h are documented as throwing exceptions, when they do not.

The documentation should not indicate that these functions throw.

Element.h's assignment operator is calling the constructor, though it is unnecessary and increases the static id variable.

This call should be removed.

Should include Dlelement.h

Shouldn't include cmath or unordered map

string visualizerIdentifier is declared in ADTVisualizer.h, but never used.

The current implementation is C++11 compliant. Most folks should have a C++11
compiler, I would imagine these days. Maybe turn some options. Its simply a lot more powerful with some of teh newer constructs with containers and makes the code a lot more elegant.

I have gone through with the first round of fixes. Need to do a little more testing
on the examples I have before pushing a version. A few issues (fairly minor)
are if we want to convert some of the classes to static since you dont need
multiple instances of them (Bridges, Connector, Validation).

And some issues I need to update my C++ knowledge on!

Great job, Dakota on all the little things you have caught on from my summer
project!

I will push a version of the C++ code tomorrow.

MAX_ELEMENT_SIZE is not used in Element.h and should be removed.

The same variable is already in place in Validation.h.

GraphAdjMatrix.h and GraphAdjList.h's documentation uses triple slashes, instead of slash star.

In GraphAdjMatrix.h, one line has a missing slash, leaving out from the documentation.

This should be replaced with slash star.

string edge_data in Edge.h is never used.
This along with its set and get functions should be removed or used for some purpose.

Validation.h's int MAX_ELEMENTS_ALLOWED, follows all the conventions of a constant but isn't declared as one.

MAX_ELEMENTS_ALLOWED should be declared an constant.

Several functions in Bridges.h check their input isn't null, but others are missing such checks.

setDataStrucutute(Element_, int) doesn't check if Element is null
setDataStrucutute(GraphAdjList_, int) doesn't check if GraphAdjList is null
setDataStrucutute(GraphAdjMatrix*, int) doesn't check if GraphAdjMatrix is null

Moved Edge into AdjList
They hold map objects now not map pointers

Made it a subclass of SLelement

setVisualizer, setRoot, and the setDataStructure functions all set pointers, who may or may not have been set earlier. If bridges owns any of these pointers than it is its own responsibility to delete the old pointers before assigning a new value.

Connector.h is lacking pre curl validation of the URL or JSON.
With an invalid URL the curl post will fail, almost acting as its own validation, however with a valid url the post will succeed no mater the input for JSON.

I don't know much about the server code, but currently it seems that
with trash input, there is an Internal server error;
and with JSON formatted trash input, It returns an application html error page;

I don't know if this is what we want, because It seems we would want to inform the user as to the specific source of their errors. Something that pre curl JSON validation could provide.

I combined Element Visualizer and Element.
Removed opacity as Color will handle transparency
Holds map objects not pointers
As the unique id was only used for mapping in JSON creation, I just used its already unique memory address for mapping and deleted unique id.
Made getRepresentation virtual so polymorphism would work with BSTElement

Cleanup already checks root is null, so no need to check children. You could just check root, then call cleanup on children and delete on self.

Same for TreeElement cleanup

There is a long if else chain in Bridges.h's visualize function that could be replaced by a switch statement, since the list of choices are string constants.

Also instead of calling so many different helper functions, the functionality should be in visualize(). Most of these helper functions have similar calls, which would reduce the chance for errors and improve readability if they were called in one place within visualize() instead of within each helper, with the unique material residing in the switch block.

I see the reasoning behind this; again, to make the C++ and Java a little more uniform. We should perhaps use new to create the BRIDGES object and use pointers in the driver program. The visualize() and setDataStructure() require pointers anyway. I slightly favor this approach, though there is only one BRIDGES object ever.

Think over this and we can decide. The goal is to make the 2 versions of BRIDGES as close as possible.

Been updating the C++ version to be in line with the Java version.

Have a text formatter that automatically formats all source files.

Location attribute - checked with infinity - if either is infinity, then its not included in teh JSON.

MLElement - multilist -- implemented.

Git repo updated. Need some testing.

In the cleanup function, it shouldn't return if root is null, as the graphs may still need to be cleaned.

Since the flow is a set of if else's, I'd put the "Array" as the else case.

Also as only one dataStructure is in use at a time and explicit down casting is already in place, you could use a void pointer to hold both the root and graphs.

Bridges.h contains two Element*, one named root and another array_list.
As I understand it, only one of these pointers is ever used at a time as decided when set, so having two seems unnecessary when the same pointer could hold either data.

These should be consolidated into one Element pointer.

Validation.h's private regex ColorPatterns, is giving an error in instantiation. Currently what is implemented (string construction) utilizes ECMAScript, and instead should be constructed with string and flag, with the flag set for basic, to avoid current error.

Because this error is within the constructor, it is also effecting any other class that calls Validation.h.

Removed opacity as color will handle transparency
Significant overlap between this class and edge...I am considering combining them somehow.
Don't see how server needs to know the weight of each link...considering removing

Element.h's static int ids variable could be defined within the class for extra readability, instead or outside the class by itself.

This method has the paramters inverted, when compared to the Java version.

Lets change this to
Bridges::initialize( assignment_number, Bridges_user_id, Bridges_API_key)

So that it is consistent.

(I am working on the user manuals/tutorial pages today, once I finish we will need to modify the impacted examples as well).

ElementVisualizer.h's setKey() function is not validating the key.

Key doesn't seem to be used and should be implemented or removed.

If implimented, there should likely be a getKey() function, which ElementVisualizer.h is lacking.

Need to fix the Java version so that the BRIDGES initialization is independent of generic parameters, similar to the C++ version, which is much cleaner and less confusing.

In BSTelement.h's setKey() function the parameter K key is unnecessarily cast to K.

ADTVisualizer.h doesn't need to include TreeElement.h or vector.h

ADTVisualizer also has a collection of strings used for JSON creation that should be made const

The setVisualizer function in Element.h is taking in an object and trying to save it as a pointer. It should take in a pointer.

setVisualizer replaces a pointer that may or may not be set already. To prevent memory leaks, the old pointer should be deleted before reassignment.

Additionally the string identifier should be made constant

In GraphAdjList.h and GraphAdjMatrix.h constructors, two maps are created and then immediately cleared, even though default construction of maps creates them clear.