Skip to content

Fixed spelling of 'quadradic' and added feature to alg input#2

Open
mariakimheinert wants to merge 2 commits intokinneerc:masterfrom
mariakimheinert:fix-quadradic-add-valid-alg-checking
Open

Fixed spelling of 'quadradic' and added feature to alg input#2
mariakimheinert wants to merge 2 commits intokinneerc:masterfrom
mariakimheinert:fix-quadradic-add-valid-alg-checking

Conversation

@mariakimheinert
Copy link

  • Fixed the spelling of "QUADRADIC" to "QUADRATIC" across all project files. The output should now read: "... algorithm is QUADRATIC".
  • Added checking to see if inputted algorithm name matches valid algorithm options. For example, before you could run $ java edu.allegheny.expose.examples.unique.UniqueExperiment bubble --verbose and it would still run the experiment, though for which algorithm I'm not sure. Now, if you run $ java edu.allegheny.expose.examples.unique.UniqueExperiment bubble --verbose, you get the following message: $ Sorry, that is not a valid "Unique" algorithm.
    This checking was implemented in main methods of SortingExperiment.java and UniqueExperiment.java. It may be good to update the README so that other extensions of the DoubleExperiment class may also include this checking.

@kinneerc
Copy link
Owner

Hi @Kimmaria, thanks for fixing those spelling errors. Performing input validation is also good idea . I tested your branch and everything seems to work. Thanks for your help!

One way to extend the input validation feature would be to output the list of accepted arguments when the user provides one that is unsupported. Whenever I forget what the valid arguments are, I have to look in the source code to see what strings are recognized. Having a message like this would be helpful:

Sorry, that is not a valid "Unique" algorithm.
Supported algorithms are: "unique1","unique2".

Also, as a minor nitpick point, maybe we should change the new validAlg variables in the UniqueExperiment and SortingExperiment classes to be local variables instead of static instance variables since only the main method needs to access them.

I am in favor of merging this pull request, what do you think @gkapfham?

@mariakimheinert
Copy link
Author

Hi there, @kinneerc. I implemented your suggestions and committed the change. Please let me know if you have any other suggestions. Thank you!

@gkapfham
Copy link
Collaborator

gkapfham commented Nov 9, 2016

Thanks @mariakimbap for adding this feature. @kinneerc, it looks like @mariakimbap has added some improvements to the system. I would like to see, in particular, the spelling mistakes in the output fixed. Also, this new feature of parameter checking seems useful. As long as you still agree, @kinneerc, I suggest that you merge this pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants