HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Searching for Values

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
valuesforsearching

Problem

This question pulls content and ideas from a good many of my other questions and their answers, and pulls a lot of things together. I am quite satisfied with my approach here, but seeing as I'm refactoring my app, I might as well ask about my practices and get suggestions for improvements.

This is my search method (thank you for helping me with it, Heslacher):

private static void GetResults(ref string[] query, ref ObservableCollection weight)
{
int position = -1;

foreach (string[] array in SearchKeys.Keys)
{
position++;
int length = array.Length;
int middle = length / 2;
char firstCharMidArray = array[middle][0];

foreach (string s in query)
{
if (s.Length == 0) continue;

int min = array[middle][0]

It works by pulling data from this:

public static ResourceLoader resourceFile = new ResourceLoader();

public static string[][] Keys = { resourceFile.GetString("SearchWSOneNote").Split(' '),
resourceFile.GetString("SearchWSMainMenu").Split(' '),
resourceFile.GetString("SearchWSTextMenu").Split(' '),
resourceFile.GetString("SearchWSTextBlockMenu").Split(' '),
resourceFile.GetString("SearchWSTableMenu").Split(' '),
resourceFile.GetString("SearchWSTableCellsMenu").Split(' '),
resourceFile.GetString("SearchWSDrawMenu").Split(' '),
resourceFile.GetString("SearchWSDrawnItemsMenu").Split(' '),
resourceFile.GetString("SearchWSPictureMenu").Split(' '),
resourceFile.GetString("SearchWSFileMenu").Split(' '),
resourceFile.GetString("SearchWSAppBarsMenu").Split(' '),
resourceFil

Solution

Var

Use the var keyword when defining local variables where the right hand side of the definition makes the type obvious. This looks cleaner and saves time when it comes to changing types during refactoring.

e.g.

int position = -1;


should be

var position = -1;


You should also use var when declaring foreach and for loop iterators.

e.g.

for (int i = min; i < max; i++)


should be:

for (var i = min; i < max; i++)


Design

I hate seeing methods with reference variables and a void return. Unless you have a really good reason to do so, you should be returning one of those parameters. In this case you can actually do away with passing either in by reference:

  • You don't appear to alter query at all, so you shouldn't pass it in using the ref keyword.



  • It doesn't really make sense to pass in weight at all. Each time you call this method you'll be appending the weight data to your weight collection. If that's by design, you should state that explicitly using an AddRange or Concat method once you have the results from this function, but not by doing it in the function. This hides it as a side effect from a programmer.



So you can instead return weight normally:

private static ICollection GetResults(ref string[] query)


Naming

GetResults might possibly be the only name other than Compute that could apply to any function.

Also, the name array isn't very descriptive of what that variable is. key would perhaps be closer to it. compoundKey closer still.

Code Snippets

int position = -1;
var position = -1;
for (int i = min; i < max; i++)
for (var i = min; i < max; i++)
private static ICollection<int> GetResults(ref string[] query)

Context

StackExchange Code Review Q#78846, answer score: 3

Revisions (0)

No revisions yet.