Is this a proper singly linked list? /*
 * LinkedList.h
 *
 *  Created on: Mar 9, 2014
 *      Author: steven
 */

#ifndef LINKEDLIST_H_
#define LINKEDLIST_H_
#include <iostream>
using std::cout;
using std::endl;

template<class T>
class LinkedList {

    template<class E>
    struct Node {
        Node<E> *rightNode;
        E data;
        Node() {
            rightNode = NULL;
        }
    };
private:
    Node<T> *startNode;
    int size;
public:
    LinkedList() {
        startNode = NULL;
        size = 0;
    }
    void pushFront(T data) {
        if (size > 0) {
            Node<T> *temp = new Node<T>();
            temp->data = data;
            temp->rightNode = startNode;
            startNode = temp;
            size++;
            return;
        }
        pushBack(data);
    }
    void pushBack(T data) {
        if (size > 0) {
            Node<T> *temp = startNode;
            for (int i = 0; i < size - 1; i++) {
                temp = temp->rightNode;
            }
            temp->rightNode = new Node<T>();
            temp->rightNode->data = data;
            size++;
            return;
        }
        startNode = new Node<T>();
        startNode->data = data;
        size++;

    }

    T get(int num) {
        if (num > 0) {
            Node<T> * temp = startNode; //1
            for (int i = 0; i < num - 1; i++) {
                temp = temp->rightNode;
            }
            T tempData = temp->rightNode->data;
            delete temp;
            return tempData;
        } else {
            return startNode->data;
        }
    }
    void removeBack() {
        if (size > 2) {
            Node<T> *temp = startNode;
            for (int i = 0; i < size - 2; i++) {
                temp = temp->rightNode;
            }
            delete temp->rightNode;
            temp->rightNode = NULL;
        } else if (size == 2) {
            delete startNode->rightNode;
            startNode->rightNode = NULL;
        } else {
            delete startNode;
            startNode = NULL;
        }
        size--;

    }
    void printLinkedList() {
        Node<T> *temp = startNode;
        cout << temp->data << endl;
        for (int i = 0; i < size - 1; i++) {
            temp = temp->rightNode;
            cout << temp->data << endl;
        }
    }
    int getSize() {
        return size;
    }
    virtual ~LinkedList() {
        if (size > 2) {
            Node<T> *temp = startNode;
            while (temp->rightNode->rightNode != NULL) {
                Node<T> *currentTemp = temp;
                temp = temp->rightNode;
                delete currentTemp;
            }
            return;
        }
        if (size == 1) {
            delete startNode;
            return;
        }
        if (size == 2) {
            delete startNode->rightNode;
            delete startNode;
            return;
        }

    }
};

#endif /* LINKEDLIST_H_ */

Please critique the singly linked list, specifically the working aspect of it (making sure deleting correctly, etc.).

When possible, use an initializer list:

Node() : rightNode()
{ }

I don't think you're using C++11, but in case you are, use nullptr.

Giving Node a full constructor would be very advantageous for simplifying a lot of boilerplate code.

For example, everything like:

Node<T> *temp = new Node<T>();
temp->data = data;
temp->rightNode = startNode;

Could become a one liner:

Node* temp = new Node(data, startNode);

Also, a proper constructor avoids the exception safety problem that user1703394 mentioned.

Just for reference, the Node with a constructor would look like:

struct Node {
    T data;
    Node *rightNode;

    Node(const T& val, Node* rightNode) : data(val), rightNode(rightNode)
    { }
};

Note that the member order was changed since it needs to match the initialization list order. Changing the order allowed the constructor to be more natural (since the constructor argument order should match the initialization list order).

In C++, it's much more common to see T* var rather than T *var. It's just a stylistic qualm, but something to be aware of.

You should have blank lines between your methods.

A non-constant operator shouldn't exist on a list. If you can't provide it in constant time, don't provide it. It's very non-standard, and it's misleading to a consumer of the class. Note that the idiomatic way to expose the data is via a forward iterator.

There's a grave lack of understanding of memory ownership in get. In particular, unless you want the ownership of the node to end, you should not be freeing it.

As a general piece of advice, whenever you have doubts about memory management, consider using something like valgrind.

Try to simplify your code down to a general case rather than having lots of special cases. As others have noted, a sentinel node can heavily simplify this. As a random note, if you do use a sentinel node, and you're concern about performance, try to not dynamically allocate it. That saves you an allocation on an empty list.

Is LinkedList actually ever going to be extended (as implied by the virtual destructor)? Considering all of the properties are private and none of the methods (other than destructor) are virtual, there's not really a meaningful way that you can extend it.

Don't use the implicit private visibility of classes. Everything should be in either a public, private, or protected block. (There's nothing technically wrong with it of course, it's just good to be consistent.) Note that this doesn't apply to structs. Structs are almost always only public, so there's no potential confusion.

Be mindful of what can cause copies. In particular, get can cause an extra copy for no reason. T tempData = temp->rightNode->data; creates a copy, and then returning could, in theory--though never in practice--create a copy. If you just return directly, you can avoid a possible copy (though one will still have to happen).

Better yet, should get really be returning a copy? You could be returning either a reference or const reference (depending on if either a const or non-const version of get is called) so that the caller can modify the data, or so that a needless copy can be avoided in the read-only situation.

Your header stuff should have more spacing in it:

#ifndef LINKEDLIST_H_
#define LINKEDLIST_H_

#include <iostream>

using std::cout; // These should not be here!
using std::endl;

#endif /* LINKEDLIST_H_ */

The comment is just useless noise unless you have a lot of nested directives. Considering you only have one, this comment is useless. Also, most modern IDEs make these types of comments useless even if there are a ton of directives since they can highlight the counterpart. (I'm of the school of thought that even without highlighting, if there's enough levels for the comments to be necessary, something is very, very wrong with the the structure of the code.)

There's not a technical need to do so, but it's nice to try to be consistent with the standard library. This allows someone who is using your class for the first time to very intuitively pick up it's use. For example, I would expect size() rather than getSize(). I also might use pop instead of remove.

Sometimes an else clause is less confusing than a hidden return, especially in void functions. For example, the following is much clearer to me:

void pushFront(T data) {
    if (size > 0) {
        Node<T> *temp = new Node<T>();
        temp->data = data;
        temp->rightNode = startNode;
        startNode = temp;
        size++;
    } else {
        pushBack(data);
    }
}

I actually had to stop and pause for a long moment the first time I read through your code to make sure you weren't adding twice.

There are a lot of const correctness issues in your code. Please look into it. The short explanation is that any time something is only being read, it should be accepted by const reference.

Consider having each node own it's rightNode. This would mean that instead of a looping destructor, you could simple delete the head node and let the deletion cascade. This would work particularly well with a automatically allocated sentinel node since it would mean you could have a default destructor in the list.

Sized based looping on a linked structure is rather rare. The standard way to do it is to loop on the pointers. In the situations  you've used it though, the uses are arguably just as clear as the pointer case.

Consider keeping a tail pointer. That lets you push and pop to/from the end in constant time.


Print max no of recent duplicate words I am new to C++, learning from the book primer C++

The exercise:

Write a program to read strings from standard input looking for duplicated
words. The program should find places in the input where one word is followed
immediately by itself. Keep track of the largest number of times a single repetition
occurs and which word is repeated. Print the maximum number of duplicates, or
else print a message saying that no word was repeated.

vector<string> words;
string w, highest_occ_word;
int occ = 1, highest_occ = 0;

while (cin >> w)
    words.push_back(w);

for (auto i = words.begin(); i != words.end(); ++i)
{
    if ((i + 1) != words.end() && *i == *(i + 1))
    {
        ++occ;
        if (highest_occ < occ)
        {
            highest_occ = occ;
            highest_occ_word = *i;
        }
    }
    else
        occ = 1;
}

cout << highest_occ_word << " occured for " << highest_occ << " times \n";

Is this good code? If not, what can I do to improve it?

You loop from the first element to the last, and in each iteration you check if a next element exists or not. This check will only be false on the last element, it will be true for all elements before that. In other words, most of the time the check is unnecessary.

To get rid of this redundancy, you could start the iteration from the second element, and make comparisons with the previous one, which always exists. No more redundant checks.

More importantly, keep in mind that you don't need to read the entire input into memory to find the most frequent repetition. You don't need a vector. You could just parse the input word by word, always keeping only the last word in memory for comparisons. That way you will drastically reduce the memory footprint of the program.


Using inputted data from vector to create two outputs I'm trying to figure out a way to do the exercise 3.20 in the C++ primer book

I have to create a program that reads input into a vector then use that data to create 2 outputs:

one is the sum of each adjacent number
one is the sum of the first and last number moving inwards (as in the 2nd and 2nd last, 3rd and 3rd last etc)

I've created the following code, but it feels off somehow.  Can I simplify/clean up this stuff a bit?

#include <vector>
#include <string>
#include <iostream>
#include <cctype>

using std::cin; using std::cout; using std::endl; using std::vector; using std::string;

int main()
{
    vector<int> nv;
    int n;

    while (cin >> n)
    {
        nv.push_back(n);
    }

    int cnt = 0;
    int output = 0;
    int oldnum = 0;

    //output sum of adjacent
    for (int cn : nv)
    {
        output += cn;
        output += oldnum;
        ++cnt;

        if (cnt == 2)
        {
            cout << output << " ";
            cnt = 1;
            output = 0;
            oldnum = cn;
        }

    }
    cout << endl;

    //output sum of first and last
    int trigger = 1;
    for (decltype(nv.size()) i = 0; i < (nv.size()/2); ++i)
    {
        cout << (nv[trigger - 1]) + (nv[nv.size() - (1 * trigger)]) << " ";
        ++trigger;
    }
    return 0;
}

Although there's room for argument that it might be an abuse, one way you could do the adjacent sums part of this would be to use std::adjacent_difference, but pass it a function that adds instead of subtracts:

std::adjacent_difference(inputs.begin(), inputs.end(),
    std::ostream_iterator<int>(std::cout, "\t"),
    std::plus<int>());

Note, however, that the first result you get from this will be just the first item, not added to anything. Each subsequent value in the result will be the sum of the current and previous items.

You might consider, instead, implementing an adjacent_sum that produces the result you want a little more directly:

template <class InIt, class OutIt>
OutIt adjacent_sum(InIt begin, InIt end, OutIt result) {
     ++begin;
     while (begin != end) {
          *result = *begin + *(begin-1);
          ++result;
          ++begin;
     }
     return result;
}

// ...

adjacent_sum(inputs.begin(), inputs.end(),
    std::ostream_iterator<int>(std::cout, " "));

For the second part, I'd probably use std::transform, supplying forward iterators for the first range, and a reverse iterator for the beginning of the second range:

std::transform(inputs.begin(), inputs.begin()+inputs.size()/2,
    inputs.rbegin(),
    std::ostream_iterator<int>(std::cout, " "),
    std::plus<int>());


Sychronous wrapper around UIApplication.openURL:options:completion: I am deprecating iOS 9 on a project. One of the differences in the framework is that the openURL method has changed.

iOS 9

- (BOOL)openURL:(NSURL *)url;

iOS 10

- (void)openURL:(NSURL *)url
         options:(NSDictionary<NSString *,id> *)options
completionHandler:(void (^)(BOOL success))completion;

Notice how the method changed from being synchronous to asynchronous.

Since I have a bunch of usages for the synchronous method, I want to create a synchronous wrapper around the asynchronous openURL:options:completion: method and I'm going to update the usages to call this asynchronous wrapper instead.

My first attempt looks like this:

- (BOOL)sync_openURL: (nullable NSURL *)url {
    __block BOOL result = nil;
    dispatch_semaphore_t semaphore = dispatch_semaphore_create(0);
    [self openURL:url options:@{} completionHandler:^(BOOL success) {
        result = success;
        dispatch_semaphore_signal(semaphore);
    }];
    while (dispatch_semaphore_wait(semaphore, DISPATCH_TIME_NOW)) {
        [[NSRunLoop currentRunLoop] runMode:NSDefaultRunLoopMode beforeDate:[NSDate dateWithTimeIntervalSinceNow:0]];
    }
    return result;
}

Is this a good replacement ? Can it be improved in anyway ?

That code should work, but blocking the main thread is never a good idea. That's why the method was changed in the first place.

I suggest a deeper refactor of your code so that you use the new async method with completion handler as intended.


Creating nested list comprehension of files starting with specific string I have a directory with 'sets' of files that start with a state name followed by 4 or 5 digits (typically indicating year). Each 'file set' contains 3 files a .txt, a .png, and a .jpg.

Example of files in directory:

California1998_reform_paper.txt
California1998_reform_paper.pdf
California1998_reform_paper.jpg
California2002_waterdensity_paper.txt
California2002_waterdensity_paper.pdf
California2002_waterdensity_paper.jpg

Based on a users input I am trying to write some code that can put each of these file sets into a list of lists. Ultimately I would like to iterate over the list of lists. That said, I am not married to any one data type if a dictionary or something else may be more efficient.

I would like the user to be able to enter either:

The state name i.e. 'California' - to get all files from California
OR
The state name + year i.e. 'California1998' to get all files from California 1998

import os
import regex

directory = #path to directory
input = 'California1998'

# Does input match proper format? If not error.
mm = regex.match('^([a-z]+)([0-9]{4,5})|^([a-z]+)', input)
dir = str(os.listdir(directory))
if mm.group(1):
    state = mm.group(1)
    number = mm.group(2)
    state_num = state + number
    fileset = regex.findall(state_num, dir)
elif mm.group(3):
    state = mm.group(3)
    fileset = regex.findall(state + r'[0-9]{4,5}', dir)
else:
    print('Put some error message here')

# Does input exist? If not error.
if len(fileset) > 0:
    fileset = tuple(set(sorted(fileset)))
else:
    print('Put some error message here')

# Get list of lists
state_num_files = [[file.path
    for file in os.scandir(directory)
    if file.name.startswith(state_num)]
    for state_num in fileset]
return state_num_files

Above is the code I have thus far. It first uses regex.match to check the input, then regex.findall to find all matching state + years. I then create a sorted() set() from this list, which is converted into a tuple() called fileset. The last bit of code is a nested list comprehension that produces the list of lists by iterating through all files in the directory and iterating through all the state + years in fileset.

It certainly works, but seems repetitive and slower than it needs to be. My goal is to increase efficiency and remove any unnecessary iteration.

Thoughts on improvements:

Possibly replace each regex.findall with a nested list comprehension? and thus remove the state_num_files nested comprehension at the end of script?

Any thoughts are greatly appreciated!

Review

Bug on Capitalization

mm = regex.match('^([a-z]+)([0-9]{4,5})|^([a-z]+)', input)

This does not work for the given use case of California1998

But it can be easily fixed by adjusting the regex to include [A-Za-z] capital letters
Stop overshadowing!

You use multiple built-in keywords as variable names ie, input dir this makes it that the overshadowed functions can no longer be used further in the program
import regex?

I think this should be import re, since that's what the library is called
Unnecesarry operations

The sort and consecutive conversion do nothing at all

Secondly you loop over the directory twice! This can be avoided by doing it one loop.
Don't print errors raise them

And make sure the user get's back useful information in the error message this generic message is not very helpful :)

Alternative

Look into the glob module this does what you require,

>>> import glob
>>> glob.glob("./California1998*")
['./California1998_reform_paper.jpg', './California1998_reform_paper.txt', './California1998_reform_paper.pdf']


Add items to a list, or update the quantity if already in the list I have several lists that contain Part objects with a partNumber and quantity. When the user selects to add a part to an existing list, I want to check to see if the part is already on the list. If it is, I simply want to add the new quantity to the existing quantity, and if the part is not already on the list, I want to add the entire Part object to the list. This solution works, but it feels verbose.

My question is: Can the forEach and if(!partExistsOnTransfer) code be simplified?

var transfer = this.transferRequests[index],
    transferList = $filter("filter")(
        $this.openTransfers,
        { id: id },
        true
    ),
    partToAdd = new Part(
        $this.currentPart.partNumber, // partNumber
        transfer.quantity // quantity
    ),
    partExistsOnTransfer = false;

transferList[0].parts.forEach(function (part, indx) {
    if (part.partNumber === partToAdd.partNumber) {
        // if part already exists, add to existing quantity, do not push new part onto list
        partExistsOnTransfer = true;
        part.quantity = (Number(part.quantity) + Number(partToAdd.quantity)).toString();
        return false;
    }
});
if (!partExistsOnTransfer) {
    // if part does not exist, push new part onto list
    transferList[0].parts.push(partToAdd);
}

I am not really sure about the needs but when you see something like this it should make you question if there is a better data structure.  If you stored the parts in an Object for example keyed by partNumber the code is more algorithmically efficient (O(1) vs O(N)) and a bit cleaner.

var newPartNumber = partToAdd.partNumber,
    existingPart = parts[newPartNumber];

if(!existingPart) {
    parts[newPartNumber] = partToAdd;
}
else {
    existingPart.quantity = ...;
}

Now if there are other reasons for using an array or using an object keyed by part number wouldn't be useful anywhere else I would use the some
method.

 var partExistsOnTransfer = transferList[0].parts.some(function (part, indx) {
    if (part.partNumber === partToAdd.partNumber) {
        // if part already exists, add to existing quantity, do not push new part onto list
        part.quantity = (Number(part.quantity) + Number(partToAdd.quantity)).toString();
        return true;
    }
});

Also remember returning false on an Array forEach doesn't make it stop iterating.


Parse a value to a given datatype For our (internal) automated test project, I am writing a part where parameters can be passed to an SSIS Package before it gets executed. The tester delivers the parameter name, value and desired datatype in a table format. I get the datatype and parses it to the correct type. We assume a happy flow (when testers do something wrong, no pretty feedback is needed, they can read the exceptions).

The following code is the snippet I am not happy with:

foreach (var row in parametersTable.Rows)
{
    object value;
    if (parametersTable.ContainsColumn("DataType") && !String.IsNullOrEmpty(row["DataType"]))
    {
        string dataType = row["DataType"].ToLower();
        if ("int64".Equals(dataType))
        {
            value = Int64.Parse(row["Value"]);
        }
        else if ("datetime".Equals(dataType))
        {
            value = DateTime.Parse(row["Value"]);
        }
        else
        {
            throw new ArgumentException("DataType " + row["DataType"] + " is not supported");
        }
    }
    else
    {
        value = row["Value"];
    }

    pfe.AddParameter(row["Name"], value);
}

I think, my string manipulation and testing can be improved and I'd like to make the "test and parse" section more expandable. Maybe an Enum or an helper object where I just have to add a single value to make it work for additional datatypes.

There is no magic method in .NET that will know the datatype and magically returns the right parsed value for you. So, creating your own method of parsing and doing manipulation is not a bad thing. This can be improved though.

Use a method:

I prefere to place code that does a certain manipulation or calculation on variables in a separate method/function. This gives me the ability to reuse the code if necessary and maintaining the code is also easier then looking in chunks of code to change just a small bit (like adding a datatype in your case).

switch vs. if/else:

When facing a simple check for a value or condition of a variable, an if/else-statement will certainly do. But when you have a list of options for a variable, it's better to use a swicth-statement. Your code not only looks cleaner, it is cleaner and again, easier to maintain.

This results in following code:

public object ParseValue(string type, string value)
{
    switch(type)
    {
        case "int64" : return Int64.Parse(value);
        case "datetime" : return DateTime.Parse(value);
        case "double" : return Double.Parse(value);
        default: throw new ArgumentException("DataType is not supported");
    }
}

And the usage:

foreach (var row in parametersTable.Rows)
{
    object value;

    if (parametersTable.ContainsColumn("DataType") && !String.IsNullOrEmpty(row["DataType"]))
    {
        var dataType = row["DataType"].ToLower();
        value = ParseValue(dataType, row["Value"]);
    }
    else
    {
        value = row["Value"];
    }

    pfe.AddParameter(row["Name"], value);
}

Another way of using a method is making it return a bool to see if the parsing succeeded and using the out parameter. This results in following method and usage:

public bool TryParseValue(string type, string value, out object parsedValue)
{
    switch(type)
    {
        case "int64" : parsedValue = Int64.Parse(value); return true;
        case "datetime" : parsedValue = DateTime.Parse(value); return true;
        case "double" : parsedValue = Double.Parse(value); return true;
        default: parsedValue = null; return false;
    }
}

foreach (var row in parametersTable.Rows)
{
    object value;

    if (parametersTable.ContainsColumn("DataType") && !String.IsNullOrEmpty(row["DataType"]))
    {
        var dataType = row["DataType"].ToLower();
        if(!TryParseValue(dataType, row["Value"], out value))
        {
            throw new ArgumentException("DataType is not supported");
        }
        //'value' will be correct if the parsing succeeeded
    }
    else
    {
        value = row["Value"];
    }

    pfe.AddParameter(row["Name"], value);
}


Converting between Fahrenheit and Celsius I'm learning Ruby and working through some exercises. I completed the following exercise without truly understanding why and how it works. I know that's an embarrassing thing to say but I feel like my subconscious mind wrote the code and conscious me (or maybe unconscious me you could say) is having trouble understanding why and how it works and if it's even written correctly.

I would appreciate if someone can run through this code and give me a breakdown of how it works and if you think it's written well.

My Code

class Temperature

  class << self
    def from_fahrenheit temp
      Temperature.new({f: temp})
    end

    def from_celsius temp
      Temperature.new({c: temp})
    end
  end

  def initialize(options={})
    @f = options[:f]
    @c = options[:c]
  end

  def in_fahrenheit
    return @f if @f
    (@c * (9.0 / 5.0)) + 32
  end

  def in_celsius
    return @c if @c
    (@f - 32) * 5.0 / 9.0
  end
end

class Celsius < Temperature
  def initialize temp
    super(c: temp)
  end
end

class Fahrenheit < Temperature
  def initialize temp
    super(f: temp)
  end
end

Rspec Test

describe Temperature do

  describe "can be constructed with an options hash" do
    describe "in degrees fahrenheit" do
      it "at 50 degrees" do
        Temperature.new(:f => 50).in_fahrenheit.should == 50
      end

      describe "and correctly convert to celsius" do
        it "at freezing" do
          Temperature.new(:f => 32).in_celsius.should == 0
        end

        it "at boiling" do
          Temperature.new(:f => 212).in_celsius.should == 100
        end

        it "at body temperature" do
          Temperature.new(:f => 98.6).in_celsius.should == 37
        end

        it "at an arbitrary temperature" do
          Temperature.new(:f => 68).in_celsius.should == 20
        end
      end
    end

    describe "in degrees celsius" do
      it "at 50 degrees" do
        Temperature.new(:c => 50).in_celsius.should == 50
      end

      describe "and correctly convert to fahrenheit" do
        it "at freezing" do
          Temperature.new(:c => 0).in_fahrenheit.should == 32
        end

        it "at boiling" do
          Temperature.new(:c => 100).in_fahrenheit.should == 212
        end

        it "at body temperature" do
          Temperature.new(:c => 37).in_fahrenheit.should be_within(0.1).of(98.6)
        end
      end
    end
  end

  describe "can be constructed via factory methods" do

    it "in degrees celsius" do
      Temperature.from_celsius(50).in_celsius.should == 50
      Temperature.from_celsius(50).in_fahrenheit.should == 122
    end

    it "in degrees fahrenheit" do
      Temperature.from_fahrenheit(50).in_fahrenheit.should == 50
      Temperature.from_fahrenheit(50).in_celsius.should == 10
    end

  end

  describe "utility class methods" do

  end

  # Here's another way to solve the problem!
  describe "Temperature subclasses" do
    describe "Celsius subclass" do
      it "is constructed in degrees celsius" do
        Celsius.new(50).in_celsius.should == 50
        Celsius.new(50).in_fahrenheit.should == 122
      end

      it "is a Temperature subclass" do
        Celsius.new(0).should be_a(Temperature)
      end
    end

    describe "Fahrenheit subclass" do
      it "is constructed in degrees fahrenheit" do
        Fahrenheit.new(50).in_fahrenheit.should == 50
        Fahrenheit.new(50).in_celsius.should == 10
      end

      it "is a Temperature subclass" do
        Fahrenheit.new(0).should be_a(Temperature)
      end
    end
  end

end

For someone who wasn't conscious when he wrote this code, it looks quite nice :)

Some observations:

Tests should do only one thing

This is OK in most of your tests, but you got a little 'lazy' at the last two tests - make a different test for each should. You could use RSpec's one-liner syntax, just for the fun of it:

 describe "in degrees celsius" do
   subject { Temperature.from_celsius(50) }

   its (:in_celsius) { should == 50 }
   its (:in_fahrenheit) { should == 122 }
 end

Meaningful Names

Again, in most cases, you are doing fine, and there may even be good arguments to say that @f and @c are clear enough in context, but there really is no reason not to call them @fahrenheit and @celsius. temp is also a quirk you should fix...

When things don't change

After you create a Temperature instance, its celsius and fahrenheit values will never change, so there is no reason why you should continue to calculate them - simply store the result in the appropriate member:

  def in_fahrenheit
    return @f if @f
    @f = (@c * (9.0 / 5.0)) + 32
  end

You can even make this shorter, using the ||= operator, which will compute and assign the member if it is not assigned:

  def in_fahrenheit
    @f ||= (@c * (9.0 / 5.0)) + 32
  end

This is looking more and more like a getter function, and really, since it calls for the state of the instance, you don't need to imply there is ever a calculation, simply call it fahrenheit:

def fahrenheit
  @fahrenheit ||= (@celsius * (9.0 / 5.0)) + 32
end

Have a consistent style

In your code you call methods with hashes with three different styles:

Temperature.new({f: temp})
super(c: temp)
Temperature.new(:c => 50).in_celsius.should == 50

All of these styles work, but it is better to choose one, and stick with it. When using a hash in method, my favorite is the second style (Temperature.new(f: temp))

Use language features

Since Ruby 2.0, named parameters are supported, and you are encouraged to use it. It will give you free validation, as any parameter which is not one of those you declared, an error will be issued:

def initialize(celsius: nil, fahrenheit: nil)
  @fahrenheit = fahrenheit
  @celsius = celsius
end

One last point, regarding your Celsius and Fahrenheit classes, I personally don't see any utility in having them, since they add no logic besides a construction convenience, and may even confuse, since their name implies they support only one scale or the other...

So now, your class will look like this:

class Temperature

  class << self
    def from_fahrenheit fahrenheit
      Temperature.new(fahrenheit: fahrenheit)
    end

    def from_celsius celsius
      Temperature.new(celsius: celsius)
    end
  end

  def initialize(celsius: nil, fahrenheit: nil)
    @fahrenheit = fahrenheit
    @celsius = celsius
  end

  def fahrenheit
    @fahrenheit ||= (@celsius * (9.0 / 5.0)) + 32
  end

  def celsius
    @celsius ||= (@fahrenheit - 32) * 5.0 / 9.0
  end
end


Searching for files across multiple code projects I was tasked with conducting a large number of text searches in the code files of multiple projects. Initially, I just used the Find in Files feature of Visual Studio in a couple of the running instances, but then I realized I needed to search other solutions as well.  So I quickly created an F# Script (below) to conduct the searches across all code files async.

For all matching results, the script prints:

C:\path-to-file
linenumber: line of code

How can this be improved?  The script runs extremely fast and did its job well.  Now, out of curiosity, I would like to know what improvements could be made to optimize performance?

open System
open System.IO

let directory = @"C:\path-to-top-level-directory"

// this just keeps async operations from writing to the console concurrently (garbled result)
let agent = MailboxProcessor.Start(fun inbox ->
    // the message processing function
    let rec messageLoop () = async {
        let! msg = inbox.Receive()
        printfn "%s" msg
        return! messageLoop()
    }
    // start the loop
    messageLoop ()
)

let checkIfContains value file = async {
    use reader = new StreamReader(File.OpenRead(file))
    let lineNum = ref -1
    let postResult (i, s) = sprintf "%s\r\n%i: %s\r\n" file i s |> agent.Post

    seq { while not reader.EndOfStream do
            incr lineNum
            yield (!lineNum, reader.ReadLine()) }
        |> Seq.filter (fun (_, s) -> s.Contains(value))
        |> Seq.iter postResult
}

let files =
    Directory.EnumerateFiles(directory, "*.vb", SearchOption.AllDirectories)
        |> Seq.append (Directory.EnumerateFiles(directory, "*.cs", SearchOption.AllDirectories))

while true do
    agent.Post "Search string: "
    let value = Console.ReadLine()
    agent.Post "Searching...\r\n"

    files
        |> Seq.map (checkIfContains value)
        |> Async.Parallel
        |> Async.RunSynchronously
        |> ignore

    agent.Post "\r\nDone.\r\n\r\n"

For a base line, I modified your script so that it is not dominated by printing to the console:

open System
open System.IO

let directory = @"C:\Dev"

// this just keeps async operations from writing to the console concurrently (garbled result)
let agent = MailboxProcessor.Start(fun inbox ->
    // the message processing function
    let count = ref 0
    let rec messageLoop () = async {
        let! msg = inbox.Receive()
        count :=  !count + 1
        return! messageLoop()
    }
    // start the loop
    messageLoop ()
)

let checkIfContains value file = async {
    use reader = new StreamReader(File.OpenRead(file))
    let lineNum = ref -1
    let postResult (i, s) = sprintf "%s\r\n%i: %s\r\n" file i s |> agent.Post

    seq { while not reader.EndOfStream do
            incr lineNum
            yield (!lineNum, reader.ReadLine()) }
        |> Seq.filter (fun (_, s) -> s.Contains(value))
        |> Seq.iter postResult
}

let files =
   Directory.EnumerateFiles(directory, "*.cs", SearchOption.AllDirectories)

let value = "papa"

files
    |> Seq.map (checkIfContains value)
    |> Async.Parallel
    |> Async.RunSynchronously
    |> ignore

agent.Post "\r\nDone.\r\n\r\n"

This takes ~10 seconds on my machine.
Like Carsten said, there's no need for a MailboxProcessor. Removing it takes me down to ~7.5 seconds.
Next, there's no need for Async. There is no ! in that async comp expression. We could use Seq.AsParallel instead (in System.Linq namespace). Down to ~6.3 secs.

Now this task is dominated by IO, and AsParallel shouldn't help here. In fact, removing it yields the same result, ~6.3 secs.

Now, that seq inside of checkIfContains looks wasteful. Getting rid of that yields ~6.1 secs, so no big win.

I wouldn't consider this idiomatic code. I just took out some unused stuff, and wound up with this:

open System
open System.IO
open System.Linq
let directory = @"C:\Dev"
let count = ref 0
let print s = lock count (fun () -> count := !count + 1)

let checkIfContains value file =
    use reader = new StreamReader(File.OpenRead(file))
    let lineNum = ref -1
    let postResult i s = sprintf "%s\r\n%i: %s\r\n" file i s |> print
    while not reader.EndOfStream do
      incr lineNum
      let line = reader.ReadLine()
      if line.Contains(value) then postResult !lineNum line

let files = Directory.EnumerateFiles(directory, "*.cs", SearchOption.AllDirectories)
let value = "papa"
let f = files
              |> Seq.map (checkIfContains value)
              |> Seq.iter id


Count has_many relationships I have 3 models: Import, Product, Values.
An import has_many products.
A product has_many values.

I want to find the product with the highest values count and the sum of all values for all those products. My current method is very expensive.

@import = Import.find(params[:id])
@products = @import.products
@max_product = @products.max_by{|product| product.values.count}
@values_count = @products.sum{|product| product.values.count}

How can I make this more efficient ?

You can make use ":counter_cache", it finds the number of belonging objects more efficient.

class Book < ApplicationRecord
  belongs_to :author, counter_cache: true
end
class Author < ApplicationRecord
  has_many :books
end

Asking for the value of @author.books.size requires making a call to the database to perform a COUNT(*) query. To avoid this call, you can add a counter cache, with this declaration, Rails will keep the cache value up to date, and then return that value in response to the size method.

Reference : http://guides.rubyonrails.org/association_basics.html


Retrieve value from a formula in cells into different merged cells and retrieved value can be edited I have created this macro and having a small issue. I am very new to this VBA and did this with experts’ guides by googling it. I am creating an excel form for a work purpose at work. The Excel form has 7 sheets and each have named tables with different data.

1st sheet is the editable form, which retrieves data from the tables in the different sheets. This is the only visible sheet, and other sheets are protected and hidden,
1st sheet has hidden rows and columns those have logical formulas that assist me to retrieve my data from the other tables, so cells with formulas (mostly-vlookup, index, match, if, or) are in those rows and columns.
I want the user to edit on the retrieved data, which is kind of suggestion to the user for to use it as-is or edit on it if required.
I cannot write vlookup or loop kind of events, so I created below macro that allows me to retrieve the value of the formula as a text (not editable) in different cell, then retrieve that text to another merged and named cell as editable text.
This macro works fine, but when I add more and more data into the Excel and add other macros, then it is quit slow.

Is it possible to this and make it function faster?

This macro applies to Sheet 1.

Private Sub Worksheet_Change(ByVal Target As Range)
    'Value as text - kind of copy+pastespecial
    If Range("F17").Value <> Range("E17").Text Then
        Range("F17").FormulaR1C1 = Range("E17")
        Range("func_R").FormulaR1C1 = Range("F17")
             'func_R and other cells below with "_R" are my merged and main cells that users will see. E17 has a vlookup formula that retrieve data from a table in Sheet 2, with this macro I copy and pastpecial the formula in E17 in F17 as a text. Then to make it editable text I copy and paste it into the merged cell with "_R".Texts in cells with "_R" come from same table in sheet 2.
        ElseIf Range("F18").Value <> Range("E18").Text Then
            Range("F18").FormulaR1C1 = Range("E18")
            Range("job_R").FormulaR1C1 = Range("F18")
        ElseIf Range("F20").Value <> Range("E20").Value Then
            Range("F20").FormulaR1C1 = Range("E20")
            Range("purp_R").FormulaR1C1 = Range("F20")
        ElseIf Range("F22").Value <> Range("E22").Value Then
            Range("F22").FormulaR1C1 = Range("E22")
            Range("duty_R").FormulaR1C1 = Range("F22")
        ElseIf Range("F25").Value <> Range("E25").Value Then
            Range("F25").FormulaR1C1 = Range("E25")
            Range("ikey_R").FormulaR1C1 = Range("F25")
        ElseIf Range("F26").Value <> Range("E26").Value Then
            Range("F26").FormulaR1C1 = Range("E26")
            Range("ekey_R").FormulaR1C1 = Range("F26")
        ElseIf Range("F28").Value <> Range("E28").Value Then
            Range("F28").FormulaR1C1 = Range("E28")
            Range("iimp_R").FormulaR1C1 = Range("F28")
        ElseIf Range("F29").Value <> Range("E29").Value Then
            Range("F29").FormulaR1C1 = Range("E29")
            Range("eimp_R").FormulaR1C1 = Range("F29")
        'Generel requirement - it is basicly same as above, but data comes from different table in Sheet 3.
        ElseIf Range("F31").Value <> Range("E31").Value Then
            Range("F31").FormulaR1C1 = Range("E31")
            Range("req_1").FormulaR1C1 = Range("F31")
        ElseIf Range("F32").Value <> Range("E32").Value Then
            Range("F32").FormulaR1C1 = Range("E32")
            Range("req_2").FormulaR1C1 = Range("F32")
        ElseIf Range("F33").Value <> Range("E33").Value Then
            Range("F33").FormulaR1C1 = Range("E33")
            Range("req_3").FormulaR1C1 = Range("F33")
        ElseIf Range("F34").Value <> Range("E34").Value Then
            Range("F34").FormulaR1C1 = Range("E34")
            Range("req_4").FormulaR1C1 = Range("F34")
        ElseIf Range("F35").Value <> Range("E35").Value Then
            Range("F35").FormulaR1C1 = Range("E35")
            Range("req_5").FormulaR1C1 = Range("F35")
        End If
    End Sub

I use these two methods when running macros that do intensive work of reading/writing information to several sheets:

' hide the mouse pointer, which in some PCs makes macros very slow. put this at the top
' of your module
Private Declare PtrSafe Function ShowCursor Lib "USER32" (ByVal fShow As Integer) As Integer

' disable some Excel features while the macro is running to improve performance
Sub enableMacroOptimizations()

    Dim cursorCount As Integer

    Application.ScreenUpdating = False
    Application.Calculation = xlCalculationManual
    Application.EnableEvents = False
    Application.DisplayScrollBars = False
    ActiveWindow.DisplayWorkbookTabs = False
    Application.DisplayStatusBar = True
    Do
        cursorCount = ShowCursor(False)
    Loop While (cursorCount >= 0)

End Sub

' enable Excel features disabled with enableMacroOptimizations()
Sub disableMacroOptimizations()

    Dim cursorCount As Integer

    Application.ScreenUpdating = True
    Application.Calculation = xlCalculationAutomatic
    Application.EnableEvents = True
    Application.DisplayScrollBars = True
    ActiveWindow.DisplayWorkbookTabs = True
    Application.DisplayStatusBar = True
    Do
        cursorCount = ShowCursor(True)
    Loop While (cursorCount < 0)

End Sub

This will improve the performance of your macros no matter what. We have used this code to reduce macro times from more than 10 minutes to just a few seconds.

Just call enableMacroOptimizations() at the top of you own intensive method, and then call disableMacroOptimizations() at the end (to leave the user workspace as found initially).


Makefile for a custom operating system kernel I'm now posting the second semi-complete part of my operating system makefile. The kernel itself is currently incomplete, so don't be surprised by the small size of the makefile. When the kernel expands, it will follow suit.

OFLAGS = -Ofast
DEBUGFLAGS = -g -g3 -ggdb3
TARGET = i686-elf
CC = gcc
INCDIR = -I kernel/include/
KERNEL = opal101
CFLAGS = -ffreestanding -Wall -Wextra -fno-exceptions -funsigned-char
LDFLAGS = -T linker.ld -Ofast -nostdlib -lgcc
AS = as
VPATH = kernel/arch/$(TARGET) $(shell find . -type d ! -name "arch"*)
include kernel/arch/$(TARGET)/Makefile
include kernel/drivers/vga/Makefile
include kernel/lib/Makefile
OBJECTS = $(C_OBJECTS) $(ASM_OBJECTS)
.PHONY: help all clean iso run debug
help:
        @echo "make: Usage: make [target]";\
        echo "Targets [run | iso | all | cleani | debug]"
debug: CFLAGS:= $(CFLAGS) $(DEBUGFLAGS)
debug: all
        qemu-system-i386 --kernel $(KERNEL).bin -S -s& \
        gdb-multiarch -s $(KERNEL).bin
run:iso
        qemu-system-i386 -cdrom $(KERNEL).iso
iso: CFLAGS := $(CFLAGS) $(OFLAGS)
iso: all
        mkdir -p $(KERNEL)/boot/grub;\
        cp kernel/arch/i686-elf/stage2_eltorito $(KERNEL)/boot/grub;\
        cp $(KERNEL).bin $(KERNEL)/boot;\
        printf "default 0\n" >>menu.lst;\
        printf "title %s\n" $(KERNEL) >>menu.lst;\
        printf "kernel /boot/%s.bin" $(KERNEL) >>menu.lst;\
        mv menu.lst $(KERNEL)/boot/grub;\
        genisoimage -R -b boot/grub/stage2_eltorito -no-emul-boot \
        -boot-load-size 4 -boot-info-table -o $(KERNEL).iso $(KERNEL)
all: $(KERNEL).bin

$(KERNEL).bin:$(OBJECTS)
        $(TARGET)-$(CC) $(CFLAGS) $(LDFLAGS) $(OBJECTS) -o $(KERNEL).bin
$(C_OBJECTS):%.o:%.c
        $(TARGET)-$(CC) $(CFLAGS) -c $< -o $@ $(INCDIR)
$(ASM_OBJECTS): %.o:%.s
        $(TARGET)-$(AS) $< -o $@
clean:
        rm -rf *.o *.bin *.iso opal101/

I will show the kernel/lib/Makefile. All the rest are almost the same.

C_OBJECTS := $(C_OBJECTS) string.o
string.o:string.h

NOTE: I am not willing to use auto dependency generators because I found them very complex to learn and use, so please don't ask me to use them.

Disclaimer: I'm not experienced with this field, but this question definitely needs some activity, so this is my attempt at an answer. If there's anything that you want me to cover that I don't mention here, just tell me about it and I'll try my best to see what I can do.

Style/nitpicks

I find that some of your rule indentation is a bit excessive, and makes your code a little harder to read. Right now you're indenting your rules by tabs with a width of eight spaces. I'd recommend that you indent by tabs with a width of four, like this:

some_rule : file.c
    do_something file.c -foo -bar

You also seem to have a lack of blank lines between your rules as well. I'd highly recommend including at least one blank line between each rule, and a comment, if necessary:

# ...
some_rule1 : file.c
    ...

some_rule2 : file.c
    ...

Finally, you're inconsistent in your colon (:) placement in some places. For example, sometimes you place your colons like this:

all: $(KERNEL).bin

Other times, you place them like this:

$(C_OBJECTS):%.o:%.c

I prefer the following style, but as long as you're, again, consistent, any of the above or below are fine:

some_rule : ...


Minimal colored write I remembered a cwrite application that I used in the good old DOS times.

Basically it took a color code as first argument and some text as the second and printed the given text in the specified color. Also it supported some escape sequences.

Since Windows NT does not support ANSI Escape Codes it's not as easy to change the color of certain text within a batch script.

Well Windows 10 does support ANSI.... (It took MS 10 Versions to implement that?!)

Now I wanted to re-implement that cwrite from those days. With minimal requirements. Just win32 api. Nothing else.

I have commented some lines in the code that I am unsure of.

#include <Windows.h>

int main(int argc, char ** argv) {
    HANDLE h = GetStdHandle(STD_OUTPUT_HANDLE);
    if(!h) { return -2; } // god help us

    CONSOLE_SCREEN_BUFFER_INFO csbiInfo;
    BOOL v = GetConsoleScreenBufferInfo(h, &csbiInfo);
    if(!v) { return -1; } // still... god should help us

    WORD wOld = csbiInfo.wAttributes;

    // No arguments: show the user how it works
    //  and how the colors look...
    if(argc < 2) {
        printf("%s <color> [text]\n", argv[0]);
        printf(" <color> can be any of the following:\n");
        WORD i;

        for(i = 0; i < 0x100; i++) {
            v &= SetConsoleTextAttribute ( h, i );
            if(i % 0x10 == 0) { printf("\n"); }
            printf("%4i", i);

        }
        printf("\n");
        v &= SetConsoleTextAttribute( h, wOld);
        return v;
    }

    WORD wNew = (WORD)atoi(argv[1]);

    v &= SetConsoleTextAttribute ( h, wNew );
    // well now this is... unfortunate
    if(!v) { return -3; }

    /*
     * Classic "it's not a bug, but a feature"
     * If there is no argv[2] then we just change the color
     * ... without resetting to the original
     */
    if(argc < 3) { return 2; }

    // Now this is to handle some special characters
    // parhaps I should use sprintf?
    WORD j;
    for(j = 0; j < strlen(argv[2]); j++) {
        if(argv[2][j] == '\\') {
            switch(argv[2][j + 1]) {
                case 'n':
                case 'r':
                    argv[2][j]   = 0xa;
                    argv[2][j+1] = 0xd;
                    break;
                case 't':
                    argv[2][j] = ' ';
                    argv[2][j+1] = ' ';
                    break;
                default:
                    /* ignore */
                    break;
            }
        }
    }

    // Okay:
    // How do I turn a char ** into a va_list ???
    printf(argv[2], argv[3], argv[4], argv[5], argv[6], argv[7], argv[8], argv[9]);

    // Now reset the color
    v &= SetConsoleTextAttribute ( h, wOld );
    if(!v) { /* nop */ } // could not reset color...

    return v;
}

A few things I noticed when reading through (I don't know much about how Windows.h works, so I mostly focused on stuff not related to it)

This made me laugh:

// god help us

At the same time, though, it doesn't really describe what's happening. In this case, you don't really need an explanation; it's decently obvious what's up just by reading the relevant documentation. In other cases, though, you don't explain. For example...
here:

// parhaps I should use sprintf?

Why aren't you? I'm assuming the answer is "I couldn't see a clear benefit", but then explain that in a few words. (also, *perhaps)
wOld and wNew are bad variable names. Something like oldAttributes and newAttributes would be better, since it describes what's actually going on. If that's not what's going on, you should provide better variable names so I can understand what they should be :)
Why are you making j a WORD? strlen returns a size_t; while WORD is (usually, IIRC) 16 bits, size_t can be a great deal larger. Is it unlikely that you'll be encountering strings that are longer than 2^16 characters? Yes. Is the fix so easy to implement that there's no reason not to? Also yes.
Possible UB at switch(argv[2][j + 1]). If j == strlen(argv[2]), argv[2][j + 1] accesses memory outside of argv[2] (one past the end, in fact). Depending on how, specifically, the memory is laid out, this may not crash the program. It'd be better to do for (j = 1; j < len /* see below */; ++j) and use j - 1 and j where you use j and j + 1, respectively.
strlen operates by doing something roughly like this:

char *cur = your_string;
while (*cur) ++cur;
return cur - your_string;

That means that it's O(n). Performing that calculation makes your whole loop there O(n^2). If you store the length of argv[2], then compare to the stored length, you can improve the speed by a decent amount. Of course, for the number of characters you're most often dealing with, and with how simple your loop is, this doesn't matter so much, but it's a bad habit to get into.
Pretend it's six months later and you've been dealing exclusively not with text. When you see this:

argv[2][j]   = 0xa;
argv[2][j+1] = 0xd;

are you going to say "that's super clear and I know exactly what it does", or will you have to Google what the ASCII characters 0xa and 0xd are? You should put a comment there explaining it.
// How do I turn a char ** into a va_list ???

I don't think you can. Sorry.
In that same area, why are you assuming that accessing argv at argc and above will be valid, or that argv[2] will be a valid format string? You should check. Alternatively, just forget about building formatting in and let the calling code handle it (Bash, for example, can do that easily) -- it's a lot of work to try to do it yourself, for little to no benefit.
Your error codes are uninformative. Given that all you have is an integer, that's pretty much par for the course, but the least you could do is document them in the help message.
Related: Try running this simple C program from Bash:

int main() { return -1; }

Look at its return code -- it's 255, not -1. See here for more information; the gist of it is that negative numbers might not be interpreted that way by the OS. Stick to positive ones.
What's the point of this?

if(!v) { /* nop */ } // could not reset color...

The compiler just optimizes it out anyway; you might as well replace it with

// if it fails, we don't really care

to make it blatantly obvious that it doesn't matter.
feature-request Let me put in a color's name and see the output in that color, rather than making me use the number.

That's all I saw. Someone who's more familiar with the Windows API might find more.


Find the most frequent integer in an array I have been a Python developer for a few years and I'm going through some simple exercises to learn C++.

Please consider the following Python snippet

# Find the most frequent integer in an array
lst = [1,2,4,3,1,3,1,4,5,15,5,2,3,5,4]
mydict   = {}
cnt, itm = 0, ''

for item in lst:
     mydict[item] = mydict.get(item, 0) + 1
     if mydict[item] >= cnt :
         cnt, itm = mydict[item], item

print(itm)

It may not be very clean and it misses elements that tie for most frequent, but it is succinct code.  I re-wrote this to C++11 as:

#include <iostream>
#include <unordered_map>
#include <string>

int main(int argc, char const *argv[])
{
    using std::cout;

    int foo [] = {1,2,4,3,1,3,1,4,5,15,5,2,3,5,4};

    std::unordered_map<std::string, int> dict;

    std::string item;

    int count = 0;

    std::string itm;

    for (int i : foo) {
        item = std::to_string(i);

        auto it = dict.find(item);

        if (it != dict.end()) {
            it->second = it->second++;

            if (it->second >= count) {
                count = it->second;
                itm = item;
            }
        } else {
            dict.insert(std::make_pair<std::string, int>(item, 0));
        }

    }

    std::cout << "The integer most frequent is " << itm << "\n";
    return 0;
}

This executes fine and returns 4 which is one of the correct answers.  Can someone please offer a re-write of this to make it more succinct.

At present I am not concerned about if there's more than one element in the list that has a tie for most frequent.  I am more concerned with cleaner code.

Your original Python code translates line-for-line into C++11 as:

#include <iostream>
#include <map>
#include <tuple>
#include <vector>

int main()
{
    std::vector<int> lst = {1,2,4,3,1,3,1,4,5,15,5,2,3,5,4};
    std::map<int, int> mydict = {};
    int cnt = 0;
    int itm = 0;  // in Python you made this a string '', which seems like a bug

    for (auto&& item : lst) {
        mydict[item] = mydict.emplace(item, 0).first->second + 1;
        if (mydict[item] >= cnt) {
            std::tie(cnt, itm) = std::tie(mydict[item], item);
        }
    }

    std::cout << itm << std::endl;
}

The only newly introduced lines here are the #include lines, the lines that define main(), and a couple of closing curly braces.

Your C++ code could definitely be cleaned up by removing a lot of its blank lines and premature declarations; for example, there's no need to put a blank line in between every pair of declarations, and there's no need to pre-declare item outside the for-loop. Just tightening up the style (whitespace and such) will go a long way toward readability.

You have a bug on this line:

it->second = it->second++;

In C++, it->second++ is a side-effecting expression: it means "actually increment it->second, and then return the old value." So (in C++11 and C++14), this statement as a whole has undefined behavior: it's telling the compiler that you want to increment it->second, but also assign a new value to it->second.

In C++17, this statement may or may not have defined behavior; I'll have to read up on it and get back to you. Certainly it->second++ is guaranteed to be evaluated prior to the assignment, but I don't know if that completely solves the problem.

Anyway, what you meant to write was simply

it->second = it->second+1;

or

++it->second;

Stylistically, I advise writing "increment thing" instead of "thing increment", intuitively because that's how English works, but also because it can be an optimization in some cases involving overloaded operator++ (and is never a pessimization).

Your code actually doesn't compile with any C++11 compiler I have access to, because of your misuse of the std::make_pair function template. You wrote

std::make_pair<std::string, int>(item, 0)

but you should have written

std::make_pair(item, 0)

so that the compiler would deduce the types for you. (And in this case it deduces std::make_pair<std::string&, int>; but you as the programmer shouldn't have to care about that. Just let the compiler do its job and you try to stay out of its way as much as possible.)

The function parameters argc and argv are unused, so you should get rid of them. int main() is perfectly fine in both C and C++.

I don't understand why you write using std::cout; but then proceed to use the fully qualified names of std::string, std::make_pair, and so on. I would think that you'd either use the fully qualified names of everything from std:: (my preferred choice), or else go all out and using std::... a ton of things.

Anyway, using std::cout doesn't save you any typing — it actually costs you two lines and about 10 characters — so you'd do well to get rid of it.

As far as improving your C++ skills to the point where you can translate Python line-for-line as I did above, the best advice I can give you is go to cppreference.com and study the standard library! The STL is way better today than it was in 2003, and it takes ideas from many other languages. So for example the C++ equivalent of mydict.get(item, 0) is this humongous ugly expression mydict.emplace(item, 0).first->second (and I'm not actually saying you should write that ugly expression in production code!), but it does exist, and translation from Python to C++ is just a matter of being able to remember vaguely what it's called and look it up. And that just takes a lot of practice.


Using one parent model and other extending it in a PHP framework I have multiple models which all have the same functions in my framework. I decided to create one "parent" model and all of my other models would extend and inherit its functions like this.

My other models would be like this:

class Games_model extends MY_Model {
    protected $table = 'games';
    protected $_default_select = array();
    protected $_default_join = array();

    public function __construct()  {
        $this->_default_select = array(
            $this->table.'.*',
            $this->table.'.id as id',
            $this->table.'.name as name',
            'categories.id as cat_id',
            'categories.name AS cat_name'
        );
        $this->_default_join = array('categories', 'categories.id = fiches.console_id', 'left');
    }
}

Here's how I use it:

$this->games_model->limit(30,0)->where(array('categorie_id' => 1))->get_list()->result();

Is it a better idea to do like that or to have all the code of MY_Model in Games_model?

Well, having a parent model that is extended for all your model classes is always a good idea if they all share the same behaviour and help you with your development. I have implemented a similar approximation within my models and I'm doing something similar to what you do, but instead of writing the query in the controller, I call to the model to handle the retrieve of the data.

Doing as you do it, you have to write all the time the same kind of query in your controller, losing the MVC pattern. Let's say you want to retrieve the values of categories, in a point of your controller you'd have:

$this->games_model->limit(30,0)->where(array('categorie_id' => 1))->get_list()->result();

If you want to retrieve the categories in other line of your controller, you'll have to write the same sentence, and... what if, in some point of your development,  you wanted to change the field categorie_id to, let's say, super_categorie_id? You'd have to do it in all the calls in your application code... You lost all the value the M in MVC is offering, isolate management of data in the Model layer.

So, I'd keep the extension, but writing the methods for reading with is proper filters INSIDE of the child class, and pass it only the data not needed for filtering (as limit) as a parameter, and set them to a default.

I don't know if I clearly explained myself, any doubt write a comment, ;D


Finding a max element in an array in Java Created the following method:

public static int max(int[] array) {
    int max = Integer.MIN_VALUE;

    for (int value : array) {
        if (value > max) {
            max = value;
        }
    }
    return max;
}

Question(s):

I believe the Big O Notation for this implementation is O(n), am I right?
Since this is basically a search algorithm, how to make it faster (e.g. should use recursion) and if so what would the Big O Notation be for the optimized version?

Yes, this is O(n), because you visit every element once.  You can't possibly do better than O(n), because there is no way to be sure that you have found the maximum value without inspecting every element.  (There are two exceptions.  If the array is known to be sorted in some way, then you can skip straight to the maximum.  Also, if you ever encounter Integer.MAX_VALUE, you could return early.  The latter optimization is probably not worthwhile.)  Recursion would not be faster than iteration, and, in general, iteration should be favoured over recursion in Java for performance.

Since Java 8, you can implement this as a one-liner using IntStream.max():

java.util.Arrays.stream(array).max()

Notice that IntStream.max() returns an OptionalInt.  Why OptionalInt instead of int?  Because the array might be empty.  Your code would return Integer.MIN_VALUE in that case.  If that is what you consider to be the "correct" behaviour, then you should document it in the JavaDoc.


Calling multiple functions in image upload form I have an image upload form with which I call multiple functions before uploading the file such as checking the size and dimensions, and that it is in a valid image. The code works, but calling the functions and dealing with the results seems quite cumbersome and complex.

Is there a tidier way of structuring the code?

This is what I currently have:

//Upload an image
if(checkvalidfile("img",$ext)){
    if(checksize($size, 524288)) {
        if(checkdimensions($tmp, 300)) {
            $newfilename = renamefile($ext);
            recordfileindb($brand_id, $newfilename, $mysqli);
            uploadfile($tmp, $bucket, "user_docs/agency_".$agency_id."/brand_logos/", $newfilename, $s3);
            header('Location: ./message.php?action=newlogo');
        } else {
            echo "Your image can't be bigger than 300 x 300px";
            die;
        }
    } else {
        echo "File size is too big!";
        die;
    }
} else {
    echo "Not a valid file";
    die;
}

Yes, there's room for a lot of improvement here!

First of all, you can "break early" to avoid excessive indentation. Since die terminates the script, you can check if the file is not valid.

Secondly, instead of echoing first and die later, you can call die with a parameter.

This code works in the same way, but is cleaner:

if (!checkvalidfile("img",$ext)) {
    die("Not a valid file");
}
if (!checksize($size, 524288)) {
    die ("File size is too big!");
}

if (!checkdimensions($tmp, 300)) {
    die("Your image can't be bigger than 300 x 300px");
}

$newfilename = renamefile($ext);
recordfileindb($brand_id, $newfilename, $mysqli);
uploadfile($tmp, $bucket, "user_docs/agency_".$agency_id."/brand_logos/", $newfilename, $s3);
header('Location: ./message.php?action=newlogo');


Excel Data Manipulation - parse, match and create I've got a simple Excel Data Manipulation Script to match one of my daily tasks, written in python 3.
Intro
Let's assume that I have 3 excel files: main.xlsx, 1.xlsx and 2.xlsx. In all of them I have a column named serial numbers. I have to:

lookup for all serial numbers in 1.xlsx and 2.xlsx and verify if they are in main.xlsx.

If a serial number is find:

on the last column of main.xlsx, on the same row with the serial number that was find, write OK + name_of_the_file_in which_it_was_found. Else, write NOK. At the same time, write in 1.xlsx and 2.xlsx ok or nok on the last column if the serial number was found or not.

Now, my script is simply creating new files with the last column appended (instead of appending directly to the same file), which is ok (but if you guys have a better method, shout it). What I'm looking for, is a way of making this as optimized as possible. I'm not looking for PEP8 comments as I'm aware of them, but I'll handle this part when I'll have this as optimized / improved as possible.
Code:
import petl

main = petl.fromxlsx('main.xlsx')
one = petl.fromxlsx('1.xlsx', row_offset=1)
two = petl.fromxlsx('2.xlsx')

non_serial_rows = petl.select(main, lambda rec: rec['serial number'] is None)
serial_rows = petl.select(main, lambda rec: rec['serial number'] is not None)
main_join_one = petl.join(serial_rows, petl.cut(one, ['serial number']), key='serial number')
main_join_one_file = petl.addfield(main_join_one, 'file', 'ok, 1.xlsx')
main_join_two = petl.join(serial_rows, petl.cut(two, ['serial number']), key='serial number')
main_join_two_file = petl.addfield(main_join_two, 'file', 'ok, 2.xlsx')
stacked_joins = petl.stack(main_join_two_file, main_join_one_file)
nok_rows = petl.antijoin(serial_rows, petl.cut(stacked_joins, ['serial number']), key='serial number')
nok_rows = petl.addfield(nok_rows, 'file', 'NOK')
output_main = petl.stack(stacked_joins, non_serial_rows, nok_rows)
main_final = output_main

def main_compare(table):
    non_serial_rows = petl.select(table, lambda rec: rec['serial number'] is None)
    serial_rows = petl.select(table, lambda rec: rec['serial number'] is not None)
    ok_rows = petl.join(serial_rows, petl.cut(main, ['serial number']), key='serial number')
    ok_rows = petl.addfield(ok_rows, 'file', 'OK')
    nok_rows = petl.antijoin(serial_rows, petl.cut(main, ['serial number']), key='serial number')
    nok_rows = petl.addfield(nok_rows, 'file', 'NOK')

    return petl.stack(ok_rows, nok_rows, non_serial_rows)

one_final = main_compare(one)
two_final = main_compare(two)

petl.toxlsx(main_final, 'mainNew.xlsx')
petl.toxlsx(one_final, '1New.xlsx')
petl.toxlsx(two_final, '2New.xlsx')

Sample files, can be downloaded from here. (for those who have time to play a lil' bit with the code).

I've not seen or used petl before, but there are some ways to improve your code.

Move things out of the global scope.
Make a function for your most common functions.
Change petl.select to petl.selectis or petl.selectisnot.

Since you do:

nok_rows = petl.antijoin(serial_rows, petl.cut(stacked_joins, ['serial number']), key='serial number')
nok_rows = petl.addfield(nok_rows, 'file', 'NOK')

So many times, you should change it to a function.
This function could take a fn of petl.antijoin, a left of serial_rows, a right of stacked_joins and a field of 'NOK'.

And so you can use:

def change_row_file(fn, left, right, field):
    rows = fn(left, petl.cut(right, 'serial number'), key='serial number')
    return petl.addfield(rows, 'file', field)

change_row_file(petl.antijoin, serial_rows, stacked_joins, 'NOK')

Adding this, with the changes to petl.select, to main_compare can result in something like:

def main_compare(table):
    serial_rows = petl.selectisnot(table, 'serial number', None)
    return petl.stack(
        change_row_file(petl.join, serial_rows, main, 'OK'),
        change_row_file(petl.antijoin, serial_rows, main, 'NOK'),
        petl.selectis(table, 'serial number', None))

There's not much more that I think could do with improving apart from having nothing in your global scope.
And then adding a global constant for the serial column, say SERIAL_COLUMN = 'serial number'.
This significantly reduces the amount of magic strings, to ones that aren't repeated (except 'NOK').

This can result in (Not pep8 compliant to remove a scroll bar):

import petl
SERIAL_COLUMN = 'serial number'

def change_row_file(fn, left, right, field):
    rows = fn(left, petl.cut(right, SERIAL_COLUMN), key=SERIAL_COLUMN)
    return petl.addfield(rows, 'file', field)

def update_main(main, one, two):
    serial_rows = petl.selectisnot(main, SERIAL_COLUMN, None)
    stacked_joins = petl.stack(
        change_row_file(petl.join, serial_rows, one, 'ok, 1.xlsx'),
        change_row_file(petl.join, serial_rows, two, 'ok, 2.xlsx'))
    return petl.stack(
        stacked_joins,
        petl.selectis(main, SERIAL_COLUMN, None),
        change_row_file(petl.antijoin, serial_rows, stacked_joins, 'NOK'))

def main_compare(main, table):
    serial_rows = petl.selectisnot(table, SERIAL_COLUMN, None)
    return petl.stack(
        change_row_file(petl.join, serial_rows, main, 'OK'),
        change_row_file(petl.antijoin, serial_rows, main, 'NOK'),
        petl.selectis(table, SERIAL_COLUMN, None))

def main():
    main = petl.fromxlsx('main.xlsx')
    one = petl.fromxlsx('1.xlsx', row_offset=1)
    two = petl.fromxlsx('2.xlsx')

    petl.toxlsx(update_main(main, one, two), 'mainNew.xlsx')
    petl.toxlsx(main_compare(main, one), '1New.xlsx')
    petl.toxlsx(main_compare(main, two), '2New.xlsx')

if __name__ == '__main__':
    main()


Vending Machine in Idris Type Driven Development with Idris demonstrates how Idris can be used to build a Finite State Machine for a vending machine.

Please critique my implementation. I'm new to dependent types, so I'm not sure if dependent types would enhance this implementation. In other words, if path-dependent types would significantly improve this FSM, then I'm not sure how.

module VendingMachine

-- Pounds is a form of currency/money
Pounds : Type
Pounds = Nat

-- the Machine dispenses/vends chocolate candy bars
Choc : Type
Choc = Nat

data Machine = Mach Pounds Choc

fill : Machine -> Nat -> Machine
fill (Mach ps bars) ns = Mach ps (bars + ns)

init : Machine
init = Mach 0 0

insertCoin : Machine -> Machine
insertCoin (Mach ps bars) = (Mach (ps + 1) bars)

vend : Machine -> Machine
vend (Mach Z bars)          = Mach Z bars
vend (Mach (S ps) (S bars)) = Mach ps bars

getChange : Machine -> Machine
getChange (Mach Z bars) = Mach Z bars
getChange (Mach n bars) = Mach 0 bars

I've tried searching and googling for variations on how to solve this kind of problem, and all the searches turn up either posts made by you or references to the original documentation and the Effects library. In other words, there is not much online documentation available for the Idris language, and I'm a little unsure as to whether it is worthwhile pursuing this language. However I can comment a little on general issues in your code and state machines:

Naming is slightly inconsistent – For the types I would use Pounds and ChocolateBars (or Bars), with the corresponding pounds and bars when used as parameters
Be consistent using either (S pounds) or (pounds + 1) - Try sticking to one specific notation to indicate the incremented version. The same goes for the use of Z or 0. Be consistent.
What is the price of one chocolate bar? If it´s "1 pound" which the code related to vend it doesn't make sense to have a getChange variant. This is more of a removePounds method.
States are not interlinked – I would define more states, with actions clearly linked to the states. As it stands there is no connection between insertCoin, vend and possibly the getChange actions. All of these can be executed in an arbitrarily sequence depending on availability of pounds and bars in the vending machine.
Alternative version in pseudo code:

Empty – An empty vending machine, with no money or no bars

action: fill → state: Idle: Increases the quantity of bars

Idle - A vending machine with no currently inserted money, but has bars

action: insertCoin → state: ReadyForVending: changes the coinInSlot variable
action: removePounds → state: Idle: Clears the pounds variable

ReadyForVending – With inserted money, and bars

action: vend → state: Idle: Removes one bars, and clears coinInSlot
action: returnCoin →  state: Idle: Clear the coinInSlot

To implement this you would need another state variable like coinInSlot in addition to the bars and pounds.
Machine vs Mach is unclear – I'm not sure if you've gotten it quite correct with these, and at least the names are kind of confusing. I think rather than the strange extra notation of Mach I would've used Tuples or Records to describe the internal state, and this would simplify some of the following code as well.
Use effects for the state machine – In the online compiler I found, there was not support for effects, but the documentation (and some slides/post/papers) I found indicates that to properly implement a state machine you should use the Effects library (hangman), which aren't supported in the online compilator.

I tried compiling a new version using the online compilator at http://www.tryidris.org, but there was loads of errors and it seemed a little unstable. In addition, when trying to get information from OP regarding how you actually are running your code, and expected output, I didn't get much response. As such, I've failed getting you alternate refactored code better implementing a type dependent state machine in Idris.


O(N) vs O(N * N) for Word Frequency Algorithm I am trying to come up with a better solution to the following problem:

Statement

  You are supposed to write a function which takes the contents of a document in string format and also a whole number, n, providing how many results to return. (So that's two arguments for the function.)  The function should return the n most frequently occurring words in descending order.  The algorithm should be \$O(N)\$, not \$O(N \cdot N)\$.

My Current Solution

function freq(str, n) {
  var results = [];

  var strArray = str.split(/\s+/), countObj = {}, wordArray = [], wordObj = {};

  strArray.forEach(function(el) {
    countObj[el] ? countObj[el] += 1 : countObj[el] = 1;
  });

  for(var el in countObj) {
    wordObj[countObj[el]] ? wordObj[countObj[el]].push(el) : wordObj[countObj[el]] = [el];
  }

  for(var el in wordObj){
    wordArray.push(wordObj[el].join(' '));
  }

  for(var i = 0; i < n; i++) {
    results.push(wordArray[wordArray.length - 1 - i]);
  }

  return results.join('\n');
};

Time Complexity

The solution seems to be \$O(N)\$. Are there any improvements I could make?

Best Practices

I am using ternary operators a lot for the if-else construct. Am I overusing them?

General

Any suggestions regarding how I can make this solution 'production grade' quality?

First of all, there's a bug: You're relying on the properties in wordObj being iterated in ascending order when creating wordArray. I.e. wordObj['1'] is handled before wordObj['3'] and so on. This behavior is not guaranteed!

Objects are unordered in JS. Some engines do order them, but you cannot rely on it. Even engines that do order properties are under no obligation to sort them in any particular way. They might be in descending order, or sorted lexicographically (which would make sense since keys are strings, but it'd sort 10 before 2), or something else entirely.

Secondly, it just seems like you're working too hard here.

If there's a "tie" between to words (say "foo" and "bar" both appear 3 times), you return all of them on one line. The problem description does not explicitly require you to handle ties in any particular way. However, your solution basically means that you're not actually returning N words. A line of words separated by spaces is not "a word", but if it was, there'd be more than N words in total.

So I'd simply leave that out.

Besides, it's simpler to use reduce, sort and slice. You can also use a || "failover" instead of ternaries.

function freq(str, n) {
  var words = str.split(/\s+/);

  // Count occurrences
  var counts = words.reduce(function (memo, word) {
    memo[word] = (memo[word] || 0) + 1;
    return memo;
  }, {});

  // Sort by count (descending)
  var sorted = Object.keys(counts).sort(function (a, b) {
    return counts[b] - counts[a];
  });

  // Return the first n words, separated by linebreaks
  return sorted.slice(0, n).join("\n");
}

Edit: As rolfl points out, the input should probably be trimmed before anything else, or you might get empty strings in your words array. However, to avoid empty strings being counted as words, without changing the code above too much, you can do this:

var counts = words.reduce(function (memo, word) {
  if(word) { // ignore empty strings
    memo[word] = (memo[word] || 0) + 1;
  }
  return memo;
}, {});

Edit: As was pointed out, the above isn't actually O(n) due to its use of sort. Bummer. This one should be closer, although not as elegant. It's closer to the original code in some ways, but still avoids assuming that object properties are ordered.

function freq(str, n) {
  var words = str.split(/\s+/);

  // Count words (same as before)
  var wordCounts = words.reduce(function (memo, word) {
    if(word) {
      memo[word] = (memo[word] || 0) + 1;
    }
    return memo;
  }, {});

  // Group words into nested arrays that are, in turn,
  // indexed by the word count. I.e. wordsIndexedByCount[3]
  // will be a nested array of all the words that occurred
  // 3 times.
  // Also keep track of the highest index we use (maxCount).
  var wordsIndexedByCount = [],
      maxCount = 0;

  for(var word in wordCounts) {
    var count = wordCounts[word];
    maxCount = count > maxCount ? count : maxCount;
    wordsIndexedByCount[count] = wordsIndexedByCount[count] || [];
    wordsIndexedByCount[count].push(word);
  }

  // Flatten the array from above, starting with the most
  // often occurring word(s) - i.e. the highest index.
  var sorted = [];
  for(var i = maxCount; i > 0; i--) {
    // Skip "holes" in the array (not that it's necessary; the
    // code works without this check, since push.apply ignores
    // an undefined argument)
    if(!wordsIndexedByCount[i]) {
      continue;
    }
    [].push.apply(sorted, wordsIndexedByCount[i]);
    if(sorted.length >= n) {
      break; // if we've got enough to return, there's no reason to keep going
    }
  }

  return sorted.slice(0, n).join("\n");
}


Ruby splits text into matrix creating a hash of counts of column 2 My code has two main concerns

it seems unecessairly long
unable to use new each_with_object method without shadowing answer(variable)

How should I proceed to reduce my code and make my intention clearer?

require 'pp'

data = <<EOF
0       303567       3584       Write       0.000000
1       55590       3072       Write       0.000000
0       303574       3584       Write       0.026214
1       240840       3072       Write       0.026214
1       55596       3072       Read       0.078643
0       303581       3584       Write       0.117964
1       55596       3072       Write       0.117964
0       303588       3584       Write       0.530841
1       55596       3072       Write       0.530841
0       303595       3584       Write       0.550502
1       240840       3072       Write       0.550502
1       55602       3072       Read       0.602931
0       303602       3584       Write       0.648806
1       55602       3072       Write       0.648806
0       303609       3584       Write       0.910950
1       55602       3072       Write       0.910950
0       303616       3584       Write       0.930611
1       240840       3072       Write       0.930611
1       55608       3072       Read       0.983040
0       303623       3584       Write       1.028915
1       55608       3072       Write       1.028915
0       303630       3584       Write       1.330380
1       55608       3072       Write       1.33038
EOF

ary = Array.new

data.each_line { |line|
  ary.push(line.split(" "))
}

answers = ary.map {|row| row[1]}

counts = Hash.new 0

answers.each do |answer|
  counts[answer] += 1
end

# creates shadowing
# solution = answer.each_with_object(Hash.new(0)) { |answer,counts| counts[answer] += 1}
pp counts

Sorry, but I didn't get your question on the shadowing part... Anyway, if you want to shorten it (and try to use something similar to each_with_object), you can do it this way, as a oneliner :)

pp data.each_line.with_object(Hash.new(0)) {|row,solution| solution[row.split(" ")[1]]+=1}

Except for the indirect handling with the double square brackets, I think that it is quite readable :)

Explanation (slightly changed by introducing an "index" temp variable for the sake of clarity)

pp data
 .each_line                                  #cycling line by line
 .with_object(Hash.new(0)) { |row,solution|  #using a Hash as accumulator, later called solution
     index=row.split(" ")[1]                 #setting the temp variable as the second column
     solution[index]+=1                      #increase the relevant counter in accumulator
 }


handle null in Comparator class I have a class LoanAccount that contains an attributes creationDate and  loanAmount.

  I want to implement a Comparator that sorts the accounts by loan amount in descending order.
  When the loan amounts are the same, sorts the accounts
  ascending by date.

The code look like this:

Class

public class LoanAccount {
    private String id;
    private Date creationDate;
    private BigDecimal loanAmount;
}

Comparator

public class LoanAccountAmountComparator implements Comparator<LoanAccount> {

    @Override
    public int compare(LoanAccount o1, LoanAccount o2) {
        if (o1 == null) {
            if (o2 == null) {
                return 0;
            }
            return -1;
        }
        if (o2 == null) {
            return 1;
        }
        if (o1.getLoanAmount() == null) {
            if (o2.getLoanAmount() == null) {
                return 0;
            }
            return -1;
        }
        if (o2.getLoanAmount() == null) {
            return 1;
        }

        int comparedLoanAmount = o2.getLoanAmount().compareTo(o1.getLoanAmount());
        if (comparedLoanAmount == 0) {
            if (o1.getCreationDate() == null) {
                if (o2.getCreationDate() == null) {
                    return 0;
                }
                return -1;
            }
            if (o2.getCreationDate() == null) {
                return 1;
            }
            return o1.getCreationDate().compareTo(o2.getCreationDate());
        } else {
            return comparedLoanAmount;
        }
    }
}

Objects o1, o2 can be null. Fields creationDate and loanAmount can be null.

I want to ask you if there are better ways to handle null pointer exception in compareTo method.

For me it looks like there are too many if conditions

it is possible to handle null pointer exception using Comparator.comparing static method. If you want to comparing loanAmount fields in your objects and you want that every not null value is greater than null you can use Comparator.nullFirst method in combination with Comparator.comparing like the below code:

Comparator.comparing(LoanAccount::getLoanAmount,
                     Comparator.nullsFirst(Comparator.naturalOrder()))

NaturalOrder is a method returning a comparator that compares Comparable objects in natural order.
You can also combine more comparators in a chain with method Comparator.thenComparing like the code below:

Comparator.comparing(LoanAccount::getLoanAmount,
                Comparator.nullsFirst(Comparator.naturalOrder()))
                .thenComparing(LoanAccount::getCreationDate,
                Comparator.nullsFirst(Comparator.naturalOrder()))
                .compare(o1, o2);

Now you can rewrite your comparator with equal behaviour shortly:

public class LoanAccountAmountComparator implements Comparator<LoanAccount> {
    @Override
    public int compare(LoanAccount o1, LoanAccount o2) {
        if (o1 == null && o2 == null) return 0;
        if (o1 == null) return -1;
        if (o2 == null) return 1;
        return Comparator.comparing(LoanAccount::getLoanAmount,
                Comparator.nullsFirst(Comparator.naturalOrder()))
                .thenComparing(LoanAccount::getCreationDate,
                Comparator.nullsFirst(Comparator.naturalOrder()))
                .compare(o1, o2);
    }
}

Note: the use of class Date is discouraged, it is better if you use instead the java.time package classes for time related code.


Using decorated methods in a subclass of deque to dump its state to a JSON file I am sub-classing an object in python and I need to overload a bunch of the methods with very similar logic, essentially a call to super() and then some additional operations. I want to avoid explicitly writing out each of those overloaded methods in that way, so I implemented a decorator.

Is there anything wrong with doing the following:

def decorator(function):
    def wrapper(*args, **kwargs):
        # Logic + operations here
        function()
        # Logic and operations here

    return wrapper

class SubClass(SuperClass):
    method1 = decorator(SuperClass.method1)
    method2 = decorator(SuperClass.method2)

    def __init__(self, *args, **kwargs):
        super().__init__(*args, **kwargs)
        # Extra logic here

Here is my actual implementation:

def updates_file(function):
    """
    Decorator to call update file method after super call.
    """
    def wrapper(*args, **kargs):
        function(*args, **kargs)
        args[0]._update_file()

    return wrapper

class FileMirroredDeque(deque):

    __delitem__ = updates_file(deque.__delitem__)
    __iadd__    = updates_file(deque.__iadd__)
    __imul__    = updates_file(deque.__imul__)
    __setitem__ = updates_file(deque.__setitem__)
    append      = updates_file(deque.append)
    appendleft  = updates_file(deque.appendleft)
    extend      = updates_file(deque.extend)
    extendleft  = updates_file(deque.extendleft)
    insert      = updates_file(deque.insert)
    pop         = updates_file(deque.pop)
    popleft     = updates_file(deque.popleft)
    remove      = updates_file(deque.remove)
    reverse     = updates_file(deque.reverse)
    rotate      = updates_file(deque.rotate)

    def __init__(self, cache_path, maxlen=None, clean=False, file_indent=None):
        super().__init__((), maxlen)  # Initializes the deque as well TODO: Check the implication of this calling __setitem__

        self.path = cache_path
        self._indent = file_indent
        self._bak_file = None

        if clean:
            self._update_file()  # Overwrite contents of file.
            return  # Don't import data from file.

        with open(self.path, 'a+') as f:  # Creates file if it doesn't exist
            f.seek(0, 0)  # Seek back to beginning of file for json decode.
            contents = f.read()

        try:
            self.extend(json.loads(contents or "[]"))  # initializes internal list with persistent data or empty list.

        except json.decoder.JSONDecodeError:
            new_file = os.path.basename(self.path) \
                       + datetime.datetime.utcnow().strftime("_%Y_%m_%d_%H_%M_%S") + ".bak"

            new_path = os.path.join(os.path.dirname(self.path), new_file)

            logger.warning(f"File: {self.path} was not valid JSON. It will be copied to {new_path} and a new "
                           f"file will be created.")

            shutil.copyfile(self.path, new_path)
            self._bak_file = new_path

    def _update_file(self):
        with open(self.path, 'w') as f:
            json.dump(list(self), f, indent=self._indent)

I don't think that what you're doing is good style. I think you're adding a lot of extra complexity and places for things to go wrong, or become hard to understand, or whatever. I don't think you lose anything by just making very small methods that call super and your special method. I talk about those options at the bottom of the post.

With that out of the way, I think you can accomplish your goal as written and currently designed much more safely by using descriptors and metaclasses. I don't think this is as good as the methods at the bottom of the post, but to review the code as written here is what I would say.

What you're trying to do is (semi-automatically) change how certain data elements are retrieved and executed when called. To me, this sounds like a descriptor. Descriptors allow you to override how you set, get, or delete an attribute on an object, like so (example taken from above documentation link):

class RevealAccess(object):
    """A data descriptor that sets and returns values
       normally and prints a message logging their access.
    """

    def __init__(self, initval=None, name='var'):
        self.val = initval
        self.name = name

    def __get__(self, obj, objtype):
        print('Retrieving', self.name)
        return self.val

    def __set__(self, obj, val):
        print('Updating', self.name)
        self.val = val

>>> class MyClass(object):
...     x = RevealAccess(10, 'var "x"')
...     y = 5
...
>>> m = MyClass()
>>> m.x
Retrieving var "x"
10
>>> m.x = 20
Updating var "x"
>>> m.x
Retrieving var "x"
20
>>> m.y
5

Note - if there were a way to safely identify the underlying data structure being modified when any of these operations is called, then that is what you should put the descriptor on (as that's what you actually want). Because I don't know of a safe way to do that, I suggest the approach below.

For yours, what you need is a descriptor that auto-calls super for you. Note - it's important to use super to handle cases of multiple-inheritance; as pointed out in the comments, your current implementation will only ever call the deque method directly, instead of taking the time to deal with __mro__. In the implementation below, we:

Use the __get__ method to automatically get the class (objtype) and self (obj) of the instance (not the descriptor). This provides enough information for us to call super().
Use getattr to get the attribute in question from the result of super, and lets us ignore the complexities of __mro__.
Create a wrapper function (don't forget to use functools.wraps) that calls the wrapped function with the appropriate arguments, and then the extra function you specified.
__get__ returns the wrapped function, and now MyClass.<<attr_name>> will be that function

from functools import wraps

class AutoSuperDescriptor(object):
    def __init__(self, attr_name, extra):
        self.attr_name = attr_name
        self.extra = extra

    def __get__(self, obj, objtype):
        wrapped_function = getattr(
            super(objtype, obj), self.attr_name
        )
        extra_bits = self.extra

        @wraps(wrapped_function)
        def wrapper(yourself, *args, **kwargs):
            result = wrapped_function(
                yourself, *args, **kwargs
            )
            extra_bits(yourself)
            return result

        return wrapper

You create your descriptor by providing enough information to find the overwritten attribute, as well as the function we want to add on to the end. For example, you might do this:

class FileMirroredDeque(deque):
    append = AutoSuperDescriptor("append", updates_file)

If that meets your needs, then you're done.

But wait, there's more

It's pretty tedious and error-prone to do this by hand for each method you want to apply this to. Wouldn't it be nice if there was a way to just tie into the creation of a class and have it do this for us?

There is, and the answer is metaclasses. Metaclasses are a really complex, and really powerful, tool in the Python toolbox. Anytime you find yourself using them you should think really, really whether that is the best way to do it. I don't think this is one of those times (see start of this answer), but if you wanted to do that, I think we'd have to do something like this:

Define a way to identify the elements that should be overridden at class initialization.
Define a way to identify what extra behavior needs to be added to them.
Add the behavior.

The third one is easy - we implemented it with our descriptor up above.

The first two are a little more complicated, and have a few options. We could choose between adding kwargs to the class initialization:

class FileMirroredDeque(
    deque,
    metaclass=SuperChargerMetaclass,
    methods_to_supercharge = [""],
    method_to_supercharge_with = "method_name"
):

I think this would get clunky and obscure things. Instead, I'm going to propose that we define a special attribute (I used the __<<name>>__ naming convention, which is a no-no, but I thought made it clearer for demonstrative purposes) to list the attributes to override, and then set an attribute on the extra method itself to find it. The final implementation of the class looks like so:

def supercharger(func):
    func.__is_supercharger = True
    return func

class FileMirroredDeque(
    deque, metaclass=SuperChargerMetaClass
):
    __supercharge__ = [
        "__delitem__",
        "__iadd__",
        "__imul__",
        "__setitem__",
        "append",
        "appendleft",
        "extend",
        "extendleft",
        "insert",
        "pop",
        "popleft",
        "remove",
        "reverse",
        "rotate",
    ]

    @supercharger
    def _update_file(self):
        with open(self.path, "w") as f:
            json.dump(list(self), f, indent=self._indent)

    def __init__(
        self,
        cache_path,
        maxlen=None,
        clean=False,
        file_indent=None,
    ):
        super().__init__((), maxlen)

        self.path = cache_path
        self._indent = file_indent
        self._bak_file = None

        if clean:
            self._update_file()  # Overwrite contents of file.
            return  # Don't import data from file.

        with open(
            self.path, "a+"
        ) as f:  # Creates file if it doesn't exist
            f.seek(
                0, 0
            )  # Seek back to beginning of file for json decode.
            contents = f.read()

        try:
            contents = json.loads(contents or "[]")
            self.extend(
                contents
            )  # initializes internal list with persistent data or empty list.

        except json.decoder.JSONDecodeError:
            new_file = (
                os.path.basename(self.path)
                + datetime.datetime.utcnow().strftime(
                    "_%Y_%m_%d_%H_%M_%S"
                )
                + ".bak"
            )

            new_path = os.path.join(
                os.path.dirname(self.path), new_file
            )

            print(
                f"File: {self.path} was not valid JSON. It will be copied to {new_path} and a new "
                f"file will be created."
            )

            shutil.copyfile(self.path, new_path)
            self._bak_file = new_path

But how do we actually get this to happen? Like so!

class SuperChargerMetaClass(type):
    _SUPERCHARGER_ATTRNAME = "__supercharge__"

    @classmethod
    def _get_supercharger(cls, klass_attrs):
        return next(
            method
            for name, method in klass_attrs.items()
            if getattr(method, "__is_supercharger", False)
        )

    def __init__(cls, name, bases, attrs):
        supercharger = cls._get_supercharger(attrs)
        superchargees = attrs.get(
            cls._SUPERCHARGER_ATTRNAME, ()
        )

        for method_to_supercharge in superchargees:

            attrs[
                method_to_supercharge
            ] = AutoSuperDescriptor(
                method_to_supercharge, supercharger
            )

        return super().__init__(name, bases, attrs)

In the __init__ method (__new__ would probably work too, but I ended up getting this to work as __init__ so I left it), we get the "supercharger" method to append to our functions, and our list of "superchargees" that need the behavior added to them. From there, its just a matter of making the relevant attribute look to our descriptor instead of whatever (if anything) was there before.

Despite being personally proud of coming up with that, I still feel pretty strongly that its the wrong way to go. As such, I wanted to show how easy this would be. For example, if you inherit from deque:

class FileMirroredDeque(deque):

    # _update_file and __init__ unchanged

    def __delitem__(self, idx):
        super().__delitem__(idx)
        self._update_file()

    # Same for each method

If you don't actually want inheritance, or want to deal with multiple inheritance, then you could just use composition instead, like so:

class FileMirroredDeque:

    def __init__(self, cache_path, maxlen=None, clean=False, file_indent=None):
        # Only addition to __init__
        self.data = deque((), maxlen)

        # Any references to self with the intent of hitting the deque should now reference self.data

    def __delitem__(self, idx):
        self.data.__delitem__(idx)
        self._update_file()

    # Same for each method

We can actually take that a step further then, by creating a generic descriptor that handles file mirroring any arbitrary collection, like so:

class FileMirroredCollection:

    # This is virtually identical to your previous __init__
    def __init__(self, collection, cache_path, clean=False, file_indent=None):
        self.collection = collection
        self.path = cache_path
        self._indent = file_indent
        self._bak_file = None

        if clean:
            self._update_file()  # Overwrite contents of file.
            return  # Don't import data from file.

        with open(self.path, 'a+') as f:  # Creates file if it doesn't exist
            f.seek(0, 0)  # Seek back to beginning of file for json decode.
            contents = f.read()

        try:
            # You'd need to have some intelligent way to extend the arbitrary collection; duck typing is your friend here
            self.collection.extend(json.loads(contents or "[]"))  # initializes internal list with persistent data or empty list.

        except json.decoder.JSONDecodeError:
            new_file = os.path.basename(self.path) \
                       + datetime.datetime.utcnow().strftime("_%Y_%m_%d_%H_%M_%S") + ".bak"

            new_path = os.path.join(os.path.dirname(self.path), new_file)

            logger.warning(f"File: {self.path} was not valid JSON. It will be copied to {new_path} and a new "
                           f"file will be created.")

            shutil.copyfile(self.path, new_path)
            self._bak_file = new_path

    def _update_file(self):
        with open(self.path, 'w') as f:
            json.dump(list(self.collection), f, indent=self._indent)

    def __get__(self, obj, objtype):
        self._update_file()
        return self.collection

    def __set__(self, obj, value):
        self.collection = value
        self._update_file()

    def __delete__(self, obj):
        del self.collection
        self._update_file()

class FileMirroredDeque:

    def __init__(self, cache_path, clean=False, file_indent=None, max_len=None):
        self.data = FileMirroredCollection(deque((), max_len), cache_path, clean, file_indent)

        # other stuff

This still needs a bit more work (e.g. this doesn't handle self.data.append very well), but you can see how it might be useful to separate your concerns.

Lastly, there are a few other miscellaneous improvements that might benefit you, including:

Support for more formats - json is nice, but what if I want to serialize it another way?
Not writing to file every time; this will be inefficient if you have lots of small changes. Potentially include a flush method, or some way to identify that you need to write any recent changes.
As I allude to with the generic descriptor approach, supporting other data structures could be good - you might not want a deque forever, and as-is you're pretty locked down.