Counting the number of occurrences of chars in one string, in another string This program counts the total number of times any character from the source string can be found in the target string.

E.g Source String - "Hello World"
Target String - "llo"
Output = 5 (as there are 3 l's and 2 o's)

Source String - "Hello World"
Target String - "ooood"
Output = 3 (as there are 2 o's and 1 d)

Is there any better way of write the below program?

public class CountOccurence {
public static int countChars(String target, String source){
    char a[] = source.toCharArray();

    int loc = 0, count = 0;
    boolean charRepeat = false;

    for (loc = 0; loc < source.length(); loc++) {
        charRepeat = false;
        for (int z = 0; z < loc; z++) {
            if (a[loc] == a[z]) {
                charRepeat = true;
                break;
            }
        }

        if (charRepeat == false) {
            int i = 0;
            while ((i = target.indexOf(a[loc], i)) != -1) {
                count++;
                i++;
            }
        }
    }
    return count;
}

public static void main(String[] args) {
    String target = "Hello World";
    String source = "oood";

    int count = countChars(target, source);
    System.out.println(count);

    source = "lld";
    count = countChars(target, source);
    System.out.println(count);

}

}

Style

There are a few bad habits in here that should be resolved. Here's your core method:

public static int countChars(String target, String source){
    char a[] = source.toCharArray();

    int loc = 0, count = 0;
    boolean charRepeat = false;

    for (loc = 0; loc < source.length(); loc++) {
        charRepeat = false;
        for (int z = 0; z < loc; z++) {
            if (a[loc] == a[z]) {
                charRepeat = true;
                break;
            }
        }

        if (charRepeat == false) {
            int i = 0;
            while ((i = target.indexOf(a[loc], i)) != -1) {
                count++;
                i++;
            }
        }
    }
    return count;
}

let's go through some things that are style-based.

source comes before target. The natural thinking is that you go from the source to the target, your naming, and the order of the parameters, is awkward. I would call the source something like searchFor, and the target I would call searchIn. Note, you have confused these values so much that your description does not match your code. In your description you have:

E.g Source String - "Hello World"
Target String - "ooood"
Output = 3

but in your code you have:

String target = "Hello World";
String source = "oood";
int count = countChars(target, source);

Note how you have swapped the source/target between the example and the code.

source and target are basically bad names to have...
Your code uses a number of small variables that are unconventional. loc is not a terrible name, it is clearly short for location, but using the fill location would be better. I have a habit of using pos so I can't really complain. On the other hand, the variables a for the char array, and z for an index, are bad names. You already use charRepeat, so you know how to use meaningful names, use them. The z is interesting because it is common to use x, y, and z as names for coordinate space, so seeing z automatically implies there's an x and y too. You should rather just use the standard index variables i, j, and k (but don't use j unless you are already using i, and don't use k unless you are using j too).

Algorithm

Your algorithm is a bit messed up too. Basically, you scan the searchFor chars, and find the first occurrence of each char value. If it's the first one, you then use that to scan the searchIn value, and you count the 'hits' in there. There are two issues here. The first issue is the performance of the 'first check'. The performance is \$O(n^2)\$ where \$n\$ is the number of chars in the searchFor value.

Then, for each unique char, you then do a loop through all the searchIn chars. The loop through all those are disguised in two steps, first the while loop, and then inside that, the indexOf search.

The net result is that you have a performance of \$O(nm)\$ or \$O(n^2)\$ depending on whether there are more characters in the searchIn or searchFor strings.

There is a better solution for both the uniqueness testing, and for the searching. Consider sorting the chars in each string:

char[] lookFor = searchFor.toCharArray();
char[] lookIn = searchIn.toCharArray();

Arrays.sort(lookFor);
Arrays.sort(lookIn);

The two sorts are \$O(n \log n)\$ performance, so this is, so far, significantly less complicated... now, how do we compare? Well, with a 'simple' loop over all the lookIn characters:

int forpos = 0;
int count = 0;
for (char c : lookIn) {
    // advance the searchFor cursor to the next usable position
    while (forpos < lookFor.length && lookFor[forpos] < c) {
        forpos++;
    }
    if (forpos >= lookFor.length) {
        //nothing left to look for.
        break;
    }
    if (c == lookFor[forpos]) {
        count++;
    }
}
return count;

With the above code, you do two sorts, and 1 loop through each value. The net result is that your performance complexity is simply \$O(n \log n)\$ where \$n\$ is the longer of the searchIn or searchFor variables.

By sorting each side, you give yourself a significant algorithmic advantage.


Very small start to MVC refactor in iOS app I started to refactor my app to MVC today. I wasted a lot of time because of simple mistakes.

One was I kept trying to access the getter method inside of my custom getter method, which just causes and endless loop of gets.

Even when I figured it out, I repeated this mistake several times. I think it's safe to say I'll never use self.iVar inside of it's own getter method ever again.

The thing I sort of want advice on is not just whether I did an ok MVC refactor, but whether I approached things the right way.

One of the things that is really bugging me is my implementation of the phoneNumbers getter method in HALContact.

When I first created it, my nested for loops in my New View Controller Snippet would run forever and just keep adding the same phone number instead of moving on.

I finally just got frustrated and added the removeAllObjects method at the beginning of the phoneNumbers getter. As much as it works fine, I'm not sure if that's the way it should be.

And this is how the getter now looks and works fine:

- (NSArray *)phoneNumbers {

    // The difference is I remove all of the objects
    if (_phoneNumbers) {
        [_phoneNumbers removeAllObjects];
    }

    if (!_phoneNumbers) {
        //Create _phoneNumbers array
        _phoneNumbers = [[NSMutableArray alloc]init];
    }

    ABMultiValueRef *phoneNumberRef = ABRecordCopyValue(_contactRef, kABPersonPhoneProperty);

    NSString *phoneNumber = [[NSString alloc] init];

    // Make sure multi value ref exists
    if (phoneNumberRef) {
        CFIndex numberOfPhoneNumbers = ABMultiValueGetCount(phoneNumberRef);

        for (CFIndex i = 0; i < numberOfPhoneNumbers; i++) {
            phoneNumber = (__bridge_transfer NSString *)ABMultiValueCopyValueAtIndex(phoneNumberRef, i);
            CFStringRef label = ABMultiValueCopyLabelAtIndex(phoneNumberRef, i);
            if (label) {

            [_phoneNumbers addObject:phoneNumber];
                NSLog(@"phoneNumbers count in method: %d", _phoneNumbers.count);
            }
            CFRelease(label);
        }
        CFRelease(phoneNumberRef);
    }
    return _phoneNumbers;
}

As far as the general MVC refactor, I threw up all of my new and old files on Pastebin which I will link to in parentheses if you'd rather view them in separate tabs.

Here's a snippet of what the View Controller looks like after the refactor. It now uses 2 model classes I made to handle the address book and contacts data:

// Instantiate our address book instance
self.addressBook = [[HALAddressBook alloc]init];

// Instantiate our contact instance
self.currentContact = [[HALContact alloc]init];

//Ask for access to Address Book.
BOOL result = [self.addressBook requestAccess];

if (!result) {
    [self performSegueWithIdentifier:@"addFriendsToMediaCaptureSegue" sender:self];
}

if (result) {

    // Loop through all address book contacts
    for (int index = 0; index < self.addressBook.allContacts.count; index++) {

       // Create contactRef from current contact
        self.currentContact.contactRef = (__bridge ABRecordRef)(self.addressBook.allContacts[index]);

      // Loop through all of the current contact's phone numbers
        int index2;
        for (index2 = 0; index2 < self.currentContact.phoneNumbers.count; index2++) {
            if (self.currentContact.firstName) {
                // Add current contact's phone number to array
                [self.potentiaFriendsPhoneNumberArray addObject:self.currentContact.phoneNumbers[index2]];

                // Add current contact's first name to array
                [self.potentiaFriendsNotInParseFirstNamesArray addObject:self.currentContact.firstName];
            }
        }
    }
}

And here are the 2 model classes that I added. One for the address book and one for the contact:

1A. HALAddressBook header

1B. HALAddressBook implementation

2A. HALContact header

2B. HALContact implementation

if (_phoneNumbers) {
    [_phoneNumbers removeAllObjects];
}

if (!_phoneNumbers) {
    //Create _phoneNumbers array
    _phoneNumbers = [[NSMutableArray alloc]init];
}

Instead of this, we can replace all these lines with simply:

_phoneNumbers = [NSMutableArray array];

The best advantage of this, is we don't run into any problems of emptying an array that someone else might have a reference to.  This could cause problems if, for example, you were using the phone numbers array as the data for a table view.  If you all of the sudden removed all the objects, but didn't call reloadData on the table view, the next time it tried grabbing a cell, it could be an index out of bounds.

ABMultiValueRef *phoneNumberRef = ABRecordCopyValue(_contactRef, kABPersonPhoneProperty);

NSString *phoneNumber = [[NSString alloc] init];

// Make sure multi value ref exists
if (phoneNumberRef) {
    CFIndex numberOfPhoneNumbers = ABMultiValueGetCount(phoneNumberRef);

    for (CFIndex i = 0; i < numberOfPhoneNumbers; i++) {
        phoneNumber = (__bridge_transfer NSString *)ABMultiValueCopyValueAtIndex(phoneNumberRef, i);
        CFStringRef label = ABMultiValueCopyLabelAtIndex(phoneNumberRef, i);
        if (label) {

        [_phoneNumbers addObject:phoneNumber];
            NSLog(@"phoneNumbers count in method: %d", _phoneNumbers.count);
        }
        CFRelease(label);
    }
    CFRelease(phoneNumberRef);
}

I think this section is problematic.  I don't know enough about AddressBook to really tell you for sure, so for now I'm just mentioning it and reserving a section for in the future if I come back and figure out what's wrong with it.  The CFRelease on phoneNumberRef seems particularly suspicious.

BOOL result = [self.addressBook requestAccess];

if (!result) {
    [self performSegueWithIdentifier:@"addFriendsToMediaCaptureSegue" sender:self];
}

if (result) {

This could and probably should be refactored into:

if (![self.addressBook requestAccess]) {
    [self performSegueWithIdentifier:@"addFriendsToMediaCaptureSegue" sender:self];
} else {

But... with that said, I did take a look at the requestAccess method in the link.  Strictly speaking, it's not in the question and so it's not strictly up for review, but, I'm quite certain it will always return NO, as the return is synchronous but the variable being returned is only set in an asynchronous block, and as such, will always be set too late.

int index2;
for (index2 = 0; index2 < self.currentContact.phoneNumbers.count; index2++) {
    if (self.currentContact.firstName) {
        // Add current contact's phone number to array
        [self.potentiaFriendsPhoneNumberArray addObject:self.currentContact.phoneNumbers[index2]];

        // Add current contact's first name to array
        [self.potentiaFriendsNotInParseFirstNamesArray addObject:self.currentContact.firstName];
    }
}

First, we can declare our iterator variable in the for loop initialization statement.  Remember, you did this a few lines up.  But perhaps more importantly, why don't we use a forin?

for (id phoneNumber in self.currentContact.phoneNumbers) {
    [self.potentialFriendsPhoneNumberArray addObject:phoneNumber];
    [self.potentialFriendsNotInParseFirstNamesArray addObject:self.currentContact.firstName];
}

We should also move the if check outside the loop so that the entire loop can be circumvented in the case that we're never going to add anything anyway.  Embed the loop within the if.

Also, it doesn't make a ton of sense to me for there to be two separate arrays. I understand that you'll want a name associated with all the phone numbers, however it's better to do this with a single array, either by filling it with dictionaries with a key for the phone number and a key for the name, or by creating a custom object that will hold both of these parts.


Date of Birth Value Object 2 I realize it could be argued that it is an overkill to create a ValueObject for such a simple type. I am just trying to make sure that I understand the principles of overriding/overloading these methods correctly. I would be grateful for some critical feedback. Specifically:

Are all the methods implemented correctly?
Have I compared the date of births correctly i.e. if (candidate.AddYears(21).Date > DateTime.Now.Date) assuming that I am using the western idea of age.

Please note that with regards to naming conventions; I have used what is advised here

public sealed class DateOfBirth
    {
        private readonly DateTime _value;
        public DateOfBirth(DateTime value)
        {
            if (value.TimeOfDay.TotalSeconds > 0)
                throw new ArgumentException("Date of birth cannot contain a time.", "DateOfBirth");
            if (IsValid(value) == true)
            {
                _value = value;
            }
        }

        public DateOfBirth(int year, int month, int day)
        {
            var value = new DateTime(year, month, day);
            if (IsValid(value)==true)
            {
                _value = value;
            }
        }

        public static bool IsValid(DateTime candidate)
        {
            if (candidate.AddYears(21).Date > DateTime.Now.Date)
                throw new ArgumentException("Age must be greater than 21.", "DateOfBirth");
            return true;
        }

        public DateTime Value
        {
            get
            {
                return _value;
            }
        }

    public override bool Equals(object obj)
        {
            return Equals(obj as DateOfBirth);
        }

        public override int GetHashCode()
        {
            return _value.GetHashCode();
        }

        public static implicit operator DateTime(DateOfBirth dateOfBirth)
        {
            return dateOfBirth._value;
        }

        public static bool operator ==(DateOfBirth dateOfBirth1, DateOfBirth dateOfBirth2)
        {
            if (!ReferenceEquals(dateOfBirth1, null) &&
                ReferenceEquals(dateOfBirth2, null))
            {
                return false;
            }
            if (ReferenceEquals(dateOfBirth1, null) &&
                 !ReferenceEquals(dateOfBirth2, null))
            {
                return false;
            }
            if (ReferenceEquals(dateOfBirth1, null) &&
                 ReferenceEquals(dateOfBirth2, null))
            {
                return true;
            }
            return dateOfBirth1.Equals(dateOfBirth2);
        }

        public static bool operator !=(DateOfBirth dateOfBirth1, DateOfBirth dateOfBirth2)
        {
            return !(dateOfBirth1 == dateOfBirth2);
        }

        public bool Equals(DateOfBirth other)
        {
            if (ReferenceEquals(other, null))
                return false;
            return _value == other._value;
        }
    }

Note: this is a follow-up question to this one.

Design:

I still think that a DateOfBirth class is too narrow. It should be the name of a property, not a separate class. If you must create a class, create a reusable Date class instead. Enforcing this sort of restrictions at the type level isn't worth the extra work and maintenance cost (in C#). That's my opinion, of course, but judging by your many questions it really is a lot of work, isn't it? ;)
I expect a DateOfBirth.IsValid method to check whether the given input is a valid birth-date. That is, I expect it to reject nonsense input like '-5 45 2018'. However, your implementation enforces a business rule instead. There are several problems with that:

It's confusing: it's basically telling me that '7 februari 2018' is not a valid birthdate.
It's inflexible: what if some logic applies to people above 21, but other logic only to people below 18?

Again, why expose Value directly? As I mentioned before, a date does not have a time component, but by exposing a DateTime property you make it look like it does. Of course, you may want to convert between dates and date-times, but that's what conversion operators are for. A Year, Month and Day property are far more in line with what a date actually is. Look at DateTime, for example: it uses a timestamp as internal storage, but it provides meaningful properties like Year, Month, Day, Hour, Minute, Second, and so on, which results in a much more natural and practically useful interface.
Where did the comparison operators go? Are they superseded by the IsValid method? Or are you planning to use Value for that? If so, why create a separate class in the first place?

Implementation:

No need for == true: IsValid already returns a boolean.
The constructors do not throw if validation fails: they fall back to a 'default' value instead. That's very surprising behavior, in a bad way. It's also inconsistent, because some input does produce an exception. (EDIT: Excuse me, they do throw, because IsValid throws - so that underlines the point below! ThrowIfInvalid would be a more appropriate method name.)
Why does IsValid throw an exception? Its signature suggests that it will only return true/false. Such unexpected behavior makes code more difficult to work with.
The value property can be written more succinctly as public DateTime Value => _value;, but you could also use a get-only property instead.
You may want to mention which version of C# you're using. I mentioned the a is null improvement from C#7, but I don't see that applied here
so I guess you're still stuck with an older version?

Finally, the == operator can be simplified:

if (ReferenceEquals(a, null) != ReferenceEquals(b, null))
{
    return false;       // One is null, the other is not
}
else if (ReferenceEquals(a, null))
{
    return true;        // Both are null
}
else
{
    return a.Equals(b); // Both are not null
}


Finding perfect numbers My teacher wants me to find all the perfect numbers between 2 and 1000 and must use:

for loops
% operator to determine if a number is a divisor
static final variables for 2 and 1000
A static method to return a boolean value by finding and adding the divisors of the integer parameter and comparing the sum to the parameter

The code works and I just wanted a second opinion on it.

public class Problem_B {
    //Search between 2 and 1000
    public static final int minNum = 2;
    public static final int maxNum = 1000;

    public static void main(String[] args) {
        for (int num=minNum; num <= maxNum; num++)
        if (perfectNum(num)) {
            int i_sumDiv = 0;
            for (int j=1; j <= (num/2); j++) {
                if(num%j == 0)
                    i_sumDiv = i_sumDiv + j;
            }
            if (i_sumDiv == num)
                System.out.println(num + " is a perfect number.");
        }
    }

    public static Boolean perfectNum(int num) {
        // a variable that holds the sum of the factors
        int sum = 0;

        for (int factor = 1; factor < num; factor++) {
            if (num % factor == 0) {
                sum += factor;
            }
        }

        if (sum == num) {
            return true;
        } else {
            return false;
        }
      }
    }

//Search between 2 and 1000
public static final int minNum = 2;
public static final int maxNum = 1000;

That comment doesn't say anything that the code doesn't already state IMO. Now if you need to change either value, you have to update the comment as well, otherwise you end up with this:

//Search between 2 and 1000
public static final int minNum = 2;
public static final int maxNum = 10000;

And would that be a typo in the code, or just a misleading comment?

for (int num=minNum; num <= maxNum; num++)

Spacing isn't consistent between operators, that's distracting when you read it.

for (int num = minNum; num <= maxNum; num++)

Reads more fluently. Now, you have a scope, but where's the opening brace? Code that's explicit won't omit them, even for a single statement.

if (perfectNum(num)) {

Might be I'm the verbose type, but I don't see a reason not to rename num to number or value, and perfectNum to isPerfectNumber, making the condition read like this:

if (isPerfectNumber(value)) {

Now, I like how minNum and maxNum are consistent with num, so I'd then rename them to minValue and maxValue to maintain that beautiful consistency.

int i_sumDiv = 0;

Now that's a bit of a mystery. why is there an i_ prefix? The Div abbreviation seems uncalled for, and makes the name a bit ambiguous.

Okay I need to stop the line-by-line analysis here.

You're doing the work twice. First here:

if (perfectNum(num)) {

and then if the function returns true, you do the work all over again - you know it's a perfect number already; you're ready to output right there and then:

if (isPerfectNumber(value)) {
    System.out.println(num + " is a perfect number.");
}

So let's look at that function.

public static Boolean perfectNum(int num) {
    // a variable that holds the sum of the factors
    int sum = 0;

    for (int factor = 1; factor < num; factor++) {
        if (num % factor == 0) {
            sum += factor;
        }
    }

    if (sum == num) {
        return true;
    } else {
        return false;
    }
  }
}

I like factor much more than j. I'm seeing braces on all scopes, and although sum isn't a completely descriptive noun, it's much better than i_sumDiv: I like this implementation much better than the superfluous one that executes after this function returns.

The indentation is broken though:

    }
  }
}

Should be:

        }
    }
}

Also, this is redundant:

    if (sum == num) {
        return true;
    } else {
        return false;
    }

It can easily be replaced with a single return statement:

return sum == num;

for (int factor = 1; factor < num; factor++) {

That loop is inefficient; the (redundant) one in main is better.


Code improvement suggestions for cost calculation I have to calculate costs based on country code and city(optional). There is also a condition that countries in Asia have separate pricing logic depending on both country and city and they are stored in a different table (for segregating regional price variances). For the rest, it's only dependent on country code. Let's assume we must have separate tables Asia & the rest.

Please suggest some improvements in the below code.

def get_cost_by_location(country_code: str, city=None) -> Optional[models.OperatorCost]:
    cost = None

    # check if the country is in Asia
    if models.OperatorCostAsia.objects.filter(country=country_code).exists():
        cost = models.OperatorCostAsia.objects.filter(country=country_code, city__icontains=city).first()

        if cost is None:
            cost = models.OperatorCostAsia.objects.filter(country=country_code).first()
    else:
        cost = models.OperatorCost.objects.filter(country=country_code).first()

    return cost

I have two different tables to calculate the prices. The pricing table for Asia has three columns: Country code, city name and the price itself. The pricing table for rest of the countries just has country code and price columns.
I get a mandatory country code and an optional city name as parameters from the frontend. First I check these params against the Asia pricing table. if the selected country happens to be Asian, I query against country code and city name (if there is one). If the city param is empty, i just pick the first match against the country.

If the country is not an Asian country, I simply query the second (Rest of the world) table to find the pricing against the country code.

The first thing that would benefit this code, beside removing the useless initialization for cost, is to save the models.OperatorCostAsia.objects.filter(country=country_code) queryset for reuse instead of rebuilding it each time. It feels easier to understand and filtering twice is identical to using two parameters in a single filter anyway:

def get_cost_by_location(country_code: str, city=None) -> Optional[models.OperatorCost]:
    asian_country = models.OperatorCostAsia.objects.filter(country=country_code)
    if asian_country.exists():
        cost = asian_country.filter(city__icontains=city).first()
        if cost is None:
            cost = asian_country.first()
    else:
        cost = models.OperatorCost.objects.filter(country=country_code).first()

    return cost

Now there are a few other changes that I would perform but these may change the behavior of the function so it's a bit of guessing territory on how you use this function.

For starter I would use iexact instead of icontains when searching for the city name, it feels less error prone.

Second, I would use .get(...) instead of .filter(...).first() when applicable to check for wrongly constructed inputs that would return more than the one result they are expected to return:

def get_cost_by_location(country_code: str, city=None) -> Optional[models.OperatorCost]:
    asian_country = models.OperatorCostAsia.objects.filter(country=country_code)
    if asian_country.exists():
        try:
            return asian_country.get(city__iexact=city)
        except models.OperatorCostAsia.DoesNotExist:
            return asian_country.first()
    else:
        return models.OperatorCost.objects.get(country=country_code)


Cleaning up a redundant switch statement How can I use a more generic method to clean up the redundancy in the switch statement below?

I'm working on an ASP.NET webforms app that will have five identical user controls that I will be showing and hiding based on the value of a dropdown list. (I have already been down the path of trying to add and remove the controls dynamically--the issue of managing the view states for the controls would have been very complicated, so in the interest of shipping I'm opting for a fixed number of controls.)

The approach I'm currently using works but isn't very elegant:

protected void DisplayUserControls(int numberOfControls)
{
    switch (numberOfControls)
    {
        case 2:
            UserControl1.Visible = true;
            UserControl2.Visible = true;
            UserControl3.Visible = false;
            break;

        case 3:
            UserControl1.Visible = true;
            UserControl2.Visible = true;
            UserControl3.Visible = true;
            break;

        default:
            UserControl1.Visible = true;
            UserControl2.Visible = false;
            UserControl3.Visible = false;
            break;
    }
}

What are some options here? The best I can come up with is using numberOfControls to build the name of the control, but that seems hacky. Suggestions appreciated.

EDIT: I implemented a solution similar to the accepted answer below. I'm stuck on loading up the controls in the list. The commented out code is more along the lines of what I'd like to do but can't get it working. The uncommented code works. Suggestions?

private List<ShiftControl> PopulateShiftControlList()
{
    ShiftControl shiftControlList = new List<ShiftControl>();

    //ControlCollection panelControls = ShiftPanel.Controls;

    //foreach (ShiftControl control in panelControls)
    //{
    //    shiftControlList.Add(control);
    //}

    shiftControlList.Add(ShiftControl1);
    shiftControlList.Add(ShiftControl2);
    shiftControlList.Add(ShiftControl3);
    shiftControlList.Add(ShiftControl4);

    return shiftControlList;
}

I would create a list of usercontrols. Based on the amount of controls that need to be visible, traverse the list and set x controls to visible.

protected void DisplayUserControls(List<UserControl> controls, int numberOfControls)
{
    Contract.Requires( numberOfControls >= 0 && numberOfControls <= controls.Count );

    for ( int i = 0; i < numberOfControls; ++i )
    {
        controls[ i ].Visible = true;
    }
    for ( int i = numberofControls; i < controls.Count; ++i )
    {
        controls[ i ].Visible = false;
    }
}

Or rather in one go:

protected void DisplayUserControls(List<UserControl> controls, int numberOfControls)
{
    Contract.Requires( numberOfControls >= 0 && numberOfControls <= controls.Count );

    for ( int i = 0; i < controls.Count; ++i )
    {
        controls[ i ].Visible = i < numberOfControls;
    }
}


Multiway Ruby conditional I'm a little unhappy with the look of this "flexible" way of dealing with errors from an API in Ruby:

class ApiError < StandardError
  def initialize(response)
    message = response.is_a?(String) ? response
            : !(response.respond_to? 'body') ? 'Error'
            : response.body.is_a?(Hash) ? response.body['message']
            : response.body.to_s
    super(message)
  end
end

The idea is you can pass to ApiError.new either a string, or an object with an accessor called body in which case we'll take the body if it's a string or its message property if it's a hash. We'll use a generic error message if the argument is anything else.

What would be the idiomatic alternative, if any? I'm happy to take suggestions for completely different approaches (multiple constructors, static factories, etc.) but am still interested in a single-initialize-method approach. I know I can do if...elsif...elsif...else...end. Is that better?

I would try a sequence of case statements:

class ApiError < StandardError
  def initialize(response)
    message = case
      when response.is_a?(String)
          response
      when !(response.respond_to? 'body')
        'Error'
      when response.body.is_a?(Hash)
        response.body['message']
      else
        response.body.to_s
      end
    super(message)
  end
end

###test code
class XX
  attr_accessor :body
end

p ApiError.new('string')
p ApiError.new(:x)

xx = XX.new()
p ApiError.new(xx)
xx.body={'message' => 'my message'}
p ApiError.new(xx)

I hope I understood your code correct.
Actually there is no check, if a given Hash has a 'message'-key.

You can also omit the message variable:

class ApiError < StandardError
  def initialize(response)
    super case
      when response.is_a?(String)
          response
      when !(response.respond_to? 'body')
        'Error'
      when response.body.is_a?(Hash)
        response.body['message']
      else
        response.body.to_s
      end
  end
end


Make my set proxy script better Below is my bash script to setup a proxy, I am by no means an expert in bash. I suspect there is a better way to write this, any tips would be appreciated.

Is there a better way to do nested while-loops and if-statements?

#!/bin/bash
set -e

#functions
function yesno() {
  while read -n1 -r -p "[y/n/QUIT] > "; do
    case $REPLY in
      y|n) return;;
      q) echo; echo; exit 1;;
      *) printf "\nERROR - Invalid response, please enter Yes OR No.\n";;
    esac
  done
}

function startover() {
  printf "\n\n\e[1m\e[31mStarting Over...\e[0m\n"
  sleep 2
  unset proxyport proxyhost
}

    shopt -s nocasematch
    while true; do
      printf "\nIs a proxy required to access outbound \e[1m\e[7mHTTP/HTTPS\e[0m sites?\n"
      yesno
      if [[ $REPLY == "y" ]]; then
        printf "\n\nPlease enter your proxy url\nExample - \e[1m\e[7mproxy.example.com:8080\e[0m\n"
        while read -r -p "Proxy > " proxy; do
          if [[ $proxy =~ ^https:// && $proxy =~ :[0-9]{1,5}$ ]]; then
            printf "\nERROR - HTTPS with a port is not supported, please re-enter without '\e[1m\e[97m\e[41mHTTPS://\e[0m' prefix\n"
          elif [[ $proxy =~ ^(https?://)?([a-z0-9][-a-z0-9\.]{0,61}[a-z0-9]\.[a-z]{2,5})?.((25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\.(25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\.(25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\.(25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?))?(:[0-9]{1,5})?$ ]]; then
            if [[ $proxy =~ :[0-9]{1,5}$ ]]; then
              proxyport=${proxy##*:}
              proxy=${proxy%:*}
              echo "Port entered $proxyport"
            elif [[ $proxy =~ ^https:// && ! $proxy =~ :[0-9]{1,5}$ ]]; then
              proxyport=443
              echo "URL starts with HTTPS, assuming $proxyport"
            else
              proxyport=80
              echo "No port entered, assuming $proxyport"
            fi
            proxyhost=${proxy##*/}
            break
          else
            printf "\nERROR - \e[1m\e[97m\e[41m$proxy\e[0m is not supported please try again\n"
          fi
        done
        printf "\nCreating Proxy Settings\n"
        sleep 2
        printf "Proxy Hostname: \e[1m\e[97m\e[44m$proxyhost\e[0m\nProxy Port: \e[1m\e[97m\e[44m$proxyport\e[0m\nDoes the information above look correct?\n"
        yesno
        if [[ $REPLY == "n" ]]; then
          startover
        fi
      fi
      break
    done
    shopt -u nocasematch

Update 4/6/17
I have improved the regex so it now supports IP and hostnames, i am still working on actually setting the proxy settings.

Messy formatting

The first thing I noticed about this script is the messy formatting.
Poorly formatted code is hard to read and maintain.

Extract complex conditions to helper functions

The code has many complex conditions.
It would be more readable if you extracted them into helper functions.

For example, instead of this:

  if [[ $proxy =~ ^https:// && $proxy =~ :[0-9]{1,5}$ ]]; then
    # ...

You could introduce this helper function:

isHttpsWithPort() {
  [[ $proxy =~ ^https:// && $proxy =~ :[0-9]{1,5}$ ]]
}

And then use it with:

if isHttpsWithPort; then
  # ...

I recommend to change all complex conditions like that,
and the code will be more readable,
thanks to self-explanatory names of the functions.

Make yesno return success or failure

After calling the yesno function you check the value of REPLY.
This is not very ergonomic.
You could make yesno return success for "yes" and failure for "no":

yesno() {
  while read -n1 -r -p "[y/n/QUIT] > "; do
    case $REPLY in
      y) return 0;;
      n) return 1;;
      q) echo; echo; exit 1;;
      *) printf "\nERROR - Invalid response, please enter Yes OR No.\n";;
    esac
  done
}

When written this way,
instead of this:

    yesno
    if [[ $REPLY == "n" ]]; then
      startover
    fi

You could simplify to:

yesno || startover

Similarly, instead of this:

  yesno
  if [[ $REPLY == "y" ]]; then

You could write:

if yesno; then

Loop not looping

The main while loop doesn't actually loop, so it should be removed:

while true; do
  # ...
  break
done


FileSystemWatcher created event I wrote a little function that monitors some directories. If a file gets created, it will check in the extension list for a match. Can you please tell me if I could make my function better in any way?

private void sysWatcher_Created(object sender, FileSystemEventArgs e) {

    try {

        if (File.Exists(e.FullPath)) {

            var fileName = Path.GetFileName(e.FullPath);
            var fullPath = e.FullPath;
            var changeType = e.ChangeType;
            var fileExtension = Path.GetExtension(e.FullPath);

            _extensionList.RemoveAll(r => string.IsNullOrEmpty(r.Trim()));
            _ignoreList.RemoveAll(r => string.IsNullOrEmpty(r.Trim()));

            // Checking for match in Extension List
            bool _extResult = _extensionList.Any(s => e.FullPath.Contains(s));

            if (_extResult) {

                // Checking for match in Ignore List
                bool _ignResult = _ignoreList.Any(s => e.FullPath.Contains(s));

                if (!_ignResult) {

                    if (!GeneralSettings.autoMode) {

                        App.Current.Dispatcher.Invoke(delegate {

                            if (File.Exists(e.FullPath)) {
                                var fileInfo = new FileInfo(e.FullPath);
                                var wFunc = new WarningItem(e.FullPath, e.Name, AppSettings.SizeSuffix(fileInfo.Length),
                                    fileInfo.CreationTime.ToString("g"), fileInfo.Extension);

                                var wWin = new WarningWin();
                                wWin.init(wFunc);
                                wWin.ShowDialog();

                                // Add to Warning List and List Box
                                _warningList.Add(new WarningItem {
                                    DirName = Path.GetDirectoryName(e.FullPath),
                                    CreationDate = DateTime.Now.ToString("g"),
                                    Extension = fileInfo.Extension,
                                    FullPath = e.FullPath,
                                    FileName = e.Name,
                                    FileSize = AppSettings.SizeSuffix(fileInfo.Length),
                                    StatusImage = new Uri("/Images/inWatch.Folder.png", UriKind.RelativeOrAbsolute)
                                });
                                lstWarning.ItemsSource = _warningList;
                                tabWarning.IsSelected = true;
                                cmdSearchWarnings.Visibility = Visibility.Visible;
                            }
                        });
                    }
                    else {

                        Scan.scanFile(e.FullPath);
                    }
                }
            }
        }
    }
    catch (Exception ex) {
        GeneralSettings.LogException(ex);
    }
}

Several odd elements in this code.

Pointless variables

These variables look pointless:

        var fullPath = e.FullPath;
        var changeType = e.ChangeType;

You could use e.FullPath and e.ChangeType where you need it, the extra local variables don't seem to have any purpose.

Garbage not filtered out soon enough

It's very strange to execute this code every time a file is created:

        _extensionList.RemoveAll(r => string.IsNullOrEmpty(r.Trim()));
        _ignoreList.RemoveAll(r => string.IsNullOrEmpty(r.Trim()));

Why do you need to filter out the garbage every time instead of doing it just once?

Even better: how did garbage every enter these lists? It would be better to prevent that, and organize your code in a way that it's impossible to enter garbage into these lists.

Lastly, the result of r.Trim() will never be null, so the null-check is pointless.

Why not use a Set ?

Instead of putting the extensions and ignored extensions into lists, it would be better to use sets instead. It will it a lot more efficient to check if they contain an element or not, without the need for lambdas.

Exception handling

The try block is to large. It's better to limit the scope of try blocks, to make it easier to see the piece of code where an operation might fail.

Instead of catching a generic Exception, it's better to catch the most specific kind of exception that can be thrown.


Selecting three stylesheets in a specific order I'm somewhat new to SQL (only had to use it in Database classes). I have a simple table of stylesheets, from which I want to select specific ones in a specific order (e.g. reset, then layout, then colors). Below is an example selector that my current algorithm might generate. Is there a better (more efficient or more intended use of keywords) way to do this?

SELECT *
  FROM `Stylesheets`
  WHERE
    ID = 14

UNION

SELECT *
  FROM `Stylesheets`
  WHERE
    ID = 1

UNION

SELECT *
  FROM `Stylesheets`
  WHERE
    ID = 5
;

See full example on SQLFiddle

I have a sneaking suspicion that this might should do an IN clause and then handle the sorting in code, but if you want to keep it in SQL, you can do:

SELECT *
  FROM Stylesheets
  WHERE
    ID IN (14, 1, 5)
  ORDER BY ID = 14, ID = 1, ID = 5;

Don't use backticks when you don't have to by the way. They're extra typing, and they pretty much kill portability (for no reason -- you'll eventually have to kill portability for some reason or other).

Also, I'm not a fan of ALL_CAPS identifiers. They blend in too closely with keywords which tend to be all caps ("ID IN" looks pretty odd for example).

Perhaps your actual use of this is different, but if 14, 1, and 5 are actually hard coded, that's a bit fragile. It would be better to assign them to some kind of constant and then use that in the query. Or, if you wanted to go a step farther, you could even assign them some kind of code in addition to an int primary key ('reset', 'layout', 'colors'), then not only are they less likely to change (what if they get inserted in a different order next time? suddenly all your IDs are wrong), they're also easier to remember, and the values of your constants will have more meaning.


Appendix answer key template Goal:

Extract all Appendix Titles
If Appendix Title is Answer Key call custom solutions template
If Not Answer Key call default appendix template

XML:

<appendix>
  <title>Appendix A</title>

data

</appendix>

<appendix>
  <title>Answer Key</title>

data

</appendix>

XSLT:

<xsl:template match="appendix/title">
  <xsl:choose>
    <xsl:when test="self::node()[text()='Answer Key']">

       <xsl:call-template name="solutions"/>

    </xsl:when>
    <xsl:otherwise>
      <xsl:apply-templates select="appendix"/>
    </xsl:otherwise>
  </xsl:choose>
</xsl:template>

This template works correctly; however I am interested in gaining some ground when it comes to writing solid XSL templates. Particularly the XPATH expressions. There seem to be various ways to achieve the same results when using XPath. Is the following XPath expression best?

test="self::node()[text()='Answer Key']"

I don't see how that <xsl:otherwise> would ever do anything based on your sample xml. The template matches on appendix/title so your context would be the title element. Applying templates to appendix children of title would be a no-op. I think something like this would be closer to what you want

<xsl:template match="appendix[title = 'Answer Key']">
  <div class="answer-key">
    <xsl:call-template name="solutions"/>
  </div>
</xsl:template>

<xsl:template match="appendix">
  <!-- Do something standard -->
</xsl:template>

Here, the first template has a higher implicit priority than the second template so it will match on the "Answer Keys". If the predicate evaluates to false, the second template will then match. Note though the code above will behave differently if the called template "solutions" depends on having a title element context node as it would in your code sample. In this sample the context would be the appendix element, which seems like possibly a more appropriate point in the document hierarchy to be performing whatever operations you intend to do.


Java using PostMethod multiple times I found a example of Java PostMethod here.

But I want to post several times by using for loop. I made it like this.

import org.apache.commons.httpclient.HttpClient;
import org.apache.commons.httpclient.HttpStatus;
import org.apache.commons.httpclient.methods.PostMethod;

import java.io.BufferedReader;
import java.io.InputStreamReader;

public class PostMethodExample {

  public static void main(String args[]) {

    HttpClient client = new HttpClient();
    client.getParams().setParameter("http.useragent", "Test Client");

    BufferedReader br = null;

    PostMethod method = new PostMethod("http://search.yahoo.com/search");
    method.addParameter("p", "\"java2s\"");

    try{
            String[] parameters = { "Google", "Yahoo", "MSN" };

            for (String s in parameters){
                int returnCode = client.executeMethod(method);
                method.addParameter("p", s);
                if(returnCode == HttpStatus.SC_NOT_IMPLEMENTED) {
                    System.err.println("The Post method is not implemented by this URI");
                    // still consume the response body
                    method.getResponseBodyAsString();
                } else {
                    br = new BufferedReader(new InputStreamReader(method.getResponseBodyAsStream()));
                    String readLine;
                    while(((readLine = br.readLine()) != null)) {
                        System.err.println(readLine);
                    }
                }
                method.releaseConnection();
                method = new PostMethod("http://search.yahoo.com/search");
            }
    } catch (Exception e) {
      System.err.println(e);
    } finally {
      method.releaseConnection();
      if(br != null) try { br.close(); } catch (Exception fe) {}
    }
  }
}

I modified code just inside of try block. I don't think this is the best way. Help me to improve this code.

Avoid catching all exceptions

Do not use catch (Exception e) as this will also catch NullPointerException, ArrayIndexOutOfBoundsException and a whole lot of other ones. Only catch the Exceptions you need to catch, such as IOException. It is said that one should be specific in throws clause, but that applies for catch as well.

And as stated in the comments to your question, System.err.println(e) does not give full information about the exception. If you don't have a logger system nearby that can handle exceptions completely, at least use e.printStackTrace();. It is however recommended that you should handle and exception, and not only report it. For example by displaying a more accurate error message to the user.

Also, I would say that your line if(br != null) try { br.close(); } catch (Exception fe) {} should be on multiple lines. Even if it increases line count, it improves readability.

if (br != null) {
    try {
       br.close();
    }
    catch (Exception fe) {
    }
}


An attempt to extend an enum to prevent branching without violating the Open-Closed principle I love making utility State enums, and giving them methods that do useful things based on which instance of the enum is provided. This works when the enum is very specific to the class I'm working on, but can often make no sense when I need to switch on the enum type but I don't want the enum itself to know anything about that functionality. Further, I don't want to violate the open-closed principle in my main enum. I would prefer to avoid doing switch...case on the enum if possible.

Here's my solution that attempts to get the best of both worlds. It works (which is why I'm on CodeReview and not Programmers or SO), but it's clunky. Notice that I never have to change MyEnum again, all I do is create a new helper class each time I have a new behavior that I "wish" MyEnum had. Is there a better way?

public class EnumExample {
  public static interface OneEnumVisitor<R, A> {
    R visitFoo(A args);
    R visitBar(A args);
  }

  public static interface TwoEnumVisitor<R, A, B> {
    R visitFoo(A argOne, B argTwo);
    R visitBar(A argOne, B argTwo);
  }

  public static enum MyEnum {
    FOO {
      public <R, A> R visit(OneEnumVisitor<R, A> visitor, A args) {
        return visitor.visitFoo(args);
      }
      public <R, A, B> R visit(TwoEnumVisitor<R, A, B> visitor, A argOne, B argTwo) {
        return visitor.visitFoo(argOne, argTwo);
      }
    },
    BAR {
      public <R, A> R visit(OneEnumVisitor<R, A> visitor, A args) {
        return visitor.visitBar(args);
      }
      public <R, A, B> R visit(TwoEnumVisitor<R, A, B> visitor, A argOne, B argTwo) {
        return visitor.visitBar(argOne, argTwo);
      }
    };

    public abstract <R, A> R visit(OneEnumVisitor<R, A> visitor, A args);
    public abstract <R, A, B> R visit(TwoEnumVisitor<R, A, B> visitor, A argOne, B argTwo);
  }

  public static enum NewMethod implements TwoEnumVisitor<Integer, Integer, Integer> {
    INSTANCE;

    @Override
    public Integer visitFoo(Integer argOne, Integer argTwo) {
      return argOne.intValue() - argTwo.intValue();
    }

    @Override
    public Integer visitBar(Integer argOne, Integer argTwo) {
      return argTwo.intValue() + argOne.intValue();
    }
  }

  public static class SomeObject {
    private final MyEnum type;
    private final int param;

    public SomeObject(MyEnum type, int param) {
      this.type = type;
      this.param = param;
    }

    public int doMath(int value) {
      return type.visit(NewMethod.INSTANCE, value, param);
    }
  }

  public static void main(String... args) {
    SomeObject myObject = new SomeObject(MyEnum.FOO, 5);
    SomeObject myOtherObject = new SomeObject(MyEnum.BAR, 5);

    System.out.println(myObject.doMath(10));
    System.out.println(myOtherObject.doMath(10));
  }
}

It's really hard to tell which parts of your code are the problem you are trying to solve, and which parts are the example.  I think that's a hint that the solution has made things worse, rather than better.

Attempting to summarize your constraint - you want to use an enum to select the appropriate behavior to use at some point in the code, but you don't want to use a switch statement, and you don't want to have to modify the enum to achieve the new behavior.

If so, I think an EnumMap should serve nicely.

private static final EnumMap<MyEnum, TwoArgFunction<Integer,Integer,Integer>> lookup = new EnumMap<MyEnum, TwoArgFunction> (MyEnum.class);

static {
    lookup.put(MyEnum.FOO, new TwoArgFunction<Integer,Integer,Integer> () {
        Integer apply (Integer lhs, Integer rhs) {
            return lhs.intValue() - rhs.intValue();
        }
    ) ;

    lookup.put(MyEnum.BAR, new TwoArgFunction<Integer,Integer,Integer> () {
        Integer apply (Integer lhs, Integer rhs) {
            return lhs.intValue() + rhs.intValue();
        }
    ) ;
}

public static class SomeObject {
    ...
    public int doMath(int value) {
        return lookup.get(type).apply(value, param);
    }
}

If you really want to use a visitor based approach, you should probably refactor the abstract methods to an interface; which would allow you to extend the existing vocabulary of enums without actually needing to change MyEnum at all.

interface Visitable {
    public abstract <R, A> R visit(OneEnumVisitor<R, A> visitor, A args);
    public abstract <R, A, B> R visit(TwoEnumVisitor<R, A, B> visitor, A argOne, B argTwo);
}

public static enum MyEnum implements Visitable {
    FOO { ... },
    BAR { ... };
}

public static class SomeObject {
    private final Visitable<Integer,Integer,Integer> type;
    private final int param;

    public SomeObject(Visitable<Integer,Integer,Integer> type, int param) {
        this.type = type;
        this.param = param;
    }

    public int doMath(int value) {
          return type.visit(NewMethod.INSTANCE, value, param);
    }
}

Which in turn allows other programmers to introduce their own enums that can be used by SomeObject

public static enum HisEnum implements Visitable {
    BAZ {...}
}


Cleaning up class creation / extension I was wondering for quite some time how to clean up the below code without blowing it up any further. The extension of the classes is the main concern here, it looks a bit too much like magic. That's mainly because it has to handle all the different cases, and I haven't figured out a way to reduce the code in a meaningful fashion here.

Maybe I'm paranoid and the code is just fine, but I'd still love to get some feedback on it.

The original code and test cases are here.

function is(type, obj) {
    return Object.prototype.toString.call(obj).slice(8, -1) === type;
}
function copy(val) { /* ...make shallow copy */ }
function wrap(caller, obj) {
    obj = obj || Function.call;
    return function() {
        return obj.apply(caller, arguments);
    };
}

function Class(ctor) {
    // ...default ctor stuff here....
    function clas(args) { /* ...actual instance ctor stuff... */}

    var proto = {};
    clas.init = wrap(ctor);

    // extend needs to be reduced in width, it easily goes over 80 columns
    // without some ugly if statements
    clas.extend = function(ext) {
        if (is('Function', ext)) {
            return ext.extend(proto); // holy closure!
        }

        for (var e in ext) {
            if (!ext.hasOwnProperty(e)) {
                continue; // a bit ugly imo, but it helps to prevent the indentation
                          // from blowing up
            }

            // this needs some refactoring, it creates bound and unbound
            var val = ext[e], func = is('Function', val);
            if (/^\$/.test(e)) { // statics
                proto[e] = copy(val);
                clas[e] = clas.prototype[e] = func ? wrap(clas, val) : val;

            } else if (func) {
                clas[e] = wrap(proto[e] = clas.prototype[e] = val);
            }
        }
        return clas;
    };

    // this could also need some clean up I suppose
    for (var i = ctor.hasOwnProperty('init') ? 0 : 1,
             l = arguments.length; i < l; i++) {

        var arg = arguments[i];
        is('Object', arg) ? clas.extend(arg) : arg.extend(clas);
    }
    return clas;
}

I would suggest to add comments (Javadoc-style, but probably best if much lighter) to describe the intent of each method. Even the most simple ones. I found it especially useful in JavaScript to describe:

the type and range of arguments expected
which arguments are optional and what are the default values
the type of the result value, if any
what is the result when provided arguments do not match expectations

Otherwise I agree with your inline comment "this needs some refactoring, it creates bound and unbound", corresponding code should be extracted into a separate function, which will also reduce nesting, which seems to be one of your worries.

Regarding the code itself, I would rename wrap() to bind() to match the bind() function added in ECMAScript 5 and I would rename l to length in the for loop as letter l is easily confused with number 1.

I have doubts about this portion of the code:

if (is('Function', ext)) {
    return ext.extend(proto); // holy closure!
}

In my understanding: you check first that ext is a funciton before calling the extend() method, but:

extend() is not defined in JavaScript, it is one of your custom methods, so there is no guarantee that you will find this property on the function. You should probably add a check.
I do not understand the intent: an inline comment would help :)

All in all, I think it's fine that the code is a bit hairy because adding support for classes in JavaScript is no simple matter, but a lot more inline comments (up to one comment per line of code for the most complex stuff) would improve the code readability immensely.


Plot heat map from csv file using numpy and matplotlib There's a csv file with format:

x0, y0, v00
x0, y1, v01
...
x1, y0  v10
...

And what I want to do is to plot a heat map, in which at location (x, y) the value v is plotted with corresponding color. Below is my current implementation.

import random
import numpy as np
import matplotlib.pyplot as plt

def create_test_csv(file):
    random.seed(42)
    f = open(file, "w")
    for x in range(300):
        for y in range(600):
            value = random.randrange(255)
            f.write(str(x) + "," + str(y) + "," + str(value) + "\n")

def get_xyz_from_csv_file(csv_file_path):
    '''
    get x, y, z value from csv file
    csv file format: x0,y0,z0
    '''
    x = []
    y = []
    z = []
    map_value = {}

    for line in open(csv_file_path):
        list = line.split(",")
        temp_x = float(list[0])
        temp_y = float(list[1])
        temp_z = float(list[2])
        x.append(temp_x)
        y.append(temp_y)
        z.append(temp_z)
        map_value[(temp_x, temp_y)] = temp_z

    return x, y, map_value

def draw_heatmap(x, y, map_value):

    plt_x = np.asarray(list(set(x)))
    plt_y = np.asarray(list(set(y)))
    plt_z = np.zeros(shape = (len(plt_x), len(plt_y)))

    for i in range(len(plt_x)):
        for j in range(len(plt_y)):
            if map_value.has_key((plt_x.item(i), plt_y.item(j))):
                plt_z[i][j] = map_value[(plt_x.item(i), plt_y.item(j))]

    z_min = plt_z.min()
    z_max = plt_z.max()
    plt_z = np.transpose(plt_z)

    plot_name = "demo"

    color_map = plt.cm.gist_heat #plt.cm.rainbow #plt.cm.hot #plt.cm.gist_heat
    plt.clf()
    plt.pcolor(plt_x, plt_y, plt_z, cmap=color_map, vmin=z_min, vmax=z_max)
    plt.axis([plt_x.min(), plt_x.max(), plt_y.min(), plt_y.max()])
    plt.title(plot_name)
    plt.colorbar().set_label(plot_name, rotation=270)
    ax = plt.gca()
    ax.set_aspect('equal')
    figure = plt.gcf()
    plt.show()
    return figure

if __name__ == "__main__":
    csv_file_name = "test.csv"
    create_test_csv(csv_file_name)
    x, y, map_value = get_xyz_from_csv_file(csv_file_name)
    draw_heatmap(x, y, map_value)

Function create_test_csv() created a test csv file. Function get_xyz_from_csv_file() create x, y coordinates list and a dict which key is tuple (x,y) and value is v. Function draw_heatmap() plot the heat map using list x, y and dict map_value.

It works but I would like to know if there is some more straightforward way to this, especially the transition from CSV to the matrix that created the heat map. It might worth to notice that in my real case the coordinate may not be integer.

Since you are already using numpy, you can use numpy's loadtxt function to read in all the data at once as numpy arrays from the start.  This allows you to avoid having to worry about opening or closing files (this is done automatically), converting to numpy arrays, etc.  Then it is a simple matter of converting the indexes to values.

You can also vectorize the test data creation, using numpy's meshgrid function to get a grid of corresponding X and Y coordinates.

You can make the plotting better, in my opinion at least, by using plt.subplots() to get a figure and axes object right at the beginning, then using those to do the plotting.

So here is how I would do it:

import numpy as np
import matplotlib.pyplot as plt

def create_test_csv(fname):
    np.random.seed(42)

    # Generate X and Y coordinates
    x = np.arange(300)
    y = np.arange(600)

    # Get corresponding X and Y coordinates
    xs, ys = np.meshgrid(x, y)

    # Get random values for each location
    zs = np.random.randint(0, 255, size=xs.size)

    # Convert 3 2D arrays to 1 2D array of columns
    data = np.vstack([xs.ravel(), ys.ravel(), zs.ravel()]).T

    # Save to file
    np.savetxt(fname, data, delimiter=',', fmt='%d')

def get_xyz_from_csv_file_np(csv_file_path):
    '''
    get a grid of values from a csv file
    csv file format: x0,y0,z0
    '''

    # Load the csv file into a single 2D array,
    # then split the columns into individual variables.
    x, y, z = np.loadtxt(csv_file_path, delimiter=',', dtype=np.int).T

    # Create an empty 2D array of pixels and
    # put all the values into the correct place
    plt_z = np.zeros((y.max()+1, x.max()+1))
    plt_z[y, x] = z

    return plt_z

def draw_heatmap(plt_z):
    # Generate y and x values from the dimension lengths
    plt_y = np.arange(plt_z.shape[0])
    plt_x = np.arange(plt_z.shape[1])

    # everything is the same from here on
    z_min = plt_z.min()
    z_max = plt_z.max()

    plot_name = "demo"

    color_map = plt.cm.gist_heat #plt.cm.rainbow #plt.cm.hot #plt.cm.gist_heat
    fig, ax = plt.subplots()
    cax = ax.pcolor(plt_x, plt_y, plt_z, cmap=color_map, vmin=z_min, vmax=z_max)
    ax.set_xlim(plt_x.min(), plt_x.max())
    ax.set_ylim(plt_y.min(), plt_y.max())
    fig.colorbar(cax).set_label(plot_name, rotation=270)
    ax.set_title(plot_name)
    ax.set_aspect('equal')
    plt.show()
    return figure
    figure = plt.gcf()
    plt.show()
    return figure

if __name__ == "__main__":
    fname = 'temp.csv'
    create_test_csv(fname)
    res = get_xyz_from_csv_file_np(fname)
    draw_heatmap(res)


Class for multithreaded insert into database Here I have class that supposed to take dictionaries of product objects and their articles (kind of unique identifier) from queue and insert or update them in database table.

Received dictionary looks like this:

products_and_articles= {'products':[],'articles':[]}

Please suggest any improvements I can make to this class.

class TableUpdater(threading.Thread):

def __init__(self, import_queue, model_for_update):

    self.import_queue = import_queue
    self.model_for_update = model_for_update
    threading.Thread.__init__(self)

def run(self):
    while True:
        try:
            products_and_articles = self.import_queue.get(block=True, timeout=20)

            products_for_update = self.model_for_update.objects.filter(article__in=products_and_articles['articles'])
            articles_for_update = [product.article for product in products_for_update]

            articles_for_create = list(set(products_and_articles['articles']) - set(articles_for_update))
            absent_products = [p for p in products_and_articles['products'] if p.article in articles_for_create]
            products_for_create = []

            for new_product in products_and_articles['products']:
                for old_product in products_for_update:
                    if new_product.article == old_product.article:
                        for old_field in old_product._meta.get_all_field_names():
                            for new_field in new_product._meta.get_all_field_names():
                                if old_field == new_field and old_field != 'id' and old_field != 'slug':
                                    setattr(old_product, old_field, getattr(new_product, old_field))

            bulk_update(products_for_update)

            if len(products_for_update) != len(products_and_articles['products']):
                for new_product in absent_products:
                    old_product = self.model_for_update()
                    for old_field in old_product._meta.get_all_field_names():
                        for new_field in new_product._meta.get_all_field_names():
                            if old_field == new_field and old_field != 'id':
                                setattr(old_product, old_field, getattr(new_product, old_field))
                    products_for_create.append(old_product)

                product_bulk_create(products_for_create, 0, articles_for_create)

        except Queue.Empty:
            print 'Queue is empty'
            break

As mentioned by jcfollower, you have two sections that are deeply indented. These should be extracted into sub-methods. It will make the code easier to follow and not cause the text to bunch up at the right side of the screen.

More importantly, it is currently hard to tell what is actually happening with all the for loops and if clauses. Well named methods will provide context to the reader. When I'm looking at run(), I'm not concerned with how each filed is updated. I just want to know the main tasks that are being done. Then, once I have a high level understanding, I can dive done into the implementation details to check if the updates are done correctly and efficiently1.

Specify the queue read timeout as a constant instead of a magic number. If you find the the value needs to be tuned, it will be easier to find.

self.model_for_update.objects.filter(article__in=products_and_articles['articles'])

Is the double underscore in the argument name intentional? Python has a convention for when double underscores are used and separating words in variable names is very different from the convention.

Does product_bulk_create() put some restriction on articles_for_create such that it must be a list? Unless that is the case, using the set returned by the set subtraction should be sufficient.

products_and_articles is a odd name for a variable. You never have an operation that directly uses the whole dictionary. You are just passing references of the respective lists. Instead, you can replace the dictionary with a tuple and unpack the values directly into variables.

products, articles = self.import_queue.get(block=True, timeout=20)

1 For loops that are nested 3 and 4 deep are flags that there might be an inefficient algorithm.


Over overloaded method I am using SignalR to send notifications to clients. I use IClientProxy which is a proxy for invoking hub methods.
It has 10 overload methods for SendAsync that takes parameters one by one as you see below, however, to handle this situation I have written a spagetti style method which is really weird to me.
Could you please give some suggestion?
private async Task InvokeNotification(string userName, string methodName, params object[] parameters)
{
    if (connections.ContainsKey(userName))
    {
        var proxy = Clients.Clients(connections[userName]);
        switch (parameters.Length)
        {
            case 0:
                await proxy.SendAsync(methodName);
                break;

            case 1:
                await proxy.SendAsync(methodName, parameters[0]);
                break;

            case 2:
                await proxy.SendAsync(methodName, parameters[0], parameters[1]);
                break;

            default:
                throw new HubException($"No method was defined with {parameters.Length} parameters in Hub");
        }
    }
}

The following code block is the definition of SendAsync which is coming from SignalR
public static class ClientProxyExtensions
{
    public static Task SendAsync(this IClientProxy clientProxy, string method, object arg1, object arg2, object arg3, object arg4, object arg5, object arg6, object arg7, object arg8, object arg9, object arg10, CancellationToken cancellationToken = default);
    public static Task SendAsync(this IClientProxy clientProxy, string method, object arg1, object arg2, object arg3, object arg4, object arg5, object arg6, object arg7, object arg8, object arg9, CancellationToken cancellationToken = default);
    public static Task SendAsync(this IClientProxy clientProxy, string method, object arg1, object arg2, object arg3, object arg4, object arg5, object arg6, object arg7, object arg8, CancellationToken cancellationToken = default);
    public static Task SendAsync(this IClientProxy clientProxy, string method, object arg1, object arg2, object arg3, object arg4, object arg5, object arg6, object arg7, CancellationToken cancellationToken = default);
    public static Task SendAsync(this IClientProxy clientProxy, string method, object arg1, object arg2, object arg3, object arg4, object arg5, object arg6, CancellationToken cancellationToken = default);
    public static Task SendAsync(this IClientProxy clientProxy, string method, object arg1, object arg2, object arg3, object arg4, object arg5, CancellationToken cancellationToken = default);
    public static Task SendAsync(this IClientProxy clientProxy, string method, object arg1, object arg2, object arg3, object arg4, CancellationToken cancellationToken = default);
    public static Task SendAsync(this IClientProxy clientProxy, string method, object arg1, object arg2, object arg3, CancellationToken cancellationToken = default);
    public static Task SendAsync(this IClientProxy clientProxy, string method, object arg1, object arg2, CancellationToken cancellationToken = default);
    public static Task SendAsync(this IClientProxy clientProxy, string method, object arg1, CancellationToken cancellationToken = default);
    public static Task SendAsync(this IClientProxy clientProxy, string method, CancellationToken cancellationToken = default);
}

As for me, this code is fine. Maybe there's some Reflection dark magic available but this one will be anyway faster.
The only thing I can suggest here is optimizing out an async State Machine because there's no code after await and the only await in the method can be executed.
Also note that you lookup through the Dictionary twice, first time when you check key with .ContainsKey, second time when you get the value with the same key. It can be done with single lookup using Dictionary.TryGetValue() method.
Also I suggest to follow async naming policy suggested by Microsoft, and rename the method to InvokeNotificationAsync.
private Task InvokeNotificationAsync(string userName, string methodName, params object[] parameters)
{
    if (connections.TryGetValue(userName, out var connection))
    {
        var proxy = Clients.Clients(connection);
        switch (parameters.Length)
        {
            case 0:
                return proxy.SendAsync(methodName);
                break;

            case 1:
                return proxy.SendAsync(methodName, parameters[0]);
                break;

            case 2:
                return proxy.SendAsync(methodName, parameters[0], parameters[1]);
                break;

            default:
                throw new HubException($"No method was defined with {parameters.Length} parameters in Hub");
        }
    }
}

Also it can look a bit better with C# 8.0 syntax
private Task InvokeNotificationAsync(string userName, string methodName, params object[] parameters)
{
    if (connections.TryGetValue(userName, out var connection))
    {
        var proxy = Clients.Clients(connection);
        return parameters.Length switch
        {
            0 => proxy.SendAsync(methodName),
            1 => proxy.SendAsync(methodName, parameters[0]),
            2 => proxy.SendAsync(methodName, parameters[0], parameters[1]),
            _ => throw new HubException($"No method was defined with {parameters.Length} parameters in Hub")
        };
    }
}


Working on key-value disk cache written in plain C. Is there any errors? I'm an objective-c developer. But i'm getting familar with C. I wrote it for my own develpment in C. Please point on some errors!

The point is to store/read bytes in file named "key" in cachePath directory.

Cache.h

typedef const struct Cache *CacheRef;

/* Returns cache reference for corresponding cache path */
/* NOTE: cache path should exist */

CacheRef CacheAtPath(const char *cachePath);

/* Sets bytes for key */
/* Returns 0 if failed */

int SetBytesForKey(CacheRef cache, const void *bytes, unsigned long length, const char *key);

/* Gets bytes for key */
/* Returns 0 if failed */

int GetBytesForKey(CacheRef cache, void **bytes, unsigned long *length, const char *key);

Cache.c

#include "Cache.h"
#include <stdio.h>
#include <stdlib.h>
#include <string.h>

struct Cache {
    char *path;
};

char *_FilePathForKey(CacheRef cache, const char *key);
FILE *_FileForKey(CacheRef cache, const char *key, const char *access);

char *_FilePathForKey(CacheRef cache, const char *key){
    size_t pathLength = strlen(cache->path) + strlen(key);
    char *path = malloc(pathLength);
    strcpy(path, cache->path);
    strcat(path, key);

    return path;
}

FILE *_FileForKey(CacheRef cache, const char *key, const char *access)
{
    char *filePath = _FilePathForKey(cache, key);
    if (filePath == NULL) return NULL;

    FILE *file = fopen(filePath, access);

    free(filePath);

    return file;
}

CacheRef CacheAtPath(const char *cachePath){
    struct Cache *cache = malloc(sizeof(struct Cache));
    cache->path = malloc(strlen(cachePath));
    strcpy(cache->path, cachePath);
    return (CacheRef)cache;
}

int SetBytesForKey(CacheRef cache, const void *bytes, unsigned long length, const char *key)
{
    if (cache == NULL) return 0;

    FILE *file = _FileForKey(cache, key, "wb");

    if (file == NULL) return 0;

    int result = (int)fwrite(bytes, length, 1, file);

    fclose(file);

    return result;
}

int GetBytesForKey(CacheRef cache, void **bytes, unsigned long *length, const char *key)
{
    void *buffer = NULL;
    size_t bufferSize = 0;

    FILE *file = _FileForKey(cache, key, "rb");

    if (file == NULL) return 0;

    fseek(file, 0, SEEK_END);
    bufferSize = ftell(file);
    fseek(file, 0, SEEK_SET);

    buffer = malloc(bufferSize);

    int result = 0;
    if (buffer) {
        result = (int)fread(buffer, bufferSize, 1, file);
        if (result) {
            *length = bufferSize;
            *bytes = buffer;
        } else {
            *length = 0;
            *bytes = NULL;
        }
    }
    fclose(file);

    return result;
}

size_t pathLength = strlen(cache->path) + strlen(key);
char *path = malloc(pathLength);
strcpy(path, cache->path);
strcat(path, key);

Try this instead:

size_t path_len = strlen(cache->path);
size_t key_len = strlen(key);
size_t total_len = path_len + key_len + 1;

char *path = malloc(total_len);
if (!path) { /* TODO: handle error */ }

mempcy(path, cache->path, path_len);
memcpy(path + path_len, key, key_len + 1);

This is better for the following reasons:

Your malloc call does not account for the extra 0 byte (also called NUL terminator) that must end C strings.  You repeat this mistake in a bunch of other places.  (To reiterate: if your string is size len, allocate len + 1 and set the last byte to 0.)
strcat will recompute the length of the string at runtime, by searching for that NUL character.  This is bad.  Every concatenation will lead to another traversal, leading to an O(n2) kind of curve when concatenating lots of strings.  I see the use of strcat in pretty much any code as a sort of red flag.
malloc can fail.  Check your return codes.  A large part of writing good C code is being able to answer the question: "how does this code fail?"  I feel like a lot of higher-level coders (usually Java and C#, but these days also objC) seem to fail to grasp this and code rather "optimistically" instead.

Error handling in C looks very ugly when done poorly, but if there is a general approach to coding style it can work elegantly..  The trick I like to use is to make your failure paths mostly the same as your success cases.  This typically means that there's a part of each function that does "cleanup" - deallocates everything you've allocated, releases whatever's been acquired, etc.  Kind of like the RAII pattern in C++ where your locally-declared quantities will do their cleanup in a destructor.  This would get run regardless of success or failure.  If some allocation or resource acquisition outlives the function, then make the success case be the special case, because this will ultimately allow you to make more fluid code modifications vis-a-vis complex error handling.  In case I'm writing too abstractly, an example of this would be:

void *frob(int num_frobs)
{
   void *buf = malloc(/* ... */);
   void *r = buf;   /* Let's say this will be non-null on success cases. */

   if (r && !do_frobbing(buf, num_frobs))
   {
      /* do_frobbing failed! */
      r = NULL;
   }

   if (r && !additional_frobs(buf))
   {
      /* more failure! */
      r = NULL;
   }

   if (r)
   {
      /* success, make sure not to free the buffer on the way out */
      buf = NULL;
   }

   /* clean up*/
   free(buf);

   return r;
}

OK, that's a really goofy-looking example...  But here you see that these blocks inside if (r) are really composable.  In between each of them you can add and remove more operations that can potentially fail, and your cleanup block handles releasing resources in event of either success or failure.  If you're working in C++ this would be done via RAII wrappers, i.e. in destructors of classes and possibly with throw or an early return.  But C has no such features to make that easy and correct...

Moving on to other code:

return (CacheRef)cache;

This cast is suspicious.  If I read your typedef correctly, all this does is adds an extra const.  This means the cast is totally unnecessary.  (Another general rule for anal-retentive types such as myself, by the way: casts are suspicious, especially with pointer types.  If you can make your code work without casts it's always better.)  I would say in general you don't need this typedef.

 FILE *_FileForKey(CacheRef cache, const char *key, const char *access)

Technically the C standard says you can't name things with leading underscores.  In practice your compiler will let you get away with it anyway.  But personally I think it looks weird.  If your intention is to hide internal functions from the outside would I suggest you declare them with static and/or add some prefix to their names.

Speaking of names, this brings me to another point...  I would work on better and consistent naming conventions.  Overall I would strive to have someone be able to read the header, as well as code that calls your library, and have some clue what the functions do.  In C it's common to prefix functions from a given library or module with some common name, i.e. having to do with the type.


Horizontal accordion using jQuery I am trying to create a horizontal accordion using jQuery. The code works fine, but I just want to know if my approach is correct and if there are any improvements I could make to the existing code.

$(document).ready(function () {
$("ul li").children(".Acc_Content").animate({ width: '0px' },{ duration: 1000, queue: false }).addClass("remove");
$(".Acc_Content").first().animate({ width: '600px' }, { duration: 1000, queue: false }).removeClass("remove").addClass("add");
$(".Acc_Name").click(function () {
$(".Acc_Content").animate({ width: '0px' }, { duration: 1000, queue: false }).removeClass("add").addClass("remove");
$(this).next(".Acc_Content").animate({ width: '600px' }, { duration: 1000, queue: false }).removeClass("remove").addClass("add");
});
});
body{font-family: Arial;}
ul{list-style: none;}
.Acc_Name{background-color: #808080;color: white;border:1px solid #000;margin:0px 1px;height:150px;width:30px;float:left;}
.Acc_Name span{margin:5px;float:left;line-height:20px;font-weight:bold;
-moz-transform:rotate(270deg);
-webkit-transform:rotate(270deg)}
.Acc_Content{float:left;height:150px;border:1px solid #000;}
.add{display:block;}
.remove{display:none;}
p{margin: 10px;}

    One

Hello Test1

    Two

Hello Test2

    Three

Hello Test3

    Four

Hello Test4

First, your JS and CSS could do some indenting. It's barely readable.

Now, seeing that you use transforms and you use a 2.x.x version of jQuery, I can assume you're targeting a modern browser. Instead of using jQuery's animate, you can use CSS transitions on width instead. Trigger your accordion by adding and removing classes that change the value of width. Aside from lesser JS, CSS animations are mostly hardware accelerated. This, in simple terms, mean smoother animations.

If you have the luxury of a build step, I suggest offloading the task of adding prefixes to your CSS to a task runner. Grunt or Gulp, with Autoprefixer should be able to help you.

Additionally, I would recommend being uniform with CSS colors. If you write hex most of the time, make sure they're all in hex and avoid color names. If you mostly do 6-digit hex, then write in 6-digit hex and avoid the 3 digit ones. If you're worried about file size, let a compressor task like cssmin do it for you.


Node JS delete multiple MySQL linked records and delete physical file I have the following tables in MySQL:
create table PostSchema
(id int not null primary key auto_increment,
categories int not null,
title varchar(50),
status varchar(20),
comments bool,
body varchar(255),
filename varchar(255),
date_created TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP);

create table commentSchema (
id int not null primary key auto_increment,
userID int,
message varchar(255),
image varchar(255),
date_created TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP,
postID int);

Currently..
When a post is created it saves an image in the '/uploads' directory. Path name is referenced in table PostSchema column name filename.
Next, when a comment is created with an image attached it is also saved in '/uploads' directory and the actual path name is referenced in commentSchema in column image.
My Idea
I have a router set up to delete the records from PostSchema and any comments are relevant to PostSchema. On top of that, I'll implement a unlink function to remove any images referenced in the both tables that are held in the /uploads directory.
I have a working code, just want to hear some reviews if there is anywhere I can improve :)
Current working code:
router.delete('/:id', (req, response)=>{
  // Execute query for deleting the post

    // Get filename from database
    db.query("SELECT filename FROM PostSchema WHERE id = ?", req.params.id,(err,fileResult)=>{
      // If error
      if(err){
        return res.status(500).end(err.message);
      }
      // If not an error continue...
      else{

        try {

          // Unlink comments images
          db.query("select image from commentschema where postID = ?", req.params.id, (cErr, cRes)=>{
            if(cErr){
              return res.status(500).end(err.message);
            }
            else{
              if(fs.existsSync(uploadDir + cRes[0].image)){
                fs.unlink(uploadDir + cRes[0].image, (commentFile) => {
                  if (commentFile) throw err;
                });
              }
            }
          });

          // Unlink PostSchema images
          fs.unlink(uploadDir + fileResult[0].filename, (errs)=>{
            // Delete PostSchema + Delete comments
            db.query("delete t1, t2 from postschema t1 join commentschema t2 on t1.id = t2.postid where t1.id = ?;", req.params.id,(errors, results)=>{
              if(errors){
                console.log(errors);
              }
              else{
                db.query("delete from postschema where id = ?", req.params.id, (er, res)=>{
                  if(errors){
                    console.log(errors);
                  }
                  else{
                    // Redirect to all posts page
                    req.flash('success_message', 'Post was succesfully deleted');
                    response.redirect(303, '/admin/posts');
                  }
                });
              }
            });
        });
      }
      catch (e) {
        return res.status(500).end(e.message);
      }
    }
  });
});

I am not primarily a JS developer, therefore I won't assist you with the JavaScript part itself, but I can give you a few hints regarding several issues I've found.
Lack of transactional safety
In your example, you're executing two DELETE statements on the database.
1) delete t1, t2 from postschema t1 join commentschema t2 on t1.id = t2.postid where t1.id = ?

2) delete from postschema where id = ?

Those statements are isolated in their own transactions and commit right after their execution. The problem here is, if the statement 1) succeeds, but 2) fails, all the records deleted with the statement 1) will remain deleted, which can easily lead to unexpected database state.
In order to prevent this, you want to wrap the statements which should either all succeed or all fail in a database transaction.
DELETE statement ambiguity and difficult readability
Although MySQL supports deleting records through JOIN, effectively allowing you to delete data from multiple tables in a single go, such statements are difficult to track what gets actually deleted. On top of that, certain database engines do not support such mechanism at all.
I highly recommend you to change the statement, so that it deletes only the comments, which makes the query much easier to follow and to read.
DELETE FROM commentschema WHERE postid = ?

Removing a file from the FS before removing a database record
This may introduce an unintentional side effect, if deletion of database records fails, leading to a situation where the database record points to a file location which no longer exists.
In order to solve this, the order of operation should be swapped:

delete data from the database,
(now that nothing points to the file) delete the file from the file system.

I have also noticed you're loading images for comments (plural!) by postId, but only removing the image of the first comment in the returned collection.
This may be an accidental error in your implementation, since later you're deleting all comments, which makes me believe you want to delete all images, too.
Code nesting
Your code is quite nested, due to callbacks. In order to resolve this, you can:

introduce smaller functions with different responsibilities (e.g. a function to load data from the database, a function to delete a file,...),
convert functions which output result through callbacks to promises and cleanup the code a little bit through promise-chaining (even better if you use async/await).


Anagram finder in F# I've been learning F# for a month or so and I'm wondering how good my "functional" aspect of coding is. When I first started, I did this using an iterative approach with a lot of <- and mutables etc. I tried to rewrite it as functional as possible. Any critique is appreciated.

This code prints out all anagrams in the english dictionary read from a "word.txt" file, the LONGEST anagram, and the anagram with the MOST permutations.

open System

let sortStringAsKey (listOfWords : string array) =
    let getKey (str : string) =
        str.ToCharArray()
        |> Array.sort
        |> String

    listOfWords
    |> Array.groupBy getKey
    |> Array.filter (fun (sortedWord, originalWord) -> originalWord.Length > 1)

[<EntryPoint>]
let main argv =
    let filename = "words.txt"
    let listOfWords = System.IO.File.ReadAllLines(filename)
    let listOfAnagrams = sortStringAsKey listOfWords

    // Prints every single anagram combination
    listOfAnagrams |> Array.iter  (fun (_ , anagramList) ->  anagramList |> Array.iter (fun str -> printfn "%s" str); printfn "")

    // Gets the LONGEST anagram
    let longestAnagrams = listOfAnagrams
                        |> Array.filter (fun (sortedWord, _) -> sortedWord.Length >= (listOfAnagrams
                                                                                |> Array.maxBy (fun (sortedWord, _)-> sortedWord.Length)
                                                                                |> fst
                                                                                |> String.length))

    // Gets the set that has the MOST anagrams
    let mostAnagrams = listOfAnagrams
                        |> Array.filter (fun (_, originalWords) -> originalWords.Length >= (listOfAnagrams
                                                                                |> Array.maxBy (fun (_, originalWords) -> originalWords.Length)
                                                                                |> snd
                                                                                |> Array.length))

    // Prints the longest and most anagrams
    longestAnagrams |> Array.iter  (fun (_ , anagramList) ->  anagramList |> Array.iter (fun str -> printfn "%s" str); printfn "")
    mostAnagrams |> Array.iter  (fun (_ , anagramList) ->  anagramList |> Array.iter (fun str -> printfn "%s" str); printfn "")

    0 // return an integer exit code

This looks like a good start, so the following is by no means a criticism, but there are various refactorings you can perform to make the code smaller, and more generic.

As a general comment, I'd advise against naming arrays listOfWhatever, since list is a concrete data type in F#, separate from arrays. I've renamed listOfWords to words, listOfAnagrams to anagrams, and so on.

Finding the anagrams

The first sortStringAsKey function doesn't need type annotations if you refactor it a bit:

// seq<'a> -> seq<string * seq<'a>> when 'a :> seq<char>
let sortStringAsKey words =
    words
    |> Seq.groupBy (Seq.sort >> Seq.toArray >> String)
    |> Seq.filter (fun (_, originalWords) -> Seq.length originalWords > 1)

You'll notice that I inlined the getKey function, but perhaps that's taking it too far. If you think that having a named local function makes the code more readable, I wouldn't disagree.

I've also changed from working explicitly with arrays, to using the Seq module. This makes the function more generic, but it can still handle arrays.

You could even perform an eta reduction on the function, in order to make it even shorter, but I'm not sure it becomes more readable by it:

let sortStringAsKey =
    Seq.groupBy (Seq.sort >> Seq.toArray >> String)
    >> Seq.filter (fun (_, originalWords) -> Seq.length originalWords > 1)

Just for the fun, you can make it even more cryptic:

let sortStringAsKey =
    Seq.groupBy (Seq.sort >> Seq.toArray >> String)
    >> Seq.filter (snd >> Seq.length >> ((<) 1))

Personally, I don't even find that readable myself; I'd prefer the first, most verbose option.

Printing the anagrams

Since there's at least three places where the code prints out the anagrams, it's more reasonable to turn that into a function:

// seq<string> -> unit
let prints a =
    a |> Seq.iter (printfn "%s")
    printfn ""

Once again, you'll notice that I chose to use Seq.iter instead of Array.iter. It'll still be able to handle arrays, but there's no reason to constrain the input if it isn't necessary.

Loading anagrams

Loading the anagrams from file can be simplified a bit as well, since you don't need the intermediate listOfWords value:

// In main function:
let filename = "../../words.txt"
let anagrams = System.IO.File.ReadAllLines(filename) |> sortStringAsKey

Because of the more generic version of sortStringAsKey, the type of anagrams is seq<string * seq<string>>.

Printing all the anagrams

With the prints function, you can easily print all the anagrams

anagrams |> Seq.map snd |> Seq.iter prints

Instead of performing work inside on of Seq.iter (which is possible), I often prefer to perform transformations etcetera first, because I can easily test such pure functions. Once you have data in the appropriate shape, you can always use Seq.iter to e.g. print it.

Finding the longest anagrams

The proposed solution suffers from calling Array.maxBy for every element, so it'd be quite inefficient.

You can make it more efficient by only doing it once:

let longestAnagrams =
    let longest = anagrams |> Seq.map (fst >> String.length) |> Seq.max
    anagrams |> Seq.filter (fst >> String.length >> ((=) longest))

This still requires two traversals of the array, so isn't as efficient as it could be, but is probably (but measure instead of assume, when it comes to performance) more efficient than doing Array.maxBy for every entry.

See below for an optional refactoring.

Finding the words with most anagrams

Likewise, you can find the largest collections of anagrams:

let mostAnagrams =
    let most = anagrams |> Seq.map (snd >> Seq.length) |> Seq.max
    anagrams |> Seq.filter (snd >> Seq.length >> ((=) most))

Notice how this is similar to longestAnagrams.

Printing

Both longestAnagrams and mostAnagrams can be printed using the prints function:

longestAnagrams |> Seq.map snd |> Seq.iter prints
mostAnagrams |> Seq.map snd |> Seq.iter prints

Alternative max and filter operation

The problem with even the refactored version of longestAnagrams is that it requires two traversals of the sequence in order to compute the results.

Once you realise that the operation is essentially a fold, you may want to optimise it to a single traversal:

let longestAnagrams =
    let folder l (k, v) =
        let xl = Seq.length k
        match xl, l with
        | _, []                                  -> [k, v]
        | x, (hk,  _)::_ when xl > Seq.length hk -> [k, v]
        | x, (hk, hv)::t when xl = Seq.length hk -> (k, v)::(hk, hv)::t
        | _                                      -> l
    anagrams |> Seq.fold folder []

As you can tell, it's more code (so more complicated), but at least in theory more efficient, as it only uses a single traversal. It does, however, potentially causes more memory allocations, so as always when performance is involved: measure.

On my machine, though, it seems to be more than twice as fast...

You can refactor the computation of mostAnagrams in the same way, but I will leave that as an exercise ;)


Optimize insertion from ArrayList to HashMap I'm trying to insert data from ArrayList to HashMap<String, Language> optimally.
Many items may have the same languge_name (code below), so I need to group items having the same language in Language class and store languages in a HashMap with the name of the language as a Key.
Item
String name;
String language_name;

Language
String language_name;
int numberItems;
LinkedList<String> Items;

I solved this as follows:
        ArrayList<Item> items; // given array of items
        HashMap<String, Language> languages = new HashMap<String, Language>();

        items.forEach(item -> {
            /** case 1: language isn't specified */
            if (item.getLanguageName() == null) {
                item.setLanguageName("unknown");
            }
            /** case 2: language already added */
            if (languages.containsKey(item.getLanguageName())) {
                languages.get(item.getLanguageName()).getItems().add(item.getName());
                languages.get(item.getLanguageName())
                        .setNumberItems(languages.get(item.getLanguageName()).getNumberItems() + 1);
            } else {
                /** case 3: language isn't added yet */
                LinkedList<String> languageItems = new LinkedList<String>();
                languageItems.add(item.getName());
                Language language = new Language(item.getLanguageName(), 1, languageItems);
                languages.put(item.getLanguageName(), language);
            }
        });

Any help would be appreciated!

The Language class needs the "addItem()" method that combines incrementing numberItems and adding to the Items collection. The design intent here would be to hide the implementation details of adding an item.

The Language class needs a constructor for the first item added.
 Language language = new Language(item);

The languages map should be a specialization class of the HashMap, with its "add(item)" method to hide details of the new/existing item logic.


Recursive a+b function I am practising recursion and have written the small code for summing a+b as below:

#include <stdio.h>

int b=6;
static int cnt;

void main(void)
{
    int a=9,sum;

    sum=succ(a);

    printf("Sum returned : %d\n",sum);
}

int succ(x)
{
    cnt++;
    return(cnt<=b ? succ(x+1) : x++);
}

I am getting the results fine using this. Can anyone suggest optimization or better code for this? I also want to  understand how stacking happens in this case for variables cnt and x. Are they stacking in pairs like 0-9, 1-10 ,2-11,3-12 ,4-13, then at last when cnt is 5 it return x from the post-increment return?

First of all, main should be declared int main.

Second, I would advise passing the cnt (which I have called iteration_number) as a parameter as well. Also, you should avoid using a global (b). If you define a helper function, that won't change the signature of your function:

int add_helper(int value, int max, int iteration_number)
{
    if (iteration_number >= max) return value;

    return add_helper(value + 1, max, iteration_number + 1);
}

int add(int a, int b)
{
    add_helper(a, b, 0);
}

Note that I have refactored your b to be a parameter, which I have called max. The function can be used like this:

int main(void)
{
    int start = 9; // Your a
    int count = 6; // Your b

    int sum = succ(start, count);
}

(If you are using C89 instead of C99 or C11, there should be a return 0; at the end of main()).

However, the "good" way to define addition recursively is by increasing one number and decreasing the other:

int add(int a, int b)
{
    if (b == 0) return a;

    return add(a + 1, b - 1);
}

Modifying the function to work with negative numbers is left as an exercise to the reader.

(Note: Normally you want to subtract from the lowest number and add to the largest. I left that out to keep the example compact.)

  I also want to understand how stacking happens in this case for variables cnt and x.

By "stacking", I assume you mean how the variables are pushed onto the stack as the function is called. Your cnt has static linkage, and is not pushed onto the stack at all. In my example, value, max and iteration_number will be pushed onto the stack for each call. (Exactly how is, as far as I know, implementation defined and depends on the calling convention in use.) In other words, assuming no optimization, the arguments will take iteration_number * sizeof(int) bytes on the stack.

If you want to keep track of the values, simply print them in the recursive function like this:

int succ_helper(int value, int max, int iteration_number)
{
    if (iteration_number >= max) return value;

    printf("%d: %d - %d\n", iteration_number, value, max);

    return succ_helper(value + 1, max, iteration_number + 1);
}

  Can anyone suggest optimization or better code for this?

int sum = 9 + 6;

:-)