Configure Table cell in model class I'm trying to show data in UITablleView after getting it from an HTTP request. I heard that the best and professional way to do it is configure the cell and do the call request in model class then send it to the view controller.

Here is what I tried to do, but I think it's not going well. I know this is a wrong way but I wanted to show you what I got based on someone.

UITable View Cell

@IBOutlet weak var status : UILabel!
@IBOutlet weak var date : UILabel!
@IBOutlet weak var model : UILabel!

 func ConfigCell (car : Car){
  let urlstr = "http://localhost:8000/api/newuser/check"
  let url = URL(string: urlstr)
  guard let token = UserDataSingleton.sharedDataContainer.token else { return    }
  let headers = ["Authorization":"Bearer \(token)"]
  var statusCode: Int = 0
request(url! ,  method: .get,  encoding: JSONEncoding.prettyPrinted ,      headers: headers )
    .responseJSON { response in
        if let value: AnyObject = response.result.value as AnyObject? {
            //Handle the results as JSON
            let json = JSON(value)
            for (key, subJson) in json["allcar"] {
                if let status = subJson["status"].string {
                    self.status.text = status
                    if let date = subJson["created_at"].string {
                          self.date.text = date

                        if let model = subJson["model"].string {
                            self.model.text = model
                            let Status = [
                                Car(model: model, status: status, date: date)]

Car model

  class Car {

private var _mode : String?
private var _status : String?
private var _date : String?

var model : String{
return _model!
   }
    var status : String {
    return _status!
     }
      var date : String {
       return _date!
           }

init(model : String , status :String,date : String) {
        self._status = status
        self._model = model
        self._date = date
   }

Controller

class testViewController: UIViewController, UITableViewDelegate , UITableViewDataSource {

    let data = [Car]()

   @IBOutlet weak var tableview: UITableView!

      override func viewDidLoad() {

   tableview.delegate = self
   tableview.dataSource = self
    print(data.count)
     }

       func numberOfSections(in tableView: UITableView) -> Int {
          return 1
   }
         func tableView(_ tableView: UITableView, numberOfRowsInSection section:  Int) -> Int {
       return data.count
    }
         func tableView(_ tableView: UITableView, cellForRowAt indexPath: IndexPath) -> UITableViewCell {

     let cell = tableView.dequeueReusableCell(withIdentifier: "CarCell",for: indexPath) as! CarC

      let entry = data[indexPath.row]
       cell.date.text = entry.date
      cell.model.text = entry.model
      cell.status.text = entry.status
      return cell

There are lot of ways to organize an architecture. I will explain how do I understand some of them. Read about MVC and MVVC patterns.

First of all, it would be better do divide all code related to requests and code responsible for UI.
Creating a model class is a good way. Model can only keep and parse data. It shouldn't contain any business logic.
You can create one more type class. Such as presenter or viewModel. This class would manage all contains in this module. Contains the cars array in your case. And here you can ask the ServerInteractor class to run the requests.
All code related to requests would be better to keep in ServerInteractor (or LocalInteractor).
It is a good practice to write methods smaller if it is possible. For example in your cellForRowAt method you can divide all code such as "cell.date.text = entry.date" to the separate method

I wish  it would be useful )


Power menu (Shutdown, Reboot, Sleep) in ELISP I've written an interactive function which lets me shudown, reboot my machine as well as put it to sleep.

I'm using emacs and I'm just starting out getting into ELISP programming. As I'm a total beginner, I'd like to know what you would do differently in the following code to make it better, lighter and so on. Many thanks in advance.

(defun db/power-menu ()
  "Interactive menu for shutdown, reboot or sleep."
  (interactive)
  (let ((actions '("Shutdown" "Reboot" "Sleep")))
    (setq action (ivy-completing-read "What do you want to do?" actions ))
    (if (y-or-n-p (concat "Execute " action "? Unsaved progress will be lost. "))
        (let ((default-directory "/sudo::"))
          (cond ((equal action "Shutdown")
                 (shell-command "systemctl poweroff"))
                ((equal action "Reboot")
                 (shell-command "systemctl reboot"))
                ((equal action "Sleep")
                 (shell-command "systemctl suspend")))))))

Disclaimer: Total beginner here too. So take everything with a grain of salt.
That being said this looks perfectly fine. However, I suggest you to use completing-read instead of ivy-completing-read as it will provide more flexibility. Don't worry, if you use ivy-mode (or customize completing-read-function to ivy-completing-read), then completing-read will use ivy-completing-read.This is especially helpful if you want to copy your snippet to another Emacs configuration where ivy isn't installed.
Other than that, I guess you want to introduce action before you setq it. However, I'm not really sure whether it's necessary.
Note that I will probably use your db/power-menu in my configuration. Well done :)


Create player according to chosen difficulty I am coding a 2D board game. I let the user choose the difficulty level in the beginning, which influences the skills points of the player.

Here is the code I wrote for the Player class:

package model;

public class Player {

    // VARIABLES ---------------------------

    private Position position;
    private int score = 0;
    private int stepsLeft;
    private int fightingSkill;
    private int jokingSkill;
    private int visionScope = 2;
    private String skillChoice;

    // CONSTRUCTOR --------------------------

    public Player(Position position, int difficultyLevel) {
        this.position = position;

        switch(difficultyLevel) {
        case 1:
            this.stepsLeft = 150;
            this.fightingSkill = 5;
            this.jokingSkill = 5;

        case 2:
            this.stepsLeft = 150;
            this.fightingSkill = 2;
            this.jokingSkill = 2;

        case 3:
            this.stepsLeft = 100;
            this.fightingSkill = 2;
            this.jokingSkill = 2;

        case 4:
            this.stepsLeft = 10;
            this.fightingSkill = 1;
            this.jokingSkill = 1;
        }
    }

    public Player(int stepsLeft, int fightingSkill, int jokingSkill) {
        this.stepsLeft = stepsLeft;
        this.fightingSkill = fightingSkill;
        this.jokingSkill = jokingSkill;
        this.position = new Position(1,1);
        this.score = 0;
    }

    // METHODS ------------------------------

    public void move(Position destination) {
        setPosition(destination);
        stepsLeft -= 1;
    }

    public void increaseScore(int bonus) {
        score += bonus;
    }

    public void increaseStepsLeft(int bonus) {
        stepsLeft+= bonus;
    }

    public void increaseFightingSkill(int bonus) {
        fightingSkill += bonus;
    }

    public void increaseJokingSkill(int bonus) {
        jokingSkill += bonus;
    }

    // GETTERS

    public Position getPosition() {
        return position;
    }

    public int getXPosition() {
        return position.getX();
    }

    public int getYPosition() {
        return position.getY();
    }

    public int getScore() {
        return score;
    }

    public int getStepsLeft() {
        return stepsLeft;
    }

    public int getFightingSkill() {
        return fightingSkill;
    }

    public int getJokingSkill() {
        return jokingSkill;
    }

    public String getSkillChoice() {
        return skillChoice;
    }

    public int getVisionScope() {
        return visionScope;
    }

    // SETTERS

    public void setPosition(Position position) {
        this.position = position;
    }

    public void setSkillChoice(int choice) {
        switch(choice) {
        case 0:
            this.skillChoice = "joke";
            break;
        case 1:
            this.skillChoice = "fight";
            break;
        case 2:
            this.skillChoice = "magic";
            break;
        default:
            ;
        }
    }

    public String toString() {
        return "play \t"+position+"\t"+stepsLeft+"\t"+jokingSkill+"\t"+score+"\n";
    }
}

My questions

I chose to use a switch statement in the constructor, to create my player with features according to the difficulty level. I have the feeling that is maybe not the good practice to have such a switch in a constructor but can't really say why, and don't find a better way. Is there a better way to do this?
Any other comments about that class? I've programmed scientific code but am quite new to OOP and Java. Any comment is welcome.

There are several things that can be improved, in my opinion:

Whitespaces around operators make the code more readable (you use them almost everywhere except this method):

public String toString() {
    return "play \t" + position + "\t" + stepsLeft + "\t"
           + jokingSkill + " \t" + score + "\n";
}

Comments like // VARIABLES ---------- or // METHODS ---------- are useless (they do not contain any additional information). Adding comments for each individual method explaining what it does (especially for public ones) or what a field is used for is a good practice.
An alternative for using switch statement is creating an array of constants. It can look like this:

private static int[] stepsLeftForLevel = new int[]{150, 150, 100, 10};

...

public Player(Position position, int difficultyLevel) {
    ...
    stepsLeft = stepsLeftForLevel[difficultyLevel];
    ...
}

It makes the code more readable in my opinion because all "magic" numbers are located in one place (and the code becomes more concise).
There is no need to use this in a constructor unless the name of the field is shadowed by a parameter (that is, I would recommend using this only when it is necessary).


Project Euler #12 Java implementation The sequence of triangle numbers is generated by adding the natural numbers. So the 7th triangle number would be 1 + 2 + 3 + 4 + 5 + 6 + 7 = 28. The first ten terms would be:

1, 3, 6, 10, 15, 21, 28, 36, 45, 55, ...

Let us list the factors of the first seven triangle numbers:

1: 1
 3: 1,3
 6: 1,2,3,6
10: 1,2,5,10
15: 1,3,5,15
21: 1,3,7,21
28: 1,2,4,7,14,28
We can see that 28 is the first triangle number to have over five divisors.

What is the value of the first triangle number to have over five hundred divisors?

Here is my code :

package coderbyte;

public class Java {

    public static void main(String[] args) {

        long start = System.nanoTime();

        int number = 0;
        int index = 0;
        boolean found = false;

        while (!found) {
            int counter = 0;
            index++;
            number = index * (index + 1) / 2;
            for (int i = 1; i * i <= number; i++) {

                if (number % i == 0) {
                    counter = (number % i == i ? counter + 1 : counter + 2);
                    if (counter > 500) {
                        found = true;
                        break;
                    }
                }

            }

        }

        System.out.println(number);

        System.out.println(System.nanoTime() - start);
    }
}

Any tips on how to improve the runtime and possible shorten the code.

Your solution is not good as you use a single method, stuffing everything inside, with no order nor logic.

It must be rebuilt from scratch.

public class TriagularNumberWithManyDivisors {

Use long, descriptive names, Example is not acceptable by any standard.

static final int DIVISORS_WANTED = 500;

I may have fun running this code for any number of divisors and I really do not want to go haunting for a magic number to change inside the code.

public static int triangular(int n) {
    return n * (n + 1) / 2;
}

This is a function, a part of your program that does only one thing, the function name also serves as documentation.

public static int divisors(int n) {
    int counter = 0;
    int limit = (int) Math.sqrt(n);
    for (int i = 1; i <= limit; i++) {
        if (n % i == 0) {
            counter += (n % i == i ?  1 : 2);
        }
    }
    return counter;
}

The code for counting divisors was kinda mixed in the main loop, and you had to pay the cost for not extracting it to a function in terms of an ugly boolean flag and two nested loops.

public static void main(String[] args) {
    for (int i = 0; true ; i++) {
        if (divisors(triangular(i)) > DIVISORS_WANTED) {
            System.out.println(triangular(i));
            break;
        }
    }
}

The main method, it is incredibly trivial to understand what this does now. It calls the others methods in a high level fashion, highlighting the algorithm.


Using custom negotiator and custom formatter in Web API 2 Here's my custom formatter which I want to use for "application/x.product" content type:

public class ProductFormatter : MediaTypeFormatter
    {
        public ProductFormatter()
        {
            SupportedMediaTypes.Add(new MediaTypeHeaderValue("application/x.product"));
        }
        public override bool CanReadType(Type type)
        {
            return false;
        }
        public override bool CanWriteType(Type type)
        {
            var result = (type == typeof(Product) || type == typeof(IEnumerable<Product>));
            return result;
        }

        public override async Task WriteToStreamAsync(Type type, object value, Stream writeStream, HttpContent content, TransportContext transportContext)
        {
            List<string> productStrings = new List<string>();
            IEnumerable<Product> products = value is Product ? new Product[] { (Product)value } : (IEnumerable<Product>)value;
            foreach (Product product in products)
            {
                productStrings.Add(string.Format("{0},{1},{2}",
                product.ProductID, product.Name, product.Price));
            }
            StreamWriter writer = new StreamWriter(writeStream);
            await writer.WriteAsync(string.Join(",", productStrings));
            writer.Flush();
        }
    }

Registered as:

config.Formatters.Add(new ProductFormatter());

I also have a custom negotiator (also registered):

public class CustomNegotiator : DefaultContentNegotiator
    {
        public override ContentNegotiationResult Negotiate(Type type,
        HttpRequestMessage request, IEnumerable<MediaTypeFormatter> formatters)
        {
            // look for an appropriate formatter
            MediaTypeFormatter formatter = null;
            foreach(var f in formatters)
            {
                var mediaType = f.SupportedMediaTypes.Where(mt => mt.MediaType == "application/x.product").SingleOrDefault();
                if (mediaType != null)
                {
                    formatter = f;
                    break;
                }
            }
            if (request.Headers.UserAgent.Where(x => x.Product != null
            && x.Product.Name.ToLower().Equals("chrome")).Count() > 0 && formatter != null)
            {
                // if client is chrome then return superproduct and use custom formatter for it
                return new ContentNegotiationResult(formatter,
                new MediaTypeHeaderValue("application/x.product")
                );
            }
            else
            {
                return base.Negotiate(type, request, formatters);
            }
        }
    }

I also want to use this with application/x.product when a client is Google Chrome.

Is my way of searching an appropriate formatter okay, or is there a better way?

ProductFormatter

The WriteToStreamAsync() method could be improved by using the using keyword with the StreamWriter. By using using the flushing and disposing of the stream is done automatically.

Having a string.Format() like it is used together with the string.Join() is suboptimal. You should better add the product details to the List<string> and let the string.Join() to the work.

Implementing this points will lead to

public override async Task WriteToStreamAsync(Type type, object value, Stream writeStream, HttpContent content, TransportContext transportContext)
{
    List<string> productStrings = new List<string>();
    IEnumerable<Product> products = value is Product ? new Product[] { (Product)value } : (IEnumerable<Product>)value;
    foreach (Product product in products)
    {
        productStrings.Add(product.ProductID);
        productStrings.Add(product.Name);
        productStrings.Add(product.Price);
    }

    using (StreamWriter writer = new StreamWriter(writeStream))
    {
        await writer.WriteAsync(string.Join(",", productStrings));
    }
}

You should also take into account that object value could be null. Assuming that it is an IEnumerable<Product> if it isn't a Product type is suboptimal.

CustomNegotiator

The retrieving of the formatter should be extracted to a separate method otherwise the Negotiate() method would do to many things and would violate the single responsibility principle.

The way how you are doing the search is ok, but could be improved by extracting the "application/x.product" to a class constant.

This if

if (request.Headers.UserAgent.Where(x => x.Product != null
&& x.Product.Name.ToLower().Equals("chrome")).Count() > 0 && formatter != null)

should be improved by first checking if formatter != null and to use Any() instead of Count() > 0. By using Count() > 0 all of the items will be iterated over and counted. Using Any() will iterate only until any item is found for which the condition is true. Instead of x.Product.Name.ToLower().Equals("chrome") you should use the overloaded Equals() method which takes a StringComparision enum as second parameter. This buyes you two things. The first is the call to ToLower() isn't needed anymore and the second is that your code will pass the turkey test.

Implementing this points will lead to

private const string mediaType = "application/x.product";
private MediaTypeFormatter FindFormatter(IEnumerable<MediaTypeFormatter> formatters)
{

    foreach (var formatter in formatters)
    {
        var foundMediaType = formatter.SupportedMediaTypes.Where(mt => mt.MediaType == mediaType).SingleOrDefault();
        if (foundMediaType != null)
        {
            return formatter;
        }
    }

    return null;
}

public class CustomNegotiator : DefaultContentNegotiator
{
    public override ContentNegotiationResult Negotiate(Type type,
                HttpRequestMessage request, IEnumerable<MediaTypeFormatter> formatters)
    {

        var formatter = FindFormatter(formatters);

        if (formatter != null && request.Headers.UserAgent.Any(x => x.Product != null
                                 && x.Product.Name.Equals("chrome", StringComparison.OrdinalIgnoreCase)))
        {
            // if client is chrome then return superproduct and use custom formatter for it
            return new ContentNegotiationResult(formatter,
                            new MediaTypeHeaderValue(mediaType));
        }
        else
        {
            return base.Negotiate(type, request, formatters);
        }
    }
}


Stack implementation with a linked list Could anyone provide any constructive criticism? I'm trying to get a jump on data structures before classes start.

One thing is that I should add a copy constructor and assignment operator. Changes made: got rid of using namespace std.

As the code allows users to specify a cap, if they wish, for their stack any ideas of how I could signal that that particular stack is full? Currently I'm outputting a message from within the method but I don't like it when method handle output. What about a method that returns whether the stack is full?

#include <iostream>

using namespace std; // TESTING

template <typename T>
class Stack
{
public:
    Stack()
        : head(nullptr), capacity(0), currSize(0), sized(false)
    {}
    Stack(int cap)
        : head(nullptr), capacity(cap), currSize(0), sized(true)
    {}
    ~Stack();

    void push(T val);
    void pop();
    bool isEmpty() const { return head == nullptr; }
    int size() const;
    T top() const;

private:
    template <typename T>
    struct StackNode
    {
        StackNode()
            :data(0), next(nullptr)
        {}
        StackNode(T val, StackNode *ptr = nullptr)
            :data(val), next(ptr)
        {}

        T data;
        StackNode *next;
    };

    StackNode<T> *head;
    int capacity;
    int currSize;
    const bool sized;
};

template <typename T>
Stack<T>::~Stack()
{
    while (head != nullptr)
    {
        StackNode<T> *tmp = head->next;
        delete head;
        head = tmp;
    }
}

template <typename T>
void Stack<T>::push(T val)
{
    if (sized == false || currSize != capacity)
    {
        head = new StackNode<T>(val, head);
        ++currSize;
    }
    else cout << "stack full" << endl; //delete
}

template <typename T>
void Stack<T>::pop()
{
    if (head != nullptr)
    {
        StackNode<T> *tmp = head;
        head = head->next;
        delete tmp;
        --currSize;
    }
}

template <typename T>
int Stack<T>::size() const
{
    return currSize;
}

template <typename T>
T Stack<T>::top() const
{
    if (head != nullptr)
    {
        return head->data;
    }
    else return NULL;
}

int main()
{
    Stack<char> myStack(3);

    cout << "is isEmpty: " << myStack.isEmpty() << endl;

    cout << "push: "; myStack.push('H'); cout << myStack.top() << endl;
    cout << "push: "; myStack.push('G'); cout << myStack.top() << endl;
    cout << "push: "; myStack.push('B'); cout << myStack.top() << endl;
    cout << "push: "; myStack.push('D'); cout << myStack.top() << endl;
    myStack.pop();
    cout << "push: "; myStack.push('D'); cout << myStack.top() << endl;

    cout << "Size: " << myStack.size() << endl;
    cout << "Destroyed" << endl;  myStack.~Stack();

    cout << endl << endl;
    system("pause"); // TESTING
    return 0;
}

using namespace std; // TESTING

Judging by your comment, I'm guessing you're aware about the problems of exposing namespace members in the global scope. But did you know that you can using namespace inside scopes other than the global? It is fine if in your unit tests you want to expose the std stuff to make the code less verbose, but do that in the shortest possible scope then, in this case, inside main():
int main()
{
    // Make an exception since this is a unit test and expose
    // the whole Standard Library inside main(), so we don't have
    // to std:: qualify everything.
    using namespace std;

    // same as before ...
}

 cout << "Destroyed" << endl;  myStack.~Stack();

Very unusual for you to be calling the destructor for myStack. A destructor is called automatically when an object goes out of scope or is deleteed. There are very few cases where a programmer would manually call a destructor, those are not present here. If you just wan't to ensure the object is destroyed before the end of main(), to log some stuff, wrap its declaration inside a new scope. E.g.:
int main()
{
    {
        Stack<char> myStack(3);
        ...
    } // <== ~myStack() called here
}

This way you won't risk ending with a half destroyed object in your hands.

template <typename T>
T Stack<T>::top() const
{
    if (head != nullptr)
    {
        return head->data;
    }
    else return NULL;
    //          ^^^^ problem here!
}

That will not work for a T type that is not a pointer or integer. NULL can be assigned to integers because it is usually implemented as #define for 0. That's one of the reasons why you should use nullptr whenever possible (C++11). If you had used nullptr, your test with T=char would have failed and you would have noticed this problem.
The usual convention for a generic stack is to throw an exception if you try to access the top for an empty stack. Returning a "default" value is less generic and also makes it harder for the caller to detect errors. Returning a default also requires the type T to be default constructible, so don't do it.
Same goes for popping on an empty stack. Right now you are not generating any errors. You should consider throwing and exception. Deriving a StackUnderflow exception type from std::runtime_error might be a good idea. Then you can extend the concept to a StackOverflow error when trying to push to the bounded stack. Granted that you can do with a simple std::runtime_error, but defining custom exception classes is a nice exercise if that's your point for writing this implementation.

cout << "push: "; myStack.push('H'); cout << myStack.top() << endl;

Don't pack that many statements in a single line. People read shorter columns of text much faster. But not just that, mixing several statements together makes it a lot easier for a quick read to miss some important part of it. Put each statement in its own line.
cout << "push: ";
myStack.push('H');
cout << myStack.top() << endl;

Not a huge thing, but usually this kind of data structures uses std::size_t for things like size and capacity. That's an unsigned integer, which makes sense since the stack size is not meant to be negative, however, there is some discussion about avoiding unsigned integers, so that's up to you to decide if it is worth it.

Last thing is a bit of a big subject, which was introduced with C++11, called move semantics. Your code does some unnecessary copying of data on push and StackNode's constructor, which might affect performance when T is something other that a native type or pointer. I'll leave you with a couple references you can read to further learn about this:

SO: What are move semantics?

cppreference.com: Move constructors.


Show first n elements of array, and count elements not shown This code mimics the "likes" tooltip from Facebook.

Given an array with the list of users that liked the content, it shows the first n users and displays a final item with the count of the users not shown.

It works as expected, but I'm sure it could be better written.

<?  if ($count = $this->likes->count()) {
        $visible = 12;
        $hidden = $count > $visible ? $count - $visible : 0;
?>

<?      $r = 0;
        foreach ($this->likes as $like) {
?>
            <?= $like->name ?>
<?          $r++;
            if ($r >= $visible) { break; };
        }
        if ($count > $visible) {
?>
            and <?= $hidden ?> more&hellip;
<?      } ?>

<?  } ?>

Edit: Removed revised code after reading this meta post.

<?  if ($count = $this->likes->count()) {

It is more common to use the long form of the opening tag.  The second tag is only optionally supported by PHP installations.  Also, it can suddenly stop working if your host adjusts the configuration (possibly accidentally during an upgrade).

<?php
    if ($count = $this->likes->count()) {

It's also more common to put the code on a new line after the opening tag.  Unless you're opening and closing the PHP block on the same line.

If you're interested, there is a PHP Coding Standard.  Personally I don't always follow it, but I find it a good way to make decisions on issues that aren't important to me.  It also makes your code more portable.

            if ($r >= $visible) { break; };

I'd break this up into multiple lines for readability.

            if ($r >= $visible) {
                break;
            }

The final semi-colon is unnecessary there.  It doesn't harm anything (perhaps a wasted clock cycle on the null instruction), but neither does it contribute anything.

        if ($count > $visible) {

I might write this as

        if ( $hidden > 0 ) {

The reason being that you then go on to output $hidden.  You also might consider moving the definition of $hidden to be directly before this if statement.  That puts all the code related to $hidden together and makes it easier to follow the logic.

Alternately, you could stay with the original if and replace $hidden with $count - $visibility.  Then you don't need to define $hidden at all.


Which is a better optmized CSS for a button design? There are two ways I have been using to create links and style them as buttons:

background: url("/images/sing_up_btn.png") no-repeat scroll 0 0 transparent;
font-family: Arial,Helvetica,Sans-serif;
text-shadow: 0 -1px #444444;
text-transform: none;

Another one is:

a.btn_link
{
    background: #4468b2; /* Old browsers */
    background: -moz-linear-gradient(top,  #4468b2 0%, #3b5a9b 100%); /* FF3.6+ */
    background: -webkit-gradient(linear, left top, left bottom, color-stop(0%,#4468b2), color-stop(100%,#3b5a9b)); /* Chrome,Safari4+ */
    background: -webkit-linear-gradient(top,  #4468b2 0%,#3b5a9b 100%); /* Chrome10+,Safari5.1+ */
    background: -o-linear-gradient(top,  #4468b2 0%,#3b5a9b 100%); /* Opera 11.10+ */
    background: -ms-linear-gradient(top,  #4468b2 0%,#3b5a9b 100%); /* IE10+ */
    background: linear-gradient(top,  #4468b2 0%,#3b5a9b 100%); /* W3C */
    filter: progid:DXImageTransform.Microsoft.gradient( startColorstr='#4468b2', endColorstr='#3b5a9b',GradientType=0 ); /* IE6-9 */
    color:#fff;
    -webkit-border-radius: 4px;
    -moz-border-radius: 4px;
    border-radius: 4px;
    -webkit-box-shadow: 2px 2px 2px 0px rgba(0, 0, 0, .4);
    -moz-box-shadow: 2px 2px 2px 0px rgba(0, 0, 0, .4);
    box-shadow: 2px 2px 2px 0px rgba(0, 0, 0, .4);
  color: #FFFFFF;
  text-decoration:none;
  padding:5px 25px 5px 25px;
}
a.btn_link:active{
margin: 2px -2px 0px 2px;
box-shadow:none;
}

The image used as a background is almost the same as the second code creates a button design. I was wondering which is more efficient and optimized way to create button. With my little understanding, I would think that the one that doesn't uses image. But I have seen many new age well designed websites using images. Does that mean if I use such heavy CSS for a simple button, the code is not optimized?

I like the second option, it is easy to read if you just reformat it a little bit (also corrected your :active selector and fixed the shadow in IE9):

a.btn_link {
    background: #4468b2; /* Old browsers */
    background: -webkit-gradient(linear, left top, left bottom, color-stop(0%,#4468b2), color-stop(100%,#3b5a9b)); /* Chrome,Safari4+ */
    background: -webkit-linear-gradient(top,  #4468b2 0%, #3b5a9b 100%); /* Chrome10+,Safari5.1+ */
    background:    -moz-linear-gradient(top,  #4468b2 0%, #3b5a9b 100%); /* FF3.6+ */
    background:     -ms-linear-gradient(top,  #4468b2 0%, #3b5a9b 100%); /* IE10+ */
    background:      -o-linear-gradient(top,  #4468b2 0%, #3b5a9b 100%); /* Opera 11.10+ */
    background:         linear-gradient(top,  #4468b2 0%, #3b5a9b 100%); /* W3C */
    filter: progid:DXImageTransform.Microsoft.gradient( startColorstr='#4468b2', endColorstr='#3b5a9b',GradientType=0 ); /* IE6-9 */

    -webkit-border-radius: 4px;
       -moz-border-radius: 4px;
            border-radius: 4px;

    -webkit-box-shadow: 2px 2px 2px 0px rgba(0, 0, 0, .4);
       -moz-box-shadow: 2px 2px 2px 0px rgba(0, 0, 0, .4);
            box-shadow: 2px 2px 2px 0px rgba(0, 0, 0, .4);
    border-collapse: separate; /* http://stackoverflow.com/questions/5617455/issue-with-box-shadow-on-ie9/5617540#5617540 */

    color: #FFFFFF;
    text-decoration: none;
    padding: 5px 25px 5px 25px;
}
a.btn_link:active {
    margin: 2px -2px 0 2px;
    -webkit-box-shadow: none;
       -moz-box-shadow: none;
            box-shadow: none;
}

I would also add a simple hover affect like this:

a.btn_link:hover {
    background: #4468b2;
    filter: progid:DXImageTransform.Microsoft.gradient(enabled = false);
}

If you can, consider using a css preprocessor like LESS

.rounded-corners (@radius: 4px) {
    -webkit-border-radius: @radius;
       -moz-border-radius: @radius;
            border-radius: @radius;
}

.gradient-box (@baseColor) {
    @darker = darken(@baseColor, 6%);
    background: @baseColor; /* Old browsers */
    background: -webkit-gradient(linear, left top, left bottom, color-stop(0%,@baseColor), color-stop(100%,@darker)); /* Chrome,Safari4+ */
    background: -webkit-linear-gradient(top,  @baseColor 0%, @darker 100%); /* Chrome10+,Safari5.1+ */
    background:    -moz-linear-gradient(top,  @baseColor 0%, @darker 100%); /* FF3.6+ */
    background:     -ms-linear-gradient(top,  @baseColor 0%, @darker 100%); /* IE10+ */
    background:      -o-linear-gradient(top,  @baseColor 0%, @darker 100%); /* Opera 11.10+ */
    background:         linear-gradient(top,  @baseColor 0%, @darker 100%); /* W3C */
    filter: progid:DXImageTransform.Microsoft.gradient( startColorstr='@baseColor', endColorstr='@darker',GradientType=0 ); /* IE6-9 */

    color: white;
    text-decoration: none;
    padding: 5px 25px 5px 25px;

    &:hover {
        background: @baseColor;
        filter: progid:DXImageTransform.Microsoft.gradient(enabled = false);
    }
}

.pressable-floating-box (@distance: 2px, @blur: 2px, @spread: 0px) {
    -webkit-box-shadow: @distance @distance @distance 0 rgba(0, 0, 0, .4);
       -moz-box-shadow: @distance @distance @distance 0 rgba(0, 0, 0, .4);
            box-shadow: @distance @distance @distance 0 rgba(0, 0, 0, .4);

    border-collapse: separate; // http://stackoverflow.com/questions/5617455/issue-with-box-shadow-on-ie9/5617540#5617540

    &:active {
        margin-right: -@distance;
        margin-left: @distance;
        -webkit-box-shadow: none;
           -moz-box-shadow: none;
                box-shadow: none;
    }
}

a.btn_link {
    .rounded-corners;
    .gradient-box(#3b5a9b);
    .pressable-floating-box;
}

I really wouldn't be too worried about css efficiency, yes this does take a little more effort on the browser's part to render than a simple css background image, but the paths are all highly optimized already and this way you have defined the style instead of giving the browser a limited example (try a two line button with each, or zooming the document, or any other of the many ways to mess the button up in one or more browsers).

Comparing the http transfer efficiency of these two probably comes out in favor of the latter as well. The image version comes in 2 requests, while the pure css version is only one. I'd bet they both are fairly minimal on the total bandwidth compared to your page as a whole, but if you were really concerned about this you could always inline the image. If you want to go that route, here is a useful tool: http://www.greywyvern.com/code/php./binary2base64


Sorting program code I have done a sorting program, it works but I am new to the coding world and I would love to hear your thoughts.
    public class CorrectPlacement {

    public void placer(int... numbers) {
        System.out.println("Before : " + Arrays.toString(numbers) + " ");
        for (int i = -1; i <= numbers.length - 2; i++) {
            int min = numbers[i + 1];
            for (int number : numbers) {

                if (number <= min && indexofTheRight(number, numbers, i) > i) {
                    min = number;
                }
            }
            numbers[indexofTheRight(min, numbers, i)] = numbers[i + 1];
            numbers[i + 1] = min;
        }
        System.out.println("After : " + Arrays.toString(numbers));
    }

    public int indexofTheRight(int element, int[] array, int range) {

        for (int i = array.length - 1; i > range; i--) {
            if (array[i] == element) {
                return i;
            }
        }
        return -1;
    }
}

A few things.
for (int i = 0; i < numbers.length-1; i++)

Also, look up quick sort.  Worst-case performance O(n^2) with a best-case of O(nlogn).  By randomizing the pivot in the quick sort you can enhance the best case scenario.
Psuedo for recursive Quicksort:
partition(arr[], low, high)
    pivot = arr[high]
    i = low
    for j:= low to high-1
        if arr[j] <= pivot
            swap arr[i] with arr[j]
            i = i+1
   swap arr[i] with arr[high]

random(arr[], low, high)
    r = a random number from low to high
    swap arr[r] and arr[high]
    return partition(arr, low, high)

quickSort(arr[], low, high)
    if low < high
        p = random(arr, low, high)
        quickSort(arr, low , p-1)
        quickSort(arr, p+1, high)


Implementing the Hashable Protocol in Swift with the DJB hash function A while back I made a custom String struct (see github repo) because of the difficulties in dealing with Mongolian Unicode rendering when using Swift String or NSString (see this question and the accepted answer for more details). The struct is an array of UInt32 so that it represents a string of Unicode scalar values.

The most difficult part (see here and here for my StackOverflow questions at the time) was making it conform to the Hashable Protocol so that it could be used as a Dictionary key.

For all that I can tell, everything has been working fine. However, since there was not a lot of documentation and guidance available about implementing the Hashable Protocol, I wanted to make sure that I did it right. Here is the relevant code:

struct ScalarString: SequenceType, Hashable, CustomStringConvertible {

    private var scalarArray: [UInt32] = []

    // ...

    // hashValue needed to implement Hashable protocol
    var hashValue: Int {
        get {

            // DJB Hash Function
            var hash = 5381

            for(var i = 0; i < self.scalarArray.count; i++)
            {
                hash = ((hash << 5) &+ hash) &+ Int(self.scalarArray[i])
            }

            return hash
        }
    }

}

// Hashable also needs struct to conform to Equatable protocol
func ==(left: ScalarString, right: ScalarString) -> Bool {

    if left.length != right.length {
        return false
    }

    for var i = 0; i < left.length; ++i {
        if left.charAt(i) != right.charAt(i) {
            return false
        }
    }

    return true
}

The Hashable protocol has only one single requirement:

  Axiom: x == y implies x.hashValue == y.hashValue.

So let's start with your implementation of ==:

// Hashable also needs struct to conform to Equatable protocol
func ==(left: ScalarString, right: ScalarString) -> Bool {

    if left.length != right.length {
        return false
    }

    for var i = 0; i < left.length; ++i {
        if left.charAt(i) != right.charAt(i) {
            return false
        }
    }

    return true
}

Looking up the definitions of length and charAt() it is clear
that here simply the left.scalarArray and right.scalarArray
arrays are checked for equality. So the operator can equivalently
but simpler be implemented as

// Hashable also needs struct to conform to Equatable protocol
func ==(left: ScalarString, right: ScalarString) -> Bool {
    return left.scalarArray == right.scalarArray
}

From this representation it becomes obvious that your hashValue
implementation is correct: It is computed from the scalarArray
property, so equal objects have the same hash value.

The hashValue computed property can be simplified using
reduce(), note also that for a read-only property, you need
not put the getter method inside a get { } block:

// hashValue (to implement Hashable protocol)
var hashValue: Int {
    return self.scalarArray.reduce(5381) {
        ($0 << 5) &+ $0 &+ Int($1)
    }
}

A different question would be how "good" the hash is.
The Swift language does not make any requirements here. Always
returning 0 would be valid, but of course ineffective when
building large dictionaries.

It may be interesting in this context that the hash value of
the Foundation type NSArray is simply the number of elements,
regardless of the contents.

In your case, the DJB hash function is a well-known hash method
for strings, so I do not see any reasons not to use it.

Update: As of Swift 4.1, the compiler can synthesize
Equatable and Hashable for types conformance automatically, if all
members conform to Equatable/Hashable (SE0185). And as of Swift 4.2,
a high-quality hash combiner is built-in into the Swift standard
library (SE-0206).

Therefore there is no need anymore to define your own hashing function,
it suffices to declare the conformance:

struct ScalarString: Hashable, ... {

    private var scalarArray: [UInt32] = []

    // ...
}


Project Euler problem #14 (longest Collatz chain) in Swift This is my Swift code for finding the starting number, under one million, which produces the longest collatz chain.

var x = 1
var max = 0
var originalNum = 0

func collatzFunc(number: Int, originalNumber: Int){
    let oNum = originalNumber
    var num = number
    if num % 2 == 0{
        num = num/2
    } else {
        num = (3 * num) + 1
    }

    if num == 1{
        x += 1
    if max < x{
        max = x
        originalNum = oNum
    }
    return
    } else {
        x += 1
        collatzFunc(number: num, originalNumber: oNum)
    }
    }

for numbers in 1...1000000{
    x = 1
    collatzFunc(number: numbers, originalNumber: numbers)
}

print(originalNum)

This code is giving correct result but it's taking almost 5 seconds on my system to execute. I believe it can be done well under 1 sec. Where could I be wasting all that time?

Some suggestion to improve your code:

The global variables max and originalNum should be named differently to match their purpose, possibly with explaining comments.
The global variable x is used to keep track of the length of the current Collatz chain, this is better done by making the function return a value. That also allows to eliminate the second
originalNumber parameter.
collatzFunc() should just compute the Collatz chain length for a
given number, and the caller of that function should compare that with previously computed values.
Why assign var num = number if that value is then overwritten?
You can simplify that to

let num = number % 2 == 0 ? number / 2 : 3 * number + 1

The indenting is not always correct, and the spacing is not
consistent (e.g. always put a space before opening curly braces).
The challenge asks for a number below 1 million, therefore the
loop must be

for number in 1..<1000000 { ... }

and the singular form number is more appropriate here.

Putting it all together, this is how I would modify your code:

/// Compute the length of the Collatz chain starting at `number`.
func collatzFunc(number: Int) -> Int {
    if number == 1 {
        return 1
    }
    return 1 + collatzFunc(number: number % 2 == 0 ? number / 2 : 3 * number + 1)
}

var maxLength = 0 // Maximal length of Collatz chain found so far.
var maxNumber = 0 // Number for which this length is achieved.

for number in 1 ..< 1_000_000 {
    let length = collatzFunc(number: number)
    if length > maxLength {
        maxLength = length
        maxNumber = number
    }
}

print(maxNumber)

This is perhaps not faster than your version, but definitely better to read and therefore better to maintain.

You can also replace the recursive function by an iterative version

/// Compute the length of the Collatz chain starting at `number`.
func collatzFunc(number: Int) -> Int {
    var length = 1
    var num = number
    while num != 1 {
        num = num % 2 == 0 ? num / 2 : 3 * num + 1
        length += 1
    }
    return length
}

and check if that performs better.

As already mentioned in the comments, caching (or memoization)
increases the performance dramatically. You'll find some caching
solutions in the various answers to
Project Euler problem 14 (longest Collatz sequence) in Swift 3.
Here is a version using an array to cache all results for
number up to a certain limit, this turned out to be quite fast:

var cache = [Int](repeating: 0, count: 1_000_000)
cache[1] = 1

func collatzFunc(number: Int) -> Int {
    if number < cache.count && cache[number] > 0 {
        return cache[number]
    }
    let length = 1 + collatzFunc(number: number % 2 == 0 ? number / 2 : 3 * number + 1)
    if number < cache.count {
        cache[number] = length
    }
    return length
}


Program to see how many points lie strictly inside a convex polygon The program takes in a set of coordinates that defines the polygon. It then takes in a set of points. The idea is to find out how many of these points lie strictly inside the convex polygon (not on the edge or outside). First the program checks if the first set of points form a convex polygon or not. If yes, the program proceeds. Then each point is checked to see if it is strictly inside the convex polygon. If yes, count variable is increased. Here is the code:

#include <iostream>
#include <vector>

// Calculate perpendicular dot product
int perdot(const std::pair<int, int> a, const std::pair<int, int> b, const std::pair<int, int> c)
{
    int ab_x = b.first - a.first;
    int ab_y = b.second - a.second;
    int ac_x = c.first - a.first;
    int ac_y = c.second - a.second;
    int per_dot = ab_x * ac_y - ab_y * ac_x;
    if(per_dot < 0)
    {
        return -1;
    }
    else if(per_dot > 0)
    {
        return 1;
    }
    else
    {
        return 0;
    }
}

// Check if given set of points form a convex polygon
bool is_convex(const std::vector<std::pair<int, int>>& convex_polygon)
{
    int n = convex_polygon.size();
    int sense = perdot(convex_polygon[n - 1], convex_polygon[0], convex_polygon[1]);
    for(int i = 0; i < n - 1; i++)
    {
        int new_sense;
        if(i == (n - 2))
        {
            new_sense = perdot(convex_polygon[i], convex_polygon[i + 1], convex_polygon[0]);
        }
        else
        {
            new_sense = perdot(convex_polygon[i], convex_polygon[i + 1], convex_polygon[i + 2]);
        }
        if(sense == 0)
        {
            sense = new_sense;
        }
        if(new_sense == (-sense) && sense != 0)
        {
            return false;
        }
    }
    return true;
}

// Check if a point is STRICTLY inside the convex polygon
bool is_inside(const std::vector<std::pair<int, int>>& convex_polygon, const std::pair<int, int> point)
{
    int n = convex_polygon.size();
    int sense = perdot(convex_polygon[n - 1], point, convex_polygon[0]);
    if(sense == 0)
    {
        return false;
    }

    for(int i = 0; i < n - 1; i++)
    {
        int new_sense;
        new_sense = perdot(convex_polygon[i], point, convex_polygon[i + 1]);
        if(new_sense == (-sense) || new_sense == 0)
        {
            return false;
        }
    }
    return true;
}

// Count the number of points STRICTLY inside the convex polygon
int p_inside(const std::vector<std::pair<int, int>>& convex_polygon, const std::vector<std::pair<int, int>>& points)
{
    int count = 0;
    for(auto point : points)
    {
        bool inside = is_inside(convex_polygon, point);
        if(inside)
        {
            count++;
        }
    }
    return count;
}

// Main
int main()
{
    int k, n;
    std::cin >> k >> n;
    std::vector<std::pair<int, int>> convex_polygon(k);
    std::vector<std::pair<int, int>> points(n);
    for(size_t i = 0; i < convex_polygon.size(); i++)
    {
        int x, y;
        std::cin >> x >> y;
        convex_polygon[i] = {x, y};
    }
    bool convex = is_convex(convex_polygon);
    if(!convex)
    {
        std::cout << "Input not convex...Exiting" << std::endl;
        return 0;
    }

    for(size_t i = 0; i < points.size(); i++)
    {
        int x, y;
        std::cin >> x >> y;
        points[i] = {x, y};
    }

    int count = p_inside(convex_polygon, points);
    std::cout << "Points inside: " << count << std::endl;
    return 0;
}

1) Should std::pair be passed as reference? Is there an exact criteria to see if a given object is large enough to be passed as reference?

2) Is the use of const correct?

3) What other improvements can be made?

4) I am planning to test it extensively using Catch2 framework. I am learning this framework myself. What suggestions do you have to maintain professional test quality?

Data types

Why not use a simple point struct instead of std::pair<int, int>? It doesn't have to do anything, just a simple one would suffice:

struct point {
    int x = 0;
    int y = 0;
};

This would increase readability and generally might make the code more bug resistent. Remember, std::pair<int, int> can represent many different things: points, vectors, health + mana points, entry in a std::map<int, int> (which by itself can have many different meanings), exchange rate of apples for oranges, ...

Using a concrete type prevents that kind of confusion.
Similarly, one could make a type alias polygon for std::vector<point>, like

using polygon = std::vector<point>;

  A more advanced implementation could instead take a polygon as a range of points (= a pair of iterators), or use its own polygon class.

perdot

The name doesn't match what it actually does. Yes, the perpendicular dot product is used internally, but the returned value isn't the result of it. The returned value only indicates whether the point is left of, right of or on the line - and the function name should reflect that, e.g. something like determine_side.
The return values seem kinda arbitrary. Why not use an enum instead, like

enum class side {
    left,
    right,
    center
};

The order of the parameters seems kind of arbitrary and unintuitive. Why do the points of the "line" have to be passed as first and third parameter? With a simple help from the type system, this could be clarified a lot by introducing a small line helper struct:

struct line {
    point from;
    point to;
};

Putting all those fixes together (and refactoring the perpendicular dot product into its own helper function), we get:

int perpendicular_dot_product(point a, point b, point c) noexcept {
    const auto ab_x = b.x - a.x;
    const auto ab_y = b.y - a.y;
    const auto ac_x = c.x - a.x;
    const auto ac_y = c.y - a.y;

    return ab_x * ac_y - ab_y * ac_x;
}

side determine_side(line l, point p) noexcept {
    const auto per_dot = perpendicular_dot_product(l.from, p, l.to);

    if(per_dot < 0)
    {
        return side::right;
    }
    else if(per_dot > 0)
    {
        return side::left;
    }

    return side::center;
}

Much nicer to read and much clearer in its intent, right?

is_convex

Bug: If n <= 2 then the code likely doesn't work as intended, and might even access out of bounds memory. Please add checks for those cases.

  Now the question becomes: What should happen in those cases? Always return false, or throw an exception?

  I think always returning false would be better in this case, since it is just a query whether the contained data set conforms to some criteria. YMMV.

Also, there is another edge case if all points in convex_polygon are on a line. Currently, is_convex returns true for that case, but is this intended? I wouldn't assume so.

Other than that, it seems to be working fine. It would need some adjustments for the changes mentioned above, though, and could be slightly simplified.

bool is_convex(const polygon& convex_polygon) noexcept
{
    const auto n = convex_polygon.size();
    if(n <= 2) return false;

    auto sense = side::center;

    for(auto i = 0u; i < n; ++i)
    {
        auto new_sense = determine_side(line{convex_polygon[i], convex_polygon[(i + 2) % n]}, convex_polygon[(i + 1) % n]);

        if(sense == side::center)
        {
            sense = new_sense;
        }
        else if(new_sense != side::center && new_sense != sense)
        {
            return false;
        }
    }

    return sense != side::center;
}

is_inside

Not much to do here, other than adjusting for above changes and some slight simplification.

bool is_inside(const polygon& convex_polygon, const point p) noexcept
{
    const auto n = convex_polygon.size();

    const auto sense = determine_side(line{convex_polygon[n - 1], convex_polygon[0]}, p);
    if(sense == side::center)
    {
        return false;
    }

    for(auto i = 0u; i + 1 < n; ++i)
    {
        auto new_sense = determine_side(line{convex_polygon[i], convex_polygon[i + 1]}, p);

        if(new_sense != sense)
        {
            return false;
        }
    }
    return true;
}

p_inside

Again, the name is weird: Why not use count_points_inside?

Other than that, the logic seems fine. It could however easily be simplified using an algorithm provided by the standard library: std::count_if (inside header <algorithm>).

int count_points_inside(const polygon& convex_polygon, const std::vector<point>& points)
{
    return std::count_if(std::begin(points), std::end(points), [&](auto p) { return is_inside(convex_polygon, p); });
}

  Note how I didn't replace the type of points with polygon and used a std::vector<point> instead, since (at least for the human reader) there is a semantic difference between a polygon and a set of points.

  A better version would takes points as a range of points instead:

template<typename Iter, typename = std::enable_if_t<std::is_same_v<point, typename std::iterator_traits<Iter>::value_type>>
int count_points_inside(const polygon& convex_polygon, Iter first, Iter last)
{
    return std::count_if(first, last, [&](auto p) { return is_inside(convex_polygon, p); });
}

  (The typename = std::enable_if_t<...> part is only there to verify that the passed iterators point to points. The code would work without that.)

main

Errors should be printed to std::cerr instead of std::cout.
Some better instructions for the user would be nice. Currently, the user only ever gets notified if something goes wrong and thus has no clue what actually is expected of him.
The entering procedures of convex_polygon and points could be refactored out into a helper function, since the logic overlaps quite a lot.
In the error case, a different return value should be used, like 1 (or EXIT_FAILURE from the <cstdlib> header, to be more descriptive) in order to indicate that the program didn't execute as planned.
The final return 0; statement could be removed, as the compiler will autogenerate that one. (If it isn't removed, EXIT_SUCCESS could be returned instead as a more descriptive value).

Q & A

Generally, I try to pass everything but containers (including classes that contain containers as data members) and non-copyable types by value as default. I only switch to references if I either know that the type is very large, or performance measurements show that passing by reference is a performance benefit.

If in doubt, measure.
const is used well for function parameters. However, local variables that aren't being changed can also benefit from being marked const (see snippets above, e.g. for perpendicular_dot_product).
See above. Other than that, some functions like perpendicular_dot_product or determine_side could be made constexpr, as esote mentions in his answer. This would allow the compiler to precalculate those calls at compile time if the values are known.
I'd suggest trying test-driven development (i.e. write test before writing the actual implementation). Other than that, know your edge cases, and test those specifically (that way, you're unlikely to get bugs like the ones in is_convex).


Locking in MySQL stored procedure for deduplication Multiple upstream servers are sending notifications to a load-balanced django app. If all the upstream servers are working correctly, the app will always receive duplicates of these notifications (since all the upstream notifiers should send the same notifications; it's just for redundancy). I want to be able to filter out these duplicates. However, since the python app is load-balanced, the only place we can check for these duplicates is in the database (unless we did distributed locking, etc, which sounds like more of a pain).

To do this, I'm hashing the messages, then invoking a stored procedure in the database that checks if a message received in the last 10 seconds had the same hash (I know about hash collisions; it's a risk). I want to be 99% sure the store procedure is safe against race conditions.

Here's the SQL code that seems to work:

DROP TABLE IF EXISTS openduty_dedup;
CREATE TABLE openduty_dedup (
  request_hash CHAR(40) NOT NULL UNIQUE PRIMARY KEY,
  triggered TIMESTAMP NOT NULL);

-- main procedure for checking if a notification is a duplicate
-- given the SHA-1 hash of a notification, it will return 1 if the
-- same notification was received in the last 10 seconds (and was not
-- treated as a duplicate then), otherwise it will return 0
DROP PROCEDURE IF EXISTS openduty_check_duplicate;
CREATE PROCEDURE openduty_check_duplicate(IN new_hash CHAR(40))
  BEGIN
    DECLARE last_trigger TIMESTAMP;
    DECLARE now TIMESTAMP;
    DECLARE is_duplicate INT;
    SELECT CURRENT_TIMESTAMP INTO now;
    DO GET_LOCK('openduty_check_duplicate', 10);
      SET TRANSACTION ISOLATION LEVEL SERIALIZABLE;
      START TRANSACTION;
        SELECT triggered FROM openduty_dedup WHERE request_hash = new_hash INTO last_trigger;
        IF last_trigger IS NULL THEN
          INSERT INTO openduty_dedup (request_hash, triggered) VALUES (new_hash, now);
          SELECT 0 INTO is_duplicate;
        ELSEIF DATE_SUB(now, INTERVAL 10 SECOND) > last_trigger THEN
          UPDATE openduty_dedup SET triggered = now WHERE request_hash = new_hash;
          SELECT 0 INTO is_duplicate;
        ELSE
          SELECT 1 INTO is_duplicate;
        END IF;
        DELETE FROM openduty_dedup WHERE DATE_SUB(now, INTERVAL 1 MINUTE) > triggered;
      COMMIT;
    DO RELEASE_LOCK('openduty_check_duplicate');
    SELECT is_duplicate;
  END;

I'm much more worried about correctness than performance since notifications will only come on the order of every few seconds. In particular, I'm worried that the commit won't happen until after the lock is released.

Will this work as intended?

Will this work as intended?

Not really.

You've indented the code as if you expect that the statements betweeb DO GET_LOCK... and DO RELEASE_LOCK... would only execute if the lock is obtained.

That is not the case.

You shouldn't use GET_LOCK() in a void context (with DO) unless you don't actually care whether you actually got the lock.  What you have written will wait up to 10 seconds for the lock, and if it isn't available, continues anyway with the next statement.  That does not sound like what you want.

A better solution for MySQL 5.5 and later looks like this:

IF GET_LOCK('my_lock',10) IS NOT TRUE THEN
  SIGNAL SQLSTATE '45000'
         SET MESSAGE_TEXT = 'timed out waiting for named semaphore lock';
END IF;

That will throw an exception and terminate the procedure after the timeout, if the lock has not been obtained.  (This is a useful tactic with the event scheduler, when you need to ensure that concurrent invocations of events don't pile up.)

Note that the logical operator IS NOT TRUE, used above, matches both 0 (FALSE) and NULL, either of which can indicate that you didn't get the lock.

The same is true of RELEASE_LOCK(), which returns TRUE only if you actually held the lock you tried to release.  If you try to release a lock you don't own, that does not throw an error.  You have to check return values.

  In particular, I'm worried that the commit won't happen until after the lock is released.

No, the lock is released after the commit, here.  The get and release operations are only functions.  They execute in sequence like everything else.

Also, it's usually best to leave the caller in charge of the transaction, because if this procedure is called while already in a transaction, START TRANSACTION silently commits the previous transaction and starts a new one, leaving the caller unable to roll back any work done before the procedure was called.  That may not be applicable, here, but it's good practice to handle it consistently across your code base.


Is this too much work for 1 listener? Is this too much work for 1 touch listener? Is this bad for devices with slow CPU's?

@Override
     public boolean onTouch(View v, MotionEvent event) {
        int id = v.getId();
        switch(id){
                      case R.id.sound1:
                          if(event.getAction()==android.view.MotionEvent.ACTION_DOWN){

                          if(intLoop ==0){
                              mp1=MediaPlayer.create(MainClass.this, R.raw.item1);
                              mp1.start();
                              v.setPressed(true);
                              return true;
                          }
                          if(intLoop == 1){
                              mp1=MediaPlayer.create(MainClass.this, R.raw.item1);
                              mp1.start();
                              mp1.setLooping(true);
                              v.setPressed(true);
                              return true;
                          }
                          }else if
                              (event.getAction()==android.view.MotionEvent.ACTION_UP){
                              mp1.stop();
                              mp1.reset();
                              mp1.release();
                              v.setPressed(false);
                              clickCounter();
                              return false;
                              }
                         break;

Dodging your question for a quick comment:

Why don't you combine these

                      if(intLoop ==0){
                          mp1=MediaPlayer.create(MainClass.this, R.raw.item1);
                          mp1.start();
                          v.setPressed(true);
                          return true;
                      }
                      if(intLoop == 1){
                          mp1=MediaPlayer.create(MainClass.this, R.raw.item1);
                          mp1.start();
                          mp1.setLooping(true);
                          v.setPressed(true);
                          return true;
                      }

into this?

                      mp1=MediaPlayer.create(MainClass.this, R.raw.item1);
                      mp1.start();
                      if(intLoop == 1){
                          mp1.setLooping(true);
                      }
                      v.setPressed(true);
                      return true;

And, as Vogel612 mentions, is there any reason intLoop is not a boolean?

I think that if the rest of your code looks like that, simply combining the functionality like this might fix the idea that the listener is doing a lot.


Simple countdown programs to print "3, 2, 1, Action!" using while loop My program is a simple while loop making a countdown starting from 3 and shouts "Action!" upon reaching zero and then breaks. I would like to know which of the following ways is better in means of clarity and functionality. I am aware that there are other ways to achieve the same thing. I would like to hear your thoughts and suggestions.

countDown = 3
while (countDown >= 0):
    print(countDown)
    countDown = countDown - 1
    if countDown == 0:
        print("Action!")
        break

or

countDown = 3
while (countDown >= 0):
    if countDown != 0:
        print(countDown)
        countDown = countDown - 1
    else:
        print("Action!")
        break

Why does it has to be a while?

Maybe it is preference but for loops look cleaner.

Reversing can be done by using reversed function which reverses a iteration

countdown = 3
for count in reversed(range(1, countdown+1)):
    print(count)
print('action!')

Or you could use the step parameter in range(start, end, step) and rewrite the for loop to

for count in range(countdown, 0, -1):


Searching all diagonals of a 2D M x M array I've started writing a piece of code to help me search for an object in all the objects found in the diagonals of an M x M 2D array.

Though the code works, I'd like to know if there is a way I can improve on it or there exists a different technique I could use to achieve the same result with fewer lines of code. I just don't want to re-invent the wheel.

// board must be a square matrix
bool allDiagonals(string[] board, string strToFind)
{
    string s = "";
    for (int col = board[0].Length - 1; col >= 0; col--)
    {

        int row = 0;
        int j = col;
        s = "";
        while (j <= board[0].Length - 1)
        {
            s += board[row++][j++];
        }
        if (s.Contains(strToFind))
            return true;
    }

    for (int row = board.Length - 1; row > 0; row--)
    {
        int colmin = 0;
        s = "";
        for (int i = row; i < board.Length; i++)
        {
            s += board[i][colmin++];
        }
        if (s.Contains(strToFind))
            return true;
    }

    for (int col = 0; col <= board[0].Length - 1; col++)
    {

        int row = 0;
        int j = col;
        s = "";
        while (j >= 0)
        {
            s += board[row++][j--];
        }
        if (s.Contains(strToFind))
            return true;
    }
    for (int row = board.Length - 1; row > 0; row--)
    {
        s = "";
        int colmax = board[0].Length - 1;
        //scan from bottom right to main diagonal
        for (int i = row; i < board.Length; i++)
        {
            s += board[i][colmax--];
        }
        if (s.Contains(strToFind))
            return true;
    }
    return false;
}

This reminds me of what I wrote a while ago for my Tic Tac Toe Ultimate game.

In case I get anything wrong, here is a relevant part of my original code:

// Scan diagonals for a winner: Bottom-right
for (int yy = 0; yy < board.getSizeY(); yy++) {
    newWin(conds, consecutive, loopAdd(board, 0, yy, 1, 1));
}

private static List<Winnable> loopAdd(HasSub<? extends Winnable> board,
        int xx, int yy, int dx, int dy) {
    List<Winnable> winnables = new ArrayList<>();

    Winnable tile;
    do {
        tile = board.getSub(xx, yy);
        xx += dx;
        yy += dy;
        if (tile != null)
            winnables.add(tile);
    }
    while (tile != null);

    return winnables;
}

When making this in C#, you can return IEnumerable<char>. The idea of the method is to start at a specific point and to iterate until you're outside the valid area.

I think it's a bad idea that you're currently using String concatenation first, and then checking if the string contains the char. Instead you can indeed as Carsten points out in the comments use a state machine for that. It only needs to be about an int really that keeps track of how many consecutive matches you've had so far, and when it reaches a non-match it starts over at zero. When it reaches the string length, it returns true.

Anyways, back to the main part, here's what I would try to do:

private IEnumerable<char> loopOver(string[] board,
        Point position, Point delta) {

    char value;
    while (true) {
        if (!charIsInsideBoard(board, position))
            break;

        value = getCharInBoard(board, position);
        position.x += delta.x;
        position.y += delta.y;
        yield return value;
    }
}

Whenever I'm using C#, I love using the wonderful yield keyword.

private boolean contains(IEnumerable<char> loop, string searchFor) {
    int matches = 0;
    foreach (char ch in loop) {
        if (ch == searchFor[matches]) {
             matches++;
        }
        else {
             matches = 0;
             if (ch == searchFor[matches]) {
                 matches++;
             }
        }
        if (matches == searchFor.Length) {
             return true;
        }
    }
    return false;
}

Then in your allDiagonals method (which really could be renamed to searchAllDiagonals) all you need to do is to call the contains and loopOver methods a couple of times.

boolean searchAllDiagonals(string[] board, string searchFor)
{
    boolean result = false;
    for (int i = 0; i < board[0].Length; i++) {
        result = result || contains(loopOver(board, new Point(i, 0), new Point(1, 1), searchFor);
    }
    for (int i = 0; i < board.Length; i++) {
        result = result || contains(loopOver(board, new Point(0, i), new Point(1, 0), searchFor);
    }
    ...
}

Please note that I have not tested this code, and as I haven't done any C# development since last time I might have mixed something up. I also might have mixed up X and Y, but that happens for me in Java too every now and then.

Other Suggestions:

Don't use i and j when you're using nested loops. row and col are so much better, or x and y. I myself use xx and yy sometimes for loops when I already have another x / y variable.
As your variable is named board, what do I know, perhaps your whole point is to check for a winner in a Tic Tac Toe game. If a game is your context, then I'd recommend not using string[]. Use a BoardField[][] variable instead, or preferably wrap such a 2D array in a Board class (which can contain the charIsInsideBoard and getCharInBoard methods).


Checking if a number is a perfect square (without Math) I typed this up quickly as an intuitive solution for determining if a given number is a perfect square.

How could I improve this? I already know that it doesn't handle the case where the number is 1. I'm wondering if I could generalize the calculation so it rounds up. What can I look into to develop a mindset for tackling these sorts of problems?

int number = 1000000;
int half = (number + 2 - 1)/2;
int square = 0;
boolean isSquare = false;

for( int i = half; i > 0 ; i-- ){

    square = i*i;

    if( square > number )
        continue;

    if( square == number ){
        System.out.println( "This number is a perfect square" );
        isSquare = true;
        break;
    }

}

if( !isSquare ){
    System.out.println( "This number is not a perfect square" );
}

You could make use of the fact that n² = the sum of the first n odd numbers (for integral n ≥ 0), rather than having to compute i * i every time through the loop.


Typescript typed Map I have the following code:
    export type AvatarSize = "xs" | "sm" | "md";

    const sizes = new Map<AvatarSize, 32 | 44 | 104>([
      ["xs", 32],
      ["sm", 44],
      ["md", 104]
    ]);

To me, it feels dirty. I wonder if there is any other way to write it without hard-coding the sizes?
Thanks!

You can declare the array passed into the constructor as const so it doesn't get widened to Array<[string, number]>, and use generics to look up the key on the constructed map to get the AvatarSize type:
const sizes = new Map([
    ["xs", 32],
    ["sm", 44],
    ["md", 104]
] as const);
export type AvatarSize = Parameters<typeof sizes.get>[0];

Another way of doing it would be to declare the array outside:
const sizesArr = [
    ["xs", 32],
    ["sm", 44],
    ["md", 104]
] as const;
const sizes = new Map(sizesArr);
export type AvatarSize = (typeof sizesArr)[number][0];


Given a binary tree, \$T\$ whose nodes have positive values, find the value of the maximal path Problem

Problem is Maximum sum path from https://firecode.io

Given a binary tree, \$T\$, whose nodes have positive values find the value of a maximal path. A path is a simple path from a node to another node. The value of a path \$P\$ is

$$val(P) = \sum\limits_{u \in P} val(u)$$

that is, the sum of the values of the nodes along \$P\$. A maximal path \$M\$ is one that satisfies:

$$\forall\ \ paths\ \ P,\ \ val(P) \le val(M)$$

Python solution

class BinaryTreeNode:
    def __init__(self, data, left = None, right = None):
        self.data = data
        self.left = left
        self.right = right

class BinaryTree:
    def __init__(self, root = None):
        self.root = root

    def max_sum_path(self, root):

        _, val_of_max_path = self._val_of_max_path(root)

        return val_of_max_path

    def _val_of_max_path(self, root):
        ''' Returns a tuple:
            ( val of the maximal path
              from a leaf up to root,

              val of the maximal path
              from a node to another
              node in this subtree )
        '''

        # base case
        if not root:
            return (0, 0)

        # recursive case

        ( val_of_max_path_from_a_leaf_up_to_left_child,
          val_of_max_path_in_left_subtree ) = (

              self._val_of_max_path(root.left) )

        ( val_of_max_path_from_a_leaf_up_to_right_child,
          val_of_max_path_in_right_subtree ) = (

              self._val_of_max_path(root.right) )

        val_of_max_path_from_a_leaf_up_to_root = root.data + max(

            val_of_max_path_from_a_leaf_up_to_left_child,
            val_of_max_path_from_a_leaf_up_to_right_child )

        val_of_max_path_that_has_a_turning_point_at_root = (

            val_of_max_path_from_a_leaf_up_to_left_child +
            root.data +
            val_of_max_path_from_a_leaf_up_to_right_child )

        val_of_max_path_in_this_subtree = max(

            val_of_max_path_in_left_subtree,
            val_of_max_path_in_right_subtree,
            val_of_max_path_that_has_a_turning_point_at_root )

        return ( val_of_max_path_from_a_leaf_up_to_root,
                 val_of_max_path_in_this_subtree )

Comments

I use very long descriptive variable names. I am not sure if I am overdoing it or not. Do you see a way to shorten them whilst maintaining meaning? I have an upcoming technical interview so I want to make sure I exhibit good style. Also, I will be coding in a shared Google Doc, so I won't have access to syntax highlighting. Any tips or suggestions are greatly appreciated.

The combination of the very long variable names and trying to follow the PEP8 "79 chars per line" rule really makes the code unreadable.

First of all, you can absolutely increase the allowed line length from 79 to a more practical 100 or 120. After all, we are writing code for humans to read, not machines.

Also, I think you can shorten the variable names by removing the "val_of_" part from the beginning. You don't lose any information by doing that.

Some other notes:

you can define tuples without extra parenthesis, e.g. return 0, 0 instead of return (0, 0)
the docstrings should be put in triple double-quotes. If a docstring is located on multiple lines, it should star with a new line (PEP8 reference)

Here is the code after applying the proposed changes:

class BinaryTree:
    def __init__(self, root=None):
        self.root = root

    def max_sum_path(self, root):
        _, max_path = self._max_path(root)
        return max_path

    def _max_path(self, root):
        """
        Returns a tuple:
            (val of the maximal path from a leaf up to root,
             val of the maximal path from a node to another node in this subtree).
        """

        # base case
        if not root:
            return 0, 0

        # recursive case
        max_path_from_a_leaf_up_to_left_child, max_path_in_left_subtree = self._max_path(root.left)
        max_path_from_a_leaf_up_to_right_child, max_path_in_right_subtree = self._max_path(root.right)

        max_path_from_a_leaf_up_to_root = root.data + max(max_path_from_a_leaf_up_to_left_child,
                                                          max_path_from_a_leaf_up_to_right_child)

        max_path_that_has_a_turning_point_at_root = max_path_from_a_leaf_up_to_left_child + \
                                                    root.data + \
                                                    max_path_from_a_leaf_up_to_right_child
        max_path_in_this_subtree = max(max_path_in_left_subtree,
                                       max_path_in_right_subtree,
                                       max_path_that_has_a_turning_point_at_root)

        return max_path_from_a_leaf_up_to_root, max_path_in_this_subtree


Mongo command to add a field to all documents in a collection This code appends a boolean column to all documents in the birds collection.

db.birds
  .find()
  .snapshot()
  .forEach(function(el) {
    el.hi = true
    db.birds.save(el)
  })

Here's how the command is run in production.

mongo db_stuff/db_name -u username -p password theScript.js

This command runs very slowly.  Should I look into the $set operator or something else that will speed this up?

This is a lot faster:

db. birds.updateMany(
   {},
   { $set:
      {
        hi: true
      }
   }
)


Topological sort using recursive DFS I am currently learning Graph algorithms by solving questions on online judges.  The below code is for implementing Topological sort, using recursive DFS.  Also, it is my first time with C++ STL. Kindly review my working code below and provide me with feedback.  The exact question for the below code is here.

#include<cstdio>
#include<set>
#include<list>
#include<stack>
#include<algorithm>
#include<vector>
#include<utility>
#include<functional>

struct node
{
    int d, f, value;

    bool operator< (const node &rhs) const
    {
        if(f<=rhs.f && f>=rhs.f)
            return value<rhs.value;
        else
            return f<rhs.f;
    }
};

std::vector< std::pair<node, node> > Edges;
std::set<node> s;
bool *visited;
int N, myTime=0,i=0;
node node1, node2;
void dfsVisit(node);

void dfs()
{
    for(std::vector< std::pair<node, node> >::iterator it=Edges.begin(); it!=Edges.end(); it++)
        if(it->first.value<N)
            if(!visited[it->first.value])
                dfsVisit(it->first);
}

void dfsVisit(node n)
{
    myTime++;                           //increment myTime
    n.d=myTime;                         //set the discovery time for node n

    if(n.value<N)
        if(visited[n.value])
            return;

    for(std::vector< std::pair<node,node> >::iterator it=Edges.begin(); it!=Edges.end(); ++it)
    {
        if(it->first.value==n.value && !visited[it->second.value])
        {
            dfsVisit(it->second);
        }
    }

    visited[n.value]=true;
    myTime++;
    n.f=myTime;

    //printf("The discovery and finishing times of %d are: %d, %d\n",n.value+1,n.d,n.f);
    //printf("Inserting %d into the set.\n",n.value+1);
    s.insert(n);
}

int main()
{
    int M, firstOfRule, secondOfRule, data, i;
    scanf("%d""%d",&N,&M);
    visited=new bool[N];

    for(i=0;i<N;i++)
        visited[i]=false;

    while(M--)
    {
        scanf("%d",&firstOfRule);
        scanf("%d",&secondOfRule);

        while(secondOfRule--)
        {
            scanf("%d",&data);
            node1.value=firstOfRule-1;
            node2.value=data-1;
            Edges.push_back(std::make_pair(node1,node2));
            printf("Connected %d and %d\n",node1.value+1,node2.value+1);
        }
    }

    dfs();

    for(std::set<node>::const_iterator it=s.begin(); it!=s.end(); it++)
        printf("%d ",it->value+1);

    return 0;
}

Sample input would be as follows:

  5 4
  3 2 1 5
  2 2 5 3
  4 1 3
  5 1 1

And the expected output is:

  1 5 3 2 4

First, there are some problems with your code:

You allocate memory for bool *visited but never free it again. Use delete[] visited when you don't need it anymore.
The condition in if(f<=rhs.f && f>=rhs.f) is true if and only if f == rhs.f. If that was what you wanted to check, then code it that way.
In line 26 you define the global variable i which is never used because it is hidden by the definition of a local i in main(), line 66.

These aside your code could be improved in the following ways:

Avoid mixing C and C++ programming styles

When choosing C++ and STL, go all the way. You already use std::vector for storing edges and nodes, so use one for visited. A vector does memory allocation and deallocation for you which is less error prone.
Use C++'s std::cout and std::cin instead of C's printf and scanf functions. See this answer for reasons why.
Stick to a set of rules for formatting your code. For example, you're inconsistent when it comes to spaces in variable definitions (see lines 26 and 27) and for statements (see lines 32 and 70) and when using {} brackets with single statement if and for scopes (see loops in lines 32 and 47).
Don't use global variables unless you have to. The definitions of node1 and node2 for example can be moved where they are needed, and myTime could be changed into a static variable of your dfsVisit function. To eleminate the other global definitions you can for example define them in main and pass them as needed by reference to dfs.
Move the definition of i inside the for statement where it is used. There is no need for it outside the fors scope.
Do bounds checking as early as possible. You know that M and N have to be between 0 and 100, so check them once you read them. Also do something when you detect an error, don't just silently ignore invalid node values.
Use node values starting by 1. Currently you read the value, decrease it by 1 and add 1 again to print it. Your program never relies on zero based node values.
Set node1.value once per rule by moving the statement one scope up because if never changes in the while loop at line 78.
When swapping the order of your dfsVisit and dfs functions you don't need the forward declaration of dfsVisit anymore.
Your nodes get copied alot. Each edge gets a copy of two nodes. You could instead use a std::map<int, node> for storing your nodes by value. An edge is then simply a pair of node values.
Consider an extra struct edge instead of using std::pair. Yes, it is extra code you don't necessarily need, but using edge.source and edge.target is more expressive than edge.first and edge.second which could be anything.
visited is a property of a node, so add bool node::visited and use this value instead of an external variable. Doesn't work unless you also implement #10, because the other edges' copies of the node don't get automagically updated when you set node.visited to true.

Consider an object oriented style instead of functional programming

If you encapsulate your algorithm in a class you can easily get rid of the global variables. Implement class methods for data input and validation and for the actual DFS. You could even split data management and DFS implementation into separate classes.


PANDAS code for calculating distance between waypoints I've written some python code designed to take a csv of waypoints for a series of trips, and calculate the distance of each trip by the sum of the distance between the waypoints.

An example csv might be:

9e77d54918dd25c3f9d2e5354ec86666,0,2015-10-01T14:14:15.000Z,45.0988,7.5811,,
9e77d54918dd25c3f9d2e5354ec86666,1,2015-10-01T14:17:15.000Z,45.0967,7.5793,,
9e77d54918dd25c3f9d2e5354ec86666,2,2015-10-01T14:20:15.000Z,45.1012,7.6144,,
9e77d54918dd25c3f9d2e5354ec86666,3,2015-10-01T14:23:15.000Z,45.0883,7.6479,,
9e77d54918dd25c3f9d2e5354ec86666,4,2015-10-01T14:26:15.000Z,45.0774,7.6444,,
ect...

I've got code working, using pandas and numpy, however I'm entirely self-taught and I want to know if there's any serious or obvious mistakes I'm using that might make my code inefficient. It currently takes quite a while to run, I'm guessing because of my for loop. The code I'm using is:

import pandas as pd
import numpy as np
from math import radians, cos, sqrt

def dist(lat1, lon1, lat2, lon2): #short distances using Equirectangular approximation
    lat1, lon1, lat2, lon2 = map(radians, [lat1, lon1, lat2, lon2])
    x = (lon2 - lon1) * cos( 0.5*(lat2+lat1) )
    y = lat2 - lat1
    D = 6371 * sqrt(x**2 + y**2)
    return D

waypoint = pd.read_csv('TripRecordsReportWaypoints.csv',sep=',',header=None, usecols=[0,3,4], names=['TripID','Lat','Lon'])
output = pd.DataFrame(columns = ['TripID','Distance','No. of Waypoints'])

tripList = waypoint['TripID'].tolist() #creates list of tripids
tripList = list(set(tripList)) #makes list unique

for ID in tripList:
    temp = waypoint.loc[waypoint['TripID'] == ID] #creates a temporary dataframe with all waypoint for each trip

    temp['endLat'] = temp['Lat'].shift(periods=-1) #adds two columns with next waypoints lat and lon
    temp['endLon'] = temp['Lon'].shift(periods=-1)

    temp['Distance']=np.vectorize(dist)(temp['Lat'],temp['Lon'],temp['endLat'],temp['endLon']) #calculates distance, can change function 'dist' for more accuracy
    SumDist = temp['Distance'].sum() #calculates the total distance
    trpId = temp['TripID'].iloc[0] #takes the tripid
    wpcount = temp.shape[0] #length of dataframe
    temp2 = pd.DataFrame([[trpId,SumDist,wpcount]],columns=['TripID','Distance','No. of Waypoints']) #creates a single row dataframe with the total distance
    output = pd.concat([output,temp2]) #adds the row to the output
output.to_csv('TripDistances.csv',sep=',')

Your code can be greatly simplified when using pandas.DataFrame.groupby. This function groups a dataframe by some key(s) and then allows performing functions that act on the whole sub-dataframe (henceforth called group) using apply or apply some aggregating function to single columns of that group using aggregate.

For this to work, we need to define a dist function that can take a DataFrame and calculate the total distance of that DataFrame. Note that I will assume that each trip is sorted within the csv, otherwise you will have to add this there as well.

To make the dist function, we need to make sure all functions it uses are vectorized, so I will be using np.cos and np.sqrt instead of the math ones. I also defined a constant, EARTH_RADIUS, because you might want to change the precision on that at some point. In any case, it is currently a magic number and giving it a name helps a lot. Or maybe you move to Mars at some point and need to use a different radius :)

import pandas as pd
import numpy as np

EARTH_RADIUS = 6371  # km

def total_dist(group):
    lat = np.radians(group.Lat)
    lon = np.radians(group.Lon)
    endLon = lon.shift(-1)
    endLat = lat.shift(-1)
    x = (endLon - lon) * np.cos(0.5 * (endLat + lat))
    y = endLat - lat
    D = EARTH_RADIUS * np.sqrt(x**2 + y**2)
    return D.sum()

Note that the conversion to radians is not as nice anymore, but we gain the ability to process a whole trip at a time!

Now we define a helper function to output not only the dist, but also the number of waypoints (by using len):

def trip_statistics(trip):
    return pd.Series({"Distance": total_dist(trip),
                      "No. of Waypoints": len(trip)})

Now the only thing left to do is apply this function to all groups and reset the index to get TripID back as a column and not just as an index:

waypoint = pd.read_csv('TripRecordsReportWaypoints.csv', sep=',',
                       header=None, usecols=[0, 3, 4],
                       names=['TripID', 'Lat', 'Lon'])
output = waypoint.groupby("TripID").apply(trip_statistics)
output.reset_index().to_csv('TripDistances.csv', sep=',', index=False)

Note that I added index=False to avoid writing the row index to the output file.


Qt Custom Scatterplot I have  created a custom scatterplot in Qt and am wondering if there is a more efficient way to do it. Here is a sketch of the problem:

The layout of the plotting area consists of a rectangular structure starting at [0,0] in the top left corner and expands by 250 pixels to the bottom right corner [250,250]. So the y-coordinate grows downwards and the x-coordinate to the right.

The task consists of mapping two vectors (A[i],B[i]) holding numerical data onto this rectangular structure such that the lowest x,y coordinates are in the bottom left corner and the highest x,y coordinates are in the top right corner.

To solve this problem I choose the following approach (pseudo code):

calculate the min and max for (A and B);
calculate the spread (max - min) for (A and B);
transform A and B by:

 foreach (i in A) {
     (i - min(A) / spread(a))* 250
 }
 foreach(j in B) {
     (250 - ((j - min(B) / spread(B))*250
 }

and the implementation:

.h file

#include <QDialog>
#include <QtGui>
#include <QtCore>
#include <QGraphicsScene>
#include <cstdlib>
#include <cmath>
#include <QVector>
#include <QRectF>

namespace Ui {
class paintEr;
}

class paintEr : public QDialog
{
    Q_OBJECT

public:
    explicit paintEr(QWidget *parent = 0);
    ~paintEr();
private slots:
    void addPoint();

private:
    Ui::paintEr *ui;
    QGraphicsScene *scatter;

protected:
    void paintEvent(QPaintEvent *event);

};

.cpp

#include "painter.h"
#include "ui_painter.h"

paintEr::paintEr(QWidget *parent) :
    QDialog(parent),
    ui(new Ui::paintEr)
{
    ui->setupUi(this);
}

void paintEr::paintEvent(QPaintEvent *event){

    scatter = new QGraphicsScene(this);
    ui->graphicsView->setSceneRect(0,0,250,250);
    ui->graphicsView->setScene(scatter);

}
void paintEr::addPoint(){
    QPen greenPen(Qt::green);
    QBrush greenBrush(Qt::green);

    QVector<double> a,b,c,d;
    a << 2 << 5 << 7;
    b << 22 << 34 << 75;
    double min_a = *std::min_element(a.begin(), a.end());
    double max_a = *std::max_element(a.begin(), a.end());
    double min_b = *std::min_element(b.begin(), b.end());
    double max_b = *std::max_element(b.begin(), b.end());
    double spread_a = max_a - min_a;
    double spread_b = max_b - min_b;

    for(int i = 0; i < a.length(); ++i){
        c.push_back(((a[i]-min_a)/spread_a)*250);
        d.push_back(250-((b[i]-min_b)/spread_b)*250);
        scatter->addEllipse(c[i], d[i], 5, 5, greenPen, greenBrush);
    }
}
paintEr::~paintEr()
{
    delete ui;
}

main.cpp

#include "painter.h"
#include <QApplication>

int main(int argc, char *argv[])
{
    QApplication a(argc, argv);
    paintEr w;
    w.show();

    return a.exec();
}

You should separate the actual data from the logic. Make your function capable of working on any QVectorq of QPairs (or pair of QVectors but that's not as normalized).

Variable names a,b,c,d don't make sense. Especially since a and b really ought to be input parameters.

You should be able to get both the min and max with one pass over the list. You'll probably have to write a custom mapreduce-style function.

For the circles in the QGraphicsScene: Are they supposed to have their center at the point in question, or the topleft of their bounding rect?


Apply a function to each element of a tuple ("map" a tuple) C++ doesn't (yet) have a 'map' higher-order function, certainly not one which applies to a compile-time sequence of values like a tuple. We have std::transform, and there's an equivalent of it for tuples listed on SO, but that's iterator-based, and anyway - I don't want to use any reference to the output. I want to pass a tuple and a function / variadic lambda, and get another tuple of the same length with the function applied to each element.

I'm also sure this has been implemented multiple times, but since I want to build up some "tuple confidence", I've done it myself. Here's what I got:

namespace detail {

template <class F, typename Tuple, size_t... Is>
auto transform_each_impl(Tuple t, F f, std::index_sequence<Is...>) {
    return std::make_tuple(
        f(std::get<Is>(t) )...
    );
}

} // namespace detail

template <class F, typename... Args>
auto transform_each(const std::tuple<Args...>& t, F f) {
    return detail::transform_each_impl(
        t, f, std::make_index_sequence<sizeof...(Args)>{});
}

And this works in the cases I've tried - I haven't tested extensively.

I'd like general feedback/suggestions, but also:

When is the function resolved or instantiated in the case of different-type arguments? In what cases will it be the same (instantiated) function with forced conversion, as opposed to different instantiations for the different types? Am I really getting the latter at all?
Is there any std::forward'ing that I need to do?
Can I avoid a helper function somehow?
What do you think about the name?

transform, or map ?

I wouldn't name this function transform, because it's very different from that stl algorithm. std::transform can be used to modify a range in place, and can be called with either unary or binary operations. What you implement is generally called map and has its roots in functional languages; so I would call it tuple_map, for instance. Then the convention is to put the function first in the argument list, and the sequence second.

Or, you could try to implement transform, but then it gets tricky, because there are three "semantic inputs" to be considered: the tuple, its elements and the function signature. I haven't had the time to consider it thoroughly, but I believe that using decltype(fn(std::get<0>(tuple))) as a semantic clue is a good approximation. If it's void, you know that the semantic is at the tuple level; if it's an lvalue reference, you use std::tie to return a tuple of references; if it's an rvalue reference, std::forward_as_tuple is the most adapted; and if it's a value, you can return a new tuple.

A sketch for transform

For instance, with C++ 17 enabled:

#include <tuple>
#include <type_traits>

template <typename Fn, typename Argument, std::size_t... Ns>
auto tuple_transform_impl(Fn&& fn, Argument&& argument, std::index_sequence<Ns...>) {
    if constexpr (sizeof...(Ns) == 0) return std::tuple<>(); // empty tuple
    else if constexpr (std::is_same_v<decltype(fn(std::get<0>(argument))), void>) {
        (fn(std::get<Ns>(argument)),...); // no return value expected
        return;
    }
    // then dispatch lvalue, rvalue ref, temporary
    else if constexpr (std::is_lvalue_reference_v<decltype(fn(std::get<0>(argument)))>) {
        return std::tie(fn(std::get<Ns>(argument))...);
    }
    else if constexpr (std::is_rvalue_reference_v<decltype(fn(std::get<0>(argument)))>) {
        return std::forward_as_tuple(fn(std::get<Ns>(argument))...);
    }
    else {
        return std::tuple(fn(std::get<Ns>(argument))...);
    }
}

template <typename Fn, typename... Ts>
auto tuple_transform(Fn&& fn, const std::tuple<Ts...>& tuple) {
    return tuple_transform_impl(std::forward<Fn>(fn), tuple,
                          std::make_index_sequence<sizeof...(Ts)>());
}

Helper function

I can't see how you would avoid to use a helper function (besides using a tuple_transform function, which you're currently implementing).

You'll see I use a forwarding reference for the function argument, since it can be expensive to copy.

I keep the const lvalue reference for the tuple argument, because it's more simple (a forwarding reference wouldn't allow to constrain the type, and sizeof... wouldn't be possible either), but mainly because I can't see a case where the tuple would be consumed differently as a rvalue reference.