C function to find the less program I've written a C function to find the program less that the operating system should provide. If lessis not provided, some other pager variable should be used. I compile it with gcc -ansi -pedantic -Wall -O3.

int findless(char *less_token, int ret) {
    int breakloop = 0;
    char *new_str;
    while (less_token != NULL) {
        if ((new_str = malloc(strlen(less_token) + strlen("/less") + 1)) != NULL) {
            new_str[0] = '\0';
            strcat(new_str, less_token);
            strcat(new_str, "/less");
            if (file_exist(new_str)) {
                /* Found less */
                ret = 0;
                breakloop = 1;
            }
            free(new_str);
            if (breakloop) {
                break;
            }
        } else {
            printf("malloc failed!\n");
        }
        less_token = strtok(NULL, ":");
    }
    return ret;
}

Can you please review it? I put in in a separate utilities util.c file and I'm including it with a util.h interface.

Clearer Naming Needed

In general, I think your variable naming could be better. I would rename less_token to be called path or something to make it clearer what you're doing here. new_str should be called current_path or curr_path. Moreover, I'd make the function name something more like find_less_program.

C Style

If you're going to be changing the ret parameter, it's idiomatic in C to pass in a pointer to ret and modify that. Either that or remove ret as a parameter and just return an int as an error code. Moreover, whenever I see a break in a while loop, I get a small frown on my face as that's usually a sign of bad control flow. See below for (IMO) a better control flow.

Moreover, it's been over a decade since C99 came out and introduced us to the bool type definitions. Prefer using them over the standard int way of signaling true or false. You'll need to include stdbool.h to use bool.

I would also create a small constant buffer containing the name of the program we're looking for (in this case "/less".) This prevents you from having to write "/less" all over the place and instead just use a variable name. There's no need to assign the first character of the allocated buffer to NULL in this case. Just call strcpy to copy the string name into the buffer instead of relying on strcat. This also makes your intent clearer as you're first copying an existing string into a buffer and then appending to it.

Here's a possible implementation taking together all of the above suggestions:

bool find_less_program(char *path) {

    bool found = false;
    char *curr_path;
    const char program[] = "/less";

    while (path && !found) {
        if ((curr_path = malloc(strlen(path) + sizeof(program))) != NULL) {

            strcpy(curr_path, path);
            strcat(curr_path, program);
            if (file_exist(curr_path)) {
                found = true; // we found the program
            }
            free(curr_path);
            path = strtok(NULL, ":");
        } else {
            fprintf(stderr, "malloc failed!\n");
            return false;
        }
    }
    return found;
}


Printing lyrics of 99 Bottles of Beer I wrote a C++ program for 99 Bottles of Beer, but it's kind of messy. Is there any way I can make it short and cleaner and more easier to read?

/*
 * Write a program that prints out the entire lyrics to a full rendition of "99 bottles of beer"
 */

#include <iostream>

using namespace std;

int main (void)
{
    int bottle = 99;
    while (bottle > 0)
    {
        cout << bottle << " bottles of beer on the wall, "
             << bottle << " bottles of beer." << endl;

        bottle--;

        cout << "Take one down and pass it around, " << bottle << " "
             << "bottles of beer on the wall.\n" << endl;

        if (bottle == 1)
        {
            break;
        }

    }

    cout << "1 bottle of beer on the wall, 1 bottle of beer." << endl
         << "Take one down and pass it around, no more bottles of beer on the wall.\n" << endl
         << "No more bottles of beer on the wall, no more bottles of beer." << endl
         << "Go to the store and buy some more, 99 bottles of beer on the wall.";

    return 0;
}

A few short comments:

Don't use namespace std:

It's bad for a few reasons, but in short: You can have big name conflicts if you are using namespace std;. For more information: Why is using namespaec std considered bad practice?

Break conditions:

well lookie here:

while (bottle > 0)
{
    //print the lyrics
    if (bottle == 1)
    {
         break;
    }
}

This could be much simpler:

while (bottle > 1)
{
     //print the lyrics
}

Naming:

bottle is a crap name for a counter. Make it bottleCount, leftBottles or about anything else, but don't name it bottle. Because it isn't a bottle...


Make a popup window that will stay open for 3 seconds I am doing an assignment and I have a task where I need to make a simulation where under certain conditions it opens a popup window and then closes it after 3 seconds. This is how I did it:

if(condition) {
    ExecutorService executor = Executors.newSingleThreadExecutor();
    executor.submit(() -> {
        Platform.runLater( () -> {
            try {
                Monument atr = ((Monument)matrica[red][kolona]);
                FXMLLoader loader = new FXMLLoader(getClass().getResource("PopUp.fxml"));
                Scene scene = new Scene(loader.load());
                loader.<PopUpController>getController().setImage(atr.getImage());
                Stage stage = new Stage();
                stage.setScene(scene);
                stage.sizeToScene();
                stage.setResizable(false);
                stage.setTitle("Image");
                stage.show();
                sleep(3000);
                ((Stage)(stage.getScene().getWindow())).close();
            } catch (IOException | InterruptedException e) {
                e.printStackTrace();
            }
        });
    });
    executor.shutdown();
}

I use Platform.runLater because I am accessing the UI from a different thread. My question is, is there any better way of doing this?

Alternative code:

ScheduledExecutorService executor =  Executors.newSingleThreadScheduledExecutor();
// assuming the stage-setup is perfomed already:
executor.submit(() -> Platform.runLater(stage::show));
executor.schedule(
    () -> Platform.runLater(((Stage)stage.getScene().getWindow()).close())
    , 3
    , TimeUnit.SECONDS);

This approach has multiple advantages:

The (costly) stage-setup is moved away from the UI Thread
Sleeping on the UI Thread doesn't happen, freeing it to do actual work
ScheduledExecutorService is more exact with it's scheduling than a sleep-call. It also doesn't involve having to handle InterruptedException or spurious wakeup

But it also has one glaring disadvantage:

You can't directly shutdown the ExecutorService, because that would cancel the scheduled Runnable.

This shouldn't be much of a problem though, since you should not be creating an ExecutorService in method scope in the first place.
Instead you should move multithreading facilities into the class scope (or even into static scope), so as to enable easy cleanup when the application is terminated or the instance reaches the end of it's lifetime.


Displaying a city name with concatenations I am learning MVC and Android. I have create a sample program which is supposed to implement the MVC philosophy. I plan to build my codes on top of this.

I wish to know whether I have got the concept more or less, or if there are any fundamental issues in my understanding of the MVC.

Summary: This application will get a city name and display it with a few concatenations (country name etc).

Model -> Model.java

/**
 *
 */
package abc.samples.mvc;
/**
 * @author Rahul
 *
 */
public class Model {

    String mData;
    String mInput;
    String mOutput;

    public void set(String temp) {
        mInput = temp;
        mData = mInput + ", India.";
    }

    public String get() {
        mOutput = "You are from "+mData;
        return mOutput;
    }

}

View -> view.xml

<RelativeLayout xmlns:android="http://schemas.android.com/apk/res/android"
    xmlns:tools="http://schemas.android.com/tools"
    android:layout_width="match_parent"
    android:layout_height="match_parent"
    tools:context="${relativePackage}.${activityClass}" >

    <Button
        android:id="@+id/vControl"
        android:layout_width="fill_parent"
        android:layout_height="wrap_content"
        android:layout_alignParentLeft="true"
        android:layout_alignParentTop="true"
        android:text="Control" />

    <EditText
        android:id="@+id/vInput"
        android:layout_width="fill_parent"
        android:layout_height="wrap_content"
        android:layout_alignParentLeft="true"
        android:layout_below="@+id/vControl"
        android:ems="10" >
        <requestFocus />
    </EditText>

    <TextView
        android:id="@+id/vOutput"
        android:layout_width="fill_parent"
        android:layout_height="wrap_content"
        android:layout_alignParentLeft="true"
        android:layout_below="@+id/vInput"
        android:text="Large Text"
        android:textAppearance="?android:attr/textAppearanceLarge" />
</RelativeLayout>

Controller -> Controller.java

package abc.samples.mvc;

import android.app.Activity;

import android.os.Bundle;
import android.view.Menu;
import android.view.MenuItem;
import android.view.View;
import android.widget.Button;
import android.widget.EditText;
import android.widget.TextView;

public class Controller extends Activity {

    EditText cInput;
    TextView cOutput;
    Button cControl;
    Model cData;

    @Override
    protected void onCreate(Bundle savedInstanceState) {
        super.onCreate(savedInstanceState);
        setContentView(R.layout.view);

        cInput = (EditText) findViewById(R.id.vInput);
        cOutput = (TextView) findViewById(R.id.vOutput);
        cControl = (Button) findViewById(R.id.vControl);

        cControl.setOnClickListener(new View.OnClickListener() {

            @Override
            public void onClick(View v) {

                cData = new Model();

                cData.set(cInput.getText().toString());
                cOutput.setText(cData.get());
            }
        });
    }
}

A model should not have presentation logic, and be mostly about containing data. The text "You are from ..." Doesn't belong in a model. As it is, your model includes logic that concerns representation, and so you're violating MVC. You could use another class to be in charge of formatting a Model instance. In other words, it would be best if your Model just contained fields and did nothing else. Furthermore, all fields can be private final, for example:

public class City {

    private final String name;

    public City(String name) {
        this.name = name;
    }

    public String getName() {
        return name;
    }
}

I removed the other fields which seemed quite pointless for a model.
And I renamed Model to City, because "Model" is way too abstract and not useful.

In the activity it's strange to create fields when you only use them in onCreate.
It's best to limit variables to the smallest scope necessary.
As such, all these fields should be local variables in onCreate, like this:

EditText input = (EditText) findViewById(R.id.vInput);
TextView output = (TextView) findViewById(R.id.vOutput);
Button control = (Button) findViewById(R.id.vControl);

These names are terrible by the way.
(Now I realize your question may have been rather hypothetical, not a real program.)
Give them better names.


Removing duplicate files in a given directory The following code is a python script that removes duplicate files in a given directory. At first I considered the most basic thing: relying on identical names, but you might have 2 different files with the same name in 2 different directories. So I decided to rely on md5checksum, since any 2 files that yield the same md5checksum almost invariably have the same content.

#A simple Python script to remove duplicate files...Coded by MCoury AKA  python-scripter
import hashlib
import os

#define a function to calculate md5checksum for a given file:
def md5(f):
    """takes one file f as an argument and generates an md5checksum for that file"""
    return hashlib.md5(open(f,'rb').read()).hexdigest()

#define our main function:
def rm_dup(path):
    """relies on the md5 function above to remove duplicate files"""
    if not os.path.isdir(path):#make sure the given directory exists
        print('specified directory does not exist!')
    else:
        md5_dict={}
        for root, dirs, files in os.walk(path):#the os.walk function allows checking subdirectories too...
            for f in files:
                if not md5(os.path.join(root,f)) in md5_dict:
                    md5_dict.update({md5(os.path.join(root,f)):[os.path.join(root,f)]})
                else:
                    md5_dict[md5(os.path.join(root,f))].append(os.path.join(root,f))
        for key in md5_dict:
            while len(md5_dict[key])>1:
                for item in md5_dict[key]:
                    os.remove(item)
                    md5_dict[key].remove(item)
        print('Done!')

 if __name__=='__main__':
    print('=======A simple Python script to remove duplicate files===========')
    print()
    print('============Coded by MCoury AKA python-scripter===================')
    print()
    print('===========The script counts on the fact the fact=================')
    print('=========that if 2 files have the same md5checksum================')
    print('==========they most likely have the same content==================')
    print()
    path=input(r'Please provide the target path\directory... for example: c: or c:\directory...')
    print()
    rm_dup(path)

Ever since discovering the Zen of python I became obsessed with using the least possible lines of code. Plus I have another (and perhaps more serious) concern; calculating md5checksum for large files takes precious memory real-estate right? Could that limit the functionality of the script? Also, what do you think of the implementation?

To get md5 of large files you can use something like this:

def md5(fname):
    hash_md5 = hashlib.md5()
    with open(fname, 'rb') as f:
        for chunk in iter(lambda: f.read(4096), b""):
            hash_md5.update(chunk)
    return hash_md5.hexdigest()

Now here

for f in files:
    if not md5(os.path.join(root,f)) in md5_dict:
        md5_dict.update({md5(os.path.join(root,f)):[os.path.join(root,f)]})
    else:
        md5_dict[md5(os.path.join(root,f))].append(os.path.join(root,f))

I see two things:

You don't need to check if key in your dict, you can use defaultdict instead
md5 is calculated twice per each file, first time to check if it's in dict, second time to actually add it to dict.

Here:

while len(md5_dict[key])>1:
    for item in md5_dict[key]:
        os.remove(item)
        md5_dict[key].remove(item)

You can just use list.pop()

So in the end your code should look like this:

import hashlib
import os
from collections import defaultdict

def md5(fname):
    hash_md5 = hashlib.md5()
    with open(fname, 'rb') as f:
        for chunk in iter(lambda: f.read(4096), b""):
            hash_md5.update(chunk)
    return hash_md5.hexdigest()

def rm_dup(path):
    """relies on the md5 function above to remove duplicate files"""
    if not os.path.isdir(path):  # make sure the given directory exists
        print('specified directory does not exist!')
        return

    md5_dict = defaultdict(list)
    for root, dirs, files in os.walk(path):  # the os.walk function allows checking subdirectories too...
        for filename in files:
            filepath = os.path.join(root, filename)
            file_md5 = md5(filename)
            md5_dict[file_md5].append(filepath)
    for key in md5_dict:
        file_list = md5_dict[key]
        while len(file_list) > 1:
            item = file_list.pop()
            os.remove(item)
    print('Done!')

if __name__ == '__main__':
    print('=======A simple Python script to remove duplicate files===========')
    print()
    print('============Coded by codereview.stackexchange.com AKA python-scripter===================')
    print()
    print('===========The script counts on the fact the fact=================')
    print('=========that if 2 files have the same md5checksum================')
    print('==========they most likely have the same content==================')
    print()
    path = input(r'Please provide the target path\directory... for example: c: or c:\directory...')
    print()
    rm_dup(path)

however there is one more thing, while in general it's right to first list all the files and then delete them one by one, because maybe in future you will add some option for user to select files he want to delete. In this current case you can just use set to keep md5 checksums in it, and delete files as soon as you fight one with the same chucksum.

P.S. two files with the same checksum are not 100% guaranteed to be the same. Think about adding some additional check.


Shopping cart with discounts I want to create a shopping cart project with discounts for products. For instance, if you buy 2 books, you can get 10% off Parker pens. I created a shopping cart example, but without exception handling in this version.

Are my classes Product, Basket, Offers and Discounts properly structured, or what changes do I need to make to make it right? To calculate discounts, should I iterate my basket or iterate the list of offers? The offers are a little tricky as mentioned at the top.

Product

public class Product {

    private String name;
    private int price;

...with get methods + hashcode and equals.

Basket

public class Basket {

    Map<Product, Integer> items;

    public Basket() {
        items = new HashMap<>();
    }

    public Map<Product, Integer> getItems() {
        return items;
    }

    public void addProduct(Product product){
        if(items.containsKey(product)){
            items.compute(product, (p,q ) -> Integer.valueOf(q+1));
        }else{
            items.put(product, 1);
        }
    }

    public void removeProduct(Product product){
        if(items.get(product).intValue() > 1){
            items.compute(product, (p, q) -> Integer.valueOf(q-1));
        }else{
            items.remove(product);
        }
    }
}

Discount

public class Discount {

    private Product product;
    private int discountPercent ;

    public Product getProduct() {
        return product;
    }

    public int getDiscountPercent() {
        return discountPercent;
    }

    public Discount(Product product, int discountPercent) {
        super();
        this.product = product;
        this.discountPercent = discountPercent;
    }

    @Override
    public String toString() {
        return "Discount [product=" + product + ", discountPercent=" + discountPercent + "]";
    }
}

Offer

public class Offer {

    private Product product;
    private int quantity;
    private Discount discount;

    public Product getProduct() {
        return product;
    }
    public void setProduct(Product product) {
        this.product = product;
    }

    public int getQuantity() {
        return quantity;
    }
    public void setQuantity(int quantity) {
        this.quantity = quantity;
    }
    public Discount getDiscount() {
        return discount;
    }
    public void setDiscount(Discount discount) {
        this.discount = discount;
    }

    public Offer(Product product, int quantity, Discount discount) {
        super();
        this.product = product;
        this.quantity = quantity;
        this.discount = discount;
    }
    @Override
    public String toString() {
        return "Offer [product=" + product + ", quantity=" + quantity + ", discount=" + discount + "]";
    }

}

You should have a class PriceComputer that computes prices and their discounts. This should be the central class of the whole design.

Then, make separate data types for the input and output of that class. That way the compiler prevents you from mixing these during the calculations (which will get quite complicated once you add more rules).

The input just needs to be the basket that you already have. You should change the HashMap to a LinkedHashMap, to keep the products in the same order they were inserted.

Each output element should have the properties product, quantity, resultingPrice, discount, discountPercent (only if applicable). All its fields should be final, since once computed, there is no reason to change them.

The general structure of your classes looks good. You might have a look at Project Lombok (which will save you from writing boring code), and you should make clear what the class Offer stands for, since it wasn't clear to me from the name alone.

Also, given an Offer, there are two ways of reaching a Product (offer.product and offer.discount.product). Are these products always the same? If so, one of them must be removed from the code.

Product.equals should not depend on the name of the product. Instead, only the SKU should be compared. Or, even simpler, make sure that you only have a single Product object per product, and don't override hashCode/equals at all.


PHP Autoload Class From Namespace I wrote this function to be passed as a param in spl_autoload_register(). It autoloads PHP classes from their respective directories using their namespace, named the same as path. I am not sure if there would be any easier way but this is quite simple and does what it needs to.

spl_autoload_register(function($class) {
    $ns = explode('\\', strtolower($class));
    $class = array_pop($ns);
    $ns = implode('\\', $ns) . '\\';
    require_once __DIR__ . '\\' .  $ns . 'class.' . $class . '.php';
});

Thoughts on this? Is there an easier way?

If your namespaces match your directory structure you would not need to fetch the class name. You could just append it to the base path and append the .php file extension.

I would argue the class. file prefix is unnecessary. I think of such prefixes from a time when namespaces was not available. If you are concerned with the ease of locating correct files I can recommend the following method. I have used for several projects and are quite happy with it.

classes/
    interfaces/
        interfaceName.php
    abstracts/
    traits/
    class.php
    anotherclass.php

I hope you get the idea. If you follow this you can use this.

spl_autoload_register(function($class) {

    $filename = __DIR__ . '\\' . $class . '.php';

    if(!file_exists($filename)) {
        return false; // End autoloader function and skip to the next if available.
    }

    include $filename;
    return true; // End autoloader successfully.

});

This is pretty much it. You should of course check that __DIR__ equals your base path. If you store your base path inside a variable and you are using a closure as callback, you can inject the path into the closure using this:

$path = 'This is your base path.';

spl_autoload_register(function($class) use ($path) {...});

If your file system uses forward-slashes (for some reason), you could wrap the $class variable in a str_replace() function call.

$class = str_replace('\\', '/', $class);

Happy coding!


Fastest way to remove/hide a lot of elements from a list I have a dropdown that contains around 100,000 rows which make up a list.

<input id="search" type="text" />

item 1

item 2

        ...

item 100,000

I have a text box which acts as a search, so as you type it matches the input to items in the list, removing what does not match. This is the class I wrote to perform the removing of list elements.

See the fiddle (list has about 2000 items)

// requires jQuery
var Search = (function(){

    var cls = function (name) {
        var self = this;

        self.elem = $('#' + name);
        self.list = $('#' + name).next('ul').children();
        self.elem.bind('keyup', function () { self.change(); });
    };

    cls.prototype = {
        change: function () {
            var self = this;
            // gets the closest ul list
            var typed = self.elem.val();

            // only do something if there is something typed
            if (typed !== '') {
                // remove irrelevent items from the list
                self.list.each(function () {
                    var item = $(this).html();
                    if (item.indexOf(typed) === -1)  {
                        $(this).addClass('zero');
                        // tried using a class with visibility hidden
                    } else {
                        $(this).removeClass('zero');
                    }
                });
            } else {
                // check what list items are 'hidden' and unhide them
                self.list.each(function () {
                    if ($(this).hasClass('zero')) {
                        $(this).removeClass('zero');
                    }
                });
            }
        }
    };
    return cls;
}());

I am just adding a class which adds a height: 0, and no margin, padding, etc, but I have also tried using visibility: hidden. I have also tried using the detach method in jQuery but this is about the same in terms of speed.

Are there any JavaScript experts who can see any problems with the code, or offer some optimization techniques?

A couple notes:

display: none does everything you're trying to do with your existing CSS class; it hides and completely removes the element from the layout. So instead of adding the zero class you would set display to none, and instead of removing it you would set display to block.
As far as optimization goes, one thing you could do is iterate the list items and store all the strings in an array as part of the initialization for your function. Then when you're iterating self.list, fetch the string from your cached array with the current index instead of calling $(this).html(). That would most likely be faster because it eliminates a DOM lookup.


Changing html element with click event: how to restore default state of element using jQuery if/else statement I have a click event that calls a function (below, txtResize) which makes some CSS changes to html text. (Acts on Text of 3 unique elements).

Q: What is the best way to refactor this code to limit excessive if/else usage, AND restore the original state of ALL text elements once the user clicks away from this section?

Here is the partially working, ugly function:

 function txtResize() {

        var clicked = this.id;

        if (clicked == "Text1")
        {
            $("#" + clicked).css("opacity", "1");
            $("#Text2, #Text3").css("opacity", "0.2");
        }
        else if (clicked == "Text2")
        {
            $("#" + clicked).css("opacity", "1");
            $("#Text1, #Text3").css("opacity", "0.2");
        }
        else if (clicked == "Text3")
        {
            $("#" + clicked).css("opacity", "1");
            $("#Text1, #Text2").css("opacity", "0.2");
        }
         // code above is ok, below does not work

        else if ( !(clicked == "Text1") && !(clicked == "Text2") && !(clicked == "Text3") )
        {
            $("#Text1, #Text2, #Text3").css("opacity", "1");  // return to default state
        }

 };

This is actually a pretty classic problem and I've addressed it in my code on numerous occasions. One of the simplest ways to do what you're doing is to just reset the state for all the elements and then correct it.

function txtResize() {
    var clicked = this.id;

    if ( clicked === "Text1" || clicked === "Text2" || clicked === "Text3" ) { //one of our text's were clicked
        $("#Text1, #Text2, #Text3").css("opacity", "0.2"); //reset them all to .2 opacity
        $(this).css("opacity", "1");//set this elements opacity to full
    } else { //its clearly not Text1/2/3
        $("#Text1, #Text2, #Text3").css("opacity", "1");  // return to default state
    }
 };

One nit-picky point is that we tend to prefer lower case in attributes for html. I would recommend text1, text2... or even better would be to give an id that better described what the element is on the page.


Logging inside a Linq fluent chain I've posted this to Stack Overflow, but someone voted to close as they felt it was opinion based, so I'm bringing it over to Code Review.

I have some Linq fluent chains to shape an original IEnumerable of objects into the proper form.

During this process, I'd like to capture and log the reason why some objects are "rejected" in a Where clause.

I was thinking about something similar to this (reduced use-case, just for the sake of demonstration):

var bar = SomeEnumerable
    .SelectMany(...)
    .Where(i =>
    {
        if(...)
        {
            return true;
        }
        else
        {
            //Log "failure" to in-memory list
            return false;
        }
    })
    .ToDictionary(...);

I understand that this introduces some side-effect, though minimal, but if at all possible, I would like to eliminate it.

Does this match some "pattern" that can be expressed in the context of Linq chains?

Thanks

Yeah, it seems to cloud up your code a bit. You could write your own LINQ-style extension method similar to the ones below. It uses a couple of C#7 features (tuple types and local methods). The first just recreates the collection with the results associated with each item (to avoid any side-effects of possibly calling the predicate multiple times) and the second will iterate and log/filter as your lambda does.

namespace System.Linq
{
    using System.Collections.Generic;

    public static class Enumerable
    {
        public static IEnumerable<(TSource, bool)> WhereResult<TSource>(
            this IEnumerable<TSource> source,
            Func<TSource, bool> predicate)
        {
            if (source == null)
            {
                throw new ArgumentNullException(nameof(source));
            }

            if (predicate == null)
            {
                throw new ArgumentNullException(nameof(predicate));
            }

            return _();

            IEnumerable<(TSource, bool)> _()
            {
                foreach (TSource current in source)
                {
                    yield return (current, predicate(current));
                }
            }
        }

        public static IEnumerable<(TSource, bool)> LogWhere<TSource>(
            this IEnumerable<(TSource, bool)> source)
        {
            if (source == null)
            {
                throw new ArgumentNullException(nameof(source));
            }

            return _();

            IEnumerable<(TSource, bool)> _()
            {
                foreach ((TSource, bool) current in source)
                {
                    if (current.Item2)
                    {
                        yield return current;
                    }
                    else
                    {
                        Log(...);
                    }
                }
            }
        }
    }
}

This is just a first thought off the top of my head. Could be improved easily with some AOP instead of the explicit logging call, for instance.


Dynamic connections classes I have an abstract Connection class which looks like this:

abstract class Connection
{

    /**
     * @var mixed $instance Represents the instance of the connection
     */
    protected $instance = null;

    /**
     * @return mixed
     */
    public function getInstance() {
        if (!$this->isInitialized()) {
            echo "Cant get instance yet because the connection isn't open";
        }
        return $this->instance;
    }

    /**
     * @return bool
     */
    public function isInitialized() {
        return $this->instance !== null;
    }

    /**
     * Initializes the connection
     */
    public abstract function initialize();

    /**
     * Closes the connection
     */
    public abstract function terminate();
}

It requires an intialize() and terminate()in its childs or subchilds.

The initialize method opens the connection
The terminate method closes the connection

The $instance determinates the object which has to be ran to start the Connection. For example, for a MySQL connection, this could be the mysqli object with its parameters.

A child class of the abstract Connection class looks like this (in this case im only showing a Connection to a database).

This class cannot be instantiated because its also abstract.

abstract class DatabaseConnection extends Connection
{

    /**
     * @var string
     */
    protected $host;

    /**
     * @var
     */
    protected $database;

    /**
     * @var
     */
    protected $username;

    /**
     * @var
     */
    protected $password;

    /**
     * @var
     */
    protected $credentials = [];

    /**
     * @param float  $host
     * @param string $database
     * @param string $username
     * @param string $password
     * @param array  $credentials
     */
    public function __construct($host, $database, $username, $password, $credentials = []) {
        $this->host        = $host;
        $this->database    = $database;
        $this->username    = $username;
        $this->password    = $password;
        $this->credentials = $credentials;
    }

    public final function initialize() {
        if ($this->isInitialized()) {
            throw new Exception("Database connection isnt open yet.");
        }
        $this->instance = $this->getConnectionObject();
    }

    public final function terminate() {
        if ($this->isInitialized()) {
            throw new Exception("Database is closed");
        }
        $this->instance = null;
    }

    protected abstract function getConnectionObject();
}

This mostly speaks for itself, you initialize the credentials in the __construct method.

The getConnectionObject() determinates the object wich has to be instantiated to initialize a Database Connection.

A child class of the DatabaseConnection looks like this:

class MySQLConnection extends DatabaseConnection
{

    /**
     * @return mysqli
     */
    public function getConnectionObject() {
        return new mysqli($this->host, $this->username, $this->password, $this->database);
    }
}

This can be instantiated, to use this, i need to use the following code:

$connection = new MySQLConnection('127.0.0.1', 'db', 'username' 'password');

To open the connection you use the following method:

$connection->initialize();

To close the connection you use the following method:

$connection->terminate();

To give this DatabaseConnection functionality I call  the getInstance() method on the $connection object i've just made.

$connection->getInstance();

In this case this returns the mysqli object, from here i can call all needed functions.

A few notes:

This is for practice, i was trying to create a connection class wich had support for multiple types of connections.
What I originally wanted was to seperate each database function in a seperate class, but i did not know how to do this.

Your code has a major issue. It's breaking a basic rule of inheritance. The "base" class (Connection) does not specify a constructor. That is to say, any constructors that a child class may contain should take either no, or only optional arguments. That's what the Liskov principle states anyway.

Your DatabaseConnection extends Connection (BTW: an abstract extending an abstract... that smells, if you ask me), but it defines a constructor with non-optional arguments. This contradicts, and breaks the inherited contract defined by Connection.
PHP historically, has been very forgiving when it came to conflicting signatures, especially in the constructors, but as the manual says:

  When inheriting from an abstract class, all methods marked abstract in the parent's class declaration must be defined by the child; additionally, these methods must be defined with the same (or a less restricted) visibility. For example, if the abstract method is defined as protected, the function implementation must be defined as either protected or public, but not private. Furthermore the signatures of the methods must match, i.e. the type hints and the number of required arguments must be the same. For example, if the child class defines an optional argument, where the abstract method's signature does not, there is no conflict in the signature. This also applies to constructors as of PHP 5.4. Before 5.4 constructor signatures could differ.

That ship has sailed. Now in case you're wondering why this is such a big deal, look at the code below:

function checkConnection(Connection $con)
{//type-hint ensures instance of Connection is passed
    //for some reason, create a copy:
    $class = get_class($con);//get actual class
    $newInstance = new $class();//<-- No arguments!!
}

Now this function has to be allowed to assume that no arguments need to be passed to a constructor, if there even is a constructor. Simply because this function is hinting at Connection  instances. Because of the type-hint, the function can only assume the methods/properties that are declared by the Connection class are available, and that the instance that it is being passed is an instance (an extension) of this contract. Connection doesn't declare a constructor, so its children can only create a public, optional constructor.

Fix this issue ASAP.
Apart from that, I'd really like to see where you're going with this, because this abstract class Connection + abstract class DatabaseConnection seems like an awful lot of abstraction for what will, essentially, be a service locator, or DI container.

A DI Container, or SL shouldn't even care what kind of dependencies/services/objects it is injecting and instantiating. So there's even less need of these abstractions you've written from that perspective.

I'm also having doubts concerning the terminate method. If a piece of code uses a mysqli connection, and needs the connection to be open for some reason (ie: a prepared statement that is pending, a huge result set is being fetched in chunks,...), having the connections managed at a central level might cause conflicts. I'd say that the only place where the container can close connections is when it is being destroyed: if the container is going out of scope, all that it has spawned is probably gone, too.

Update - A crude example of a container:

Say you have config files, that define DB connections, which could look like this;

connections:
    db:
        readConnection:
            class: mysqli
            host: 127.0.0.1
            database: db
            username: user
            password: pass
        writeConnection:
            class: PDO
            host: 8.8.8.8
            database: db
            username: user
            password: pass

Now, a container would parse this config, and allow you to write something like this:

$mysqli = $container->get('connections.db.readConnection');

This get method would then parse the slug, and interpret the configuration it finds, to end up returning:

$path = explode('.', $slug);
$section = $this->getFullConfig();
foreach ($path as $key)
    $section = $section->get($key);
$class = $section->get('class');
return new $class(
    $section->get('host'),
    $section->get('database'),
    $section->get('username'),
    $section->get('password')
);

Note that this is a grossly oversimplified, but you get the idea.


LeetCode: Diameter of Binary Tree https://leetcode.com/problems/diameter-of-binary-tree/

  Given a binary tree, you need to compute the length of the diameter of
  the tree. The diameter of a binary tree is the length of the longest
  path between any two nodes in a tree. This path may or may not pass
  through the root.

Example:
Given a binary tree
          1
         / \
        2   3
       / \
      4   5

  Return 3, which is the length of the path [4,2,1,3] or [5,2,1,3].

  Note: The length of path between two nodes is represented by the
  number of edges between them.

 [TestClass]
    public class DiameterOfBinaryTreeTest
    {
        [TestMethod]
        public void DiameterOfBinaryTreeTest1()
        {
                        /* Constructed binary tree is
                       1
                     /   \
                   2      3
                 /  \
               4     5
             */
            TreeNode root = new TreeNode(1);
            root.left = new TreeNode(2);
            root.left.left = new TreeNode(4);
            root.left.right = new TreeNode(5);
            root.right = new TreeNode(3);
            DiameterOfBinaryTreeClass diam = new DiameterOfBinaryTreeClass();

            int result = diam.DiameterOfBinaryTree(root);
            Assert.AreEqual(3, result);
        }
}

 //https://leetcode.com/problems/diameter-of-binary-tree/
    public class DiameterOfBinaryTreeClass
    {
        public int DiameterOfBinaryTree(TreeNode root)
        {
            if (root == null)
            {
                return 0;
            }

            int leftDiameter = DiameterOfBinaryTree(root.left);
            int rightDiameter = DiameterOfBinaryTree(root.right);

            int leftHeight = Height(root.left);
            int rightHeight = Height(root.right);
            return Math.Max(leftHeight + rightHeight, Math.Max(leftDiameter,rightDiameter));
        }

        private int Height(TreeNode root)
        {
            if (root == null)
            {
                return 0;
            }

            return Math.Max(Height(root.left) ,Height(root.right)) + 1;
        }
    }

Please review for performance.

Your method visits mosts nodes twice: Here

int leftDiameter = DiameterOfBinaryTree(root.left);
int rightDiameter = DiameterOfBinaryTree(root.right);

int leftHeight = Height(root.left);
int rightHeight = Height(root.right);

calling DiameterOfBinaryTree on the left and right subtree determines the height of both subtrees, and then calling Height on the subtrees computes those heights again.

This can be improved by defining a helper function which (recursively) computes both height and diameter of a tree rooted at a node:

private (int height, int diam) HeightAndDiameter(TreeNode root)
{
    if (root == null)
    {
        return (0, 0);
    }

    var (leftHeight, leftDiam) = HeightAndDiameter(root.left);
    var (rightHeight, rightDiam) = HeightAndDiameter(root.right);

    return (Math.Max(leftHeight, rightHeight) + 1,
            Math.Max(Math.Max(leftDiam, rightDiam), leftHeight + rightHeight));
}

C# (named) tuples are used here to return both values to the caller. The main function then becomes

public int DiameterOfBinaryTree(TreeNode root)
{
    return HeightAndDiameter(root).diam;
}

Each node is visited exactly once now.


Simple framework for Google Code Jam problems My main concern is code style.  Could you review this?

#pragma once

#include <time.h>
#include <iostream>
#include <fstream>
#include <sstream>
#include <string>
#include <vector>
#include <iterator>
#include <algorithm>
#include <functional>
#include <math.h>
#include <exception>
#include "InfInt.h"
#include "Array2d.h"

template <typename ContainerT>
ContainerT splitString(const std::string & source) {
    ContainerT cont;
    std::stringstream ss(source);
    typedef typename ContainerT::value_type ElementT;
    std::copy(std::istream_iterator<ElementT>(ss), std::istream_iterator<ElementT>(), std::back_inserter(cont));
    return cont;
}

template <typename T>
std::string joinString(const T & v, const std::string & delim = " ") {
    std::ostringstream s;
    bool empty = true;
    for (const auto & i : v) {
        if (empty)
            empty = false;
        else
            s << delim;
        s << i;
    }
    return s.str();
}

class CodeJam {
public:
    std::stringstream input;
    std::stringstream output;
    int curCase;

    int solveJam(const std::string & data) {

        int res = 0;
        std::cout << "Solving jam:" << std::endl << std::endl;
        try {
            input.clear();
            output.clear();
            auto inputFile = "Data\\" + data + ".in";
            std::ifstream finput(inputFile);
            if (!finput.is_open())
                throw std::runtime_error("Can not open input file " + inputFile);
            auto outputFile = "Data\\" + data + ".out";
            std::ofstream foutput(outputFile);
            if (!foutput.is_open())
                throw std::runtime_error("Can not open output file " + outputFile);
            input << finput.rdbuf();
            input.seekg(0);

            auto elapsedTime = clock();
            res = solveAll();
            elapsedTime = clock() - elapsedTime;

            foutput << output.rdbuf();
            finput.close();
            foutput.close();

            std::cout << std::endl;
            std::cout << "Success! Finished " << res << " tests in " << elapsedTime << " ms" << std::endl;
        } catch (const std::exception & e) {
            std::cout << "Error: " << e.what() << std::endl;
        }
        std::cout << std::endl;
        system("pause");

        return res;
    }
protected:

    virtual int solveAll() {
        int testCases;
        readln(testCases);

        for (curCase = 1; curCase <= testCases; ++curCase) {
            solve(curCase);
            std::cout << "Test " << curCase << " of " << testCases << std::endl;
        };
        return testCases;
    }

    virtual void solve(int task) = 0;

public:
    std::string readLine() {
        std::string line;
        std::getline(input, line);
        return line;
    }

    template<typename T>
    T readLine() {
        std::string line;
        std::getline(input, line);
        T list = splitString<T>(line);
        return list;
    }

    std::vector<int> readInts() {
        return readLine<std::vector<int>>();
    }

    std::vector<std::string> readStrings() {
        return readLine<std::vector<std::string>>();
    }

    std::vector<double> readDoubles() {
        return readLine<std::vector<double>>();
    }

    void readln() {
        readLine();
    }

    template <typename T>
    void readln(T & t) {
        input >> t;
        input.ignore();
    }

    template <typename First, typename... Rest>
    void readln(First & first, Rest &... rest) {
        input >> first;
        readln(rest...);
    }

    template <typename T>
    void writeResult(T value) {
        output << "Case #" << curCase << ": " << value << std::endl;
    };
};

#pragma once

This really only makes sense in a header, but the rest of your code doesn't really look like a header (but maybe it's intended to be one anyway--not really sure). If it is a header, I'd recommend adding normal #ifndef/#define/#endif style header guards as well, since some compilers don't support this #pragma.

template <typename ContainerT>
ContainerT splitString(const std::string & source) {
    ContainerT cont;
    std::stringstream ss(source);
    typedef typename ContainerT::value_type ElementT;
    std::copy(std::istream_iterator<ElementT>(ss), std::istream_iterator<ElementT>(), std::back_inserter(cont));
    return cont;
}

I think I'd use something a bit simpler, such as:

template <typename ContainerT>
ContainerT splitString(const std::string & source) {
    std::stringstream ss(source);
    ContainerT cont{std::istream_iterator(ss), std::istream_iterator()};
    return cont;
}

If you're stuck with C++03 instead of 11, you'll need to use:

    ContainerT cont((std::istream_iterator(ss)), std::istream_iterator());

...instead (note the double parens around the first param, to avoid the Most Vexing Parse).

As @Mat already alluded to in a comment, the fact that this contains a pure virtual function indicates that it's intended to be used as a base class for derivation. If so, you almost certainly want to make the destructor virtual as well--this is needed if the user might ever create a pointer to the base class, and destroy an instance via that pointer to the base.

To be honest, this scenario strikes me as somewhat unlikely though. Such a virtual function is generally useful when you have something like a heterogeneous container that might contain pointers to objects of any of a number of derived classes, so the correct function to call must be determined individually for each item.

In this case, it appears more likely (at least to me) that the intent is to have only one derived class per program, so what we really have is "static polymorphism" -- i.e., in a given program, there will be only one derived class, so one program will really only have one implementation of solve.

If that's correct, it would probably be better to model that a bit more directly, such as by passing the solve as a template parameter instead:

template <class solve>
class CodeJam {
// ...
    void solveAll() {
        // ...
        for (curCase = 1; curCase <= testCases; ++curCase) {
            // Note that the syntax is only minimally affected:
            solve()(curCase);
            std::cout << "Test " << curCase << " of " << testCases << std::endl;
        }
    }
};

This seems to improve flexibility at least somewhat, because the user is no longer required to write a class that's derived from CodeJam.

At the same time, I feel obliged to point out what seems to me an even greater problem (and one that's not so easily fixed either, I'm afraid). This comes down to the simple fact that your code simply doesn't seem to provide all that much functionality. I can hardly imagine how anybody could hope to save even 10 whole minutes by using your code rather than starting from nothing at all--and in some cases, I can see where this code might easily be no more than a break-even proposition.

In short, you're placing constraints on how code is written, but providing rather minimal functionality in return. I question whether it provides enough return on investment to justify itself.


SStorage (remember) - v0 Looking for general feedback on this module.  remember is property that a user sets via clicking "remember me" when logging in.

A token identifies the user - h_token.  If localStorage.h_token is set, it is assumed the user has clicked remember me, this is checked at load time.

When the remember state is changed manually by the user setRemember() is called.

Not looking for advice on any security issues or naming conventions at the moment it is not a concern.

Just general things.

/*SClientStorage
**
**
**
*/
SStorage = NS.support({
    Name: 'Storage',
    hold: {},
    clientStorage: localStorage.h_token ? localStorage : sessionStorage,
    setRemember: function (remember) {
        this.clientStorage = remember ? localStorage : sessionStorage
    },
    clear: function () {
        this.clientStorage.clear();
    },
    setAll: function (o) {
        var key;
        for (key in o) {
            if (o.hasOwnProperty(key)) {
                this.clientStorage[key] = o[key];
            }
        }
    },
    getAll: function () {
        var o = {},
            key;
        for (key in this.clientStorage) {
            if (this.clientStorage.hasOwnProperty(key)) {
                o[key] = this.clientStorage[key];
            }
        }
        return o;
    },
    set: function (key, value) {
        this.clientStorage[key] = value;
    },
    get: function (key) {
        return this.clientStorage[key];
    }
});

Looks really good. It's clear and easy to understand. Regarding this specific implementation, a couple of things that came to mind:

In setRemember(), unless you specifically only want to support the values 1 or 0, the code can be changed to:

this.clientStorage = remember ? this.localStorage : this.sessionStorage;

In setRemember(), when switching storage mechanisms it might make sense to copy the values out of the old mechanism into the new one so anything the user has previously set will be preserved. Similary, if the user is changing from localStorage to sessionSorage, would it make sense to also clear the old storage mechanism? If making either of these changes, the method could also first compare the new value against the old value and only continue if there is actually a change.
What is o used for in log() method?
In the log() method, depending on on the utility you're using for logging, you might consider logging the entire object, at once, as many console / logging utilities will allow you to inspect objects. This can sometimes be more informative, although here it's just about whatever would be more convenient because all values will be strings, anyway. If you go this route, you could rename the current function to something like logEach().
In the execution of clear(), should you preseve the h_token key?

Taking a step back, here is an alternate API for the utilitiy. Just thinking out loud.

ssn(key) - Retrieves a value. Synonymous with get()
ssn(key, value) - Sets a value. Synonymous with set()
ssn(obj) - Set all values from obj. Synonymous with setAll()
ssn.clear() - Unchanged.
ssn.clone() - Synonymous with getAll()
ssn.log() - Log the clientStorage object as a whole, as described above.
ssn.logEach() - Log each property and corresponding value, one at a time.
ssn.remember() - Retrieves the current remember setting.
ssn.remember(value) - Sets the remember value. Synonymous with setRemember()

One final thought you may want to implement some sort of 'namespace' type of functionality to be able to differentiate between settings saved for different users. I don't suggest this for security reasons, but more from a user-experience perspective. I think, with the way it's set up, multiple users on the same computer will have their settings merged, which may get confusing.


Saving row versions in MySQL I just want to confirm that I am versioning my "versionable" fields correctly, and that there isn't another, better way to do this.

I have a table event, a record of an event in time. It has some fields that I want to be invariant, like it's ID and event series it is associated with. It also has some fields that I want to be editable, like the date or the description. The other issue is that I have foreign keys on the auto_increment ids of both tables, which is why I think I need two tables and not just one, or?  But I also want to keep the history of the variant fields, So I created two tables:

invariant

id int
series int
active boolean

and

variant

eventID int //foreign key to the invariant ID field
id int //this table needs its own id which serves as a foreign key on another table
date Date
description varchar(255)
active boolean

When an edit is made to the variant fields, I am switching the active Boolean on existing rows with the same eventID to false. then when I insert my new version, I can get the just latest version on the invariant - variant join by specifying where active=true.

If/when I want to delete the event entirely, I am setting active to false in the invariant table.

As I said at the top, I just want to confirm that this is an optimal solution for the specified requirements, or if there are better ways or things I am not understanding

Do not use an 'active' column as a way of joining a table.

  The other issue is that I have foreign keys on the auto_increment ids
  of both tables

I'm assuming you mean the invariant & variant tables both have PrimaryKeys which are auto_incremented. If so, this is fine. Most tables have an auto incremented primary key.

You could put all the columns onto the variant table (which is where they belong) and have an audit table. The audit table would contain the old value(s) & new value(s) along with the changed date.

If you wish to continue with your unique versioning system, I would still remove the active indicator on both tables, and instead sort by latest date to find the current. If no rows exist on invariant, either variant is 'inactive' or should be deleted.


Reading input from console in F# (as a sequence of lines) Consuming input line-by-line until the end is often useful. In C# I would write the following loop:

while ((line = Console.ReadLine()) != null) {
    // ...

I wanted to do this using "idiomatic" F# (which to me includes avoiding let mutable) and I came up with this:

let rec readlines = seq {
    let line = Console.ReadLine()
    if line <> null then
        yield line
        yield! readlines
}

The last line shows the following warning

  Warning FS0040: This and other recursive references to the object(s)
  being defined will be checked for initialization-soundness at runtime
  through the use of a delayed reference. This is because you are
  defining one or more recursive objects, rather than recursive
  functions. This warning may be suppressed by using '#nowarn "40"' or
  '--nowarn:40'.

The warning would suggest I am actually not writing idiomatic F# (as warnings usually point out problematic code). Is there a better way to write the function that avoids the warnings?

What you have there is a value, but you probably want it to be a function:

let rec readlines () = seq {
    let line = Console.ReadLine()
    if line <> null then
        yield line
        yield! readlines ()
}


Median of two sorted arrays The task is to find the median of two Arrays. I have an O(n) solution.
Can it be written in a better time complexity?

def findMedianSortedArrays(self, A, B):
    combined = []
    while len(A) > 0 and len(B) > 0:
        if (A[0] < B[0]):
            combined.append(A.pop(0))
        else:
            combined.append(B.pop(0))
    combined.extend(A)
    combined.extend(B)

    length = len(combined)
    if length % 2 != 0:
        return combined[length // 2]
    else:
        return (combined[length // 2-1] + combined[length//2]) / 2

According to the Style Guide for Python Code, function and variable names
should be “lowercase, with words separated by underscores as necessary to improve readability.” In your case I'd suggest

def median_of_sorted_arrays(a, b):

Your code determines the median correctly. However, it modifies the
passed arguments, which might be unexpected to the caller:

a = [-5, 3, 6, 12, 15]
b = [-12, -10, -6, -3, 4, 10]

print(findMedianSortedArrays(a, b))   # prints 3
print(findMedianSortedArrays(a, b))   # prints 13.5

This could be fixed by accessing the lists via subscripting:

i = 0
j = 0
while i < len(a) and j < len(b):
    if (a[i] < b[j]):
        combined.append(a[i])
        i += 1
    else:
        combined.append(b[j])
        j += 1
combined.extend(a[i:])
combined.extend(b[j:])

The next possible improvement is to get rid of the additional storage:
Instead of storing elements from a or b into the combined list
you could just count how many elements have been merged so far,
until the median element is found.

A more efficient algorithm is described in

Median of two sorted arrays with different sizes in O(log(min(n, m)))

The basic idea is to partition both a and b into

a[:i] + a[i:]
b[:j] + b[j:]

such that the following conditions are satisfied:

i + j = (len(a) + len(b) + 1) // 2
a[i-1] <= b[j]
b[j-1] <= a[i]

and this can be done efficiently with a binary search. Then the “union”
of the first (resp. second) halves is the first (resp. second) half of the combined array, so that the median is

max(a[i - 1], b[j - 1])   # if the total length is odd
(max(a[i - 1], b[j - 1]) + min(a[i], b[j])) / 2 # if the total length is even


Scrape google apps page and store application details in database Below is a python script which scrapes specific google apps url for example https://play.google.com/store/apps/details?id=com.mojang.minecraftpe and save the developer email,application name and price in mysql database with database name "app_database" and inside table named "app_table". There are lot of data type conversions and workarounds here and i think that might not come under best practices. How can I improve this.

from lxml import html
import requests
import MySQLdb
conn = MySQLdb.connect("localhost","root","pA$$W0rd","app_database")
cursor=conn.cursor()
class AppCrawler:

    def __init__(self,starting_url,depth):
        self.starting_url=starting_url
        self.depth=depth
        self.current_depth=0
        self.depth_links=[]
        self.apps=[]

    def crawl(self):
        app=self.get_app_from_link(self.starting_url)
        self.apps.append(app)
        self.depth_links.append(app.links)

        while self.current_depth < self.depth:      #0 < 0
            current_links=[]
            for link in self.depth_links[self.current_depth]:
                current_app = self.get_app_from_link(link)
                current_links.extend(current_app.links)
                self.apps.append(current_app)
            self.current_depth += 1
            self.depth_links.append(current_links)

        return

    def get_app_from_link(self,link):
        start_page=requests.get(link)
        #print start_page.text
        tree = html.fromstring(start_page.text)
        # the tree.xpath(...) gives an array
        name = tree.xpath('//div[@class="id-app-title"]/text()')[0]

        #developer=tree.xpath('//a[@class="dev-link"]//*/div/@href')
        temp_addr=tree.xpath('//a[contains(@href,"mailto") and @class="dev-link"]/@href')[0]
        developer=temp_addr.split(':')

        price=tree.xpath('//button[@class="price buy id-track-click id-track-impression"]/span[2]/text()')
        temp_price=price[0].split()
        price=str(temp_price[0].lstrip('$'))
        if price=="Install":
            price="0"
        templinks = tree.xpath("//a[@class='title']/@href")

        links=[]
        length=len(templinks)
        domain="https://play.google.com"
        for i in templinks:
            i="https://play.google.com"+i
            links.append(i)
#        print links
        #print len(testlinks)
        #links = [h for h in tree.xpath("//a[@class='title']/@href")]
        #print links
        #return name,developer[1],price,links
        app = App(name,developer[1],price,links)
        return app

class App:
    def __init__(self, name,developer,price,links):
        self.name=name
        self.developer=developer
        self.price = price
        self.links=links

    def __str__(self):
        return(self.name.encode('UTF-8')+"|"+
        self.developer.encode('UTF-8')+"|"+
        self.price.encode('UTF-8'))

crawler = AppCrawler('https://play.google.com/store/apps/details?id=com.mojang.minecraftpe',0)
crawler.crawl()
#print type(crawler.apps)

query="INSERT INTO app_table (name,developer,price) values('%s','%s','%d ')"
for app in crawler.apps:
    data=str(app).split('|',2)
    price=float(data[2])
    try:
        cursor.execute(query % (data[0],data[1],price))
        print "OK"
        conn.commit()
    #row=x.fetcha
    except:
        print "Failed"
        conn.rollback()
    print data[0],data[1],price

This is being ported over to python 3.x from python 2.x

Docstrings: You should include a docstring at the beginning of every method/class/module you write. This will allow documentation to identify what your code is supposed to do. PEP-8 Docstring Conventions.
Variable / Operator Spacing: When not in default parameters, you should put spaces between variables, the =, and the content it's being assigned. Also, there should be spaces between variables that are being passed to methods. PEP-8 Whitespace Conventions.
Import Ordering: Standard library imports should be put first, then related third party imports, then local libraries/files, all grouped and separated by a blank line. PEP-8 Import Conventions.
String Formatting: Your __str__ in your App class is a bit messy. If you see this behavior, something is afoot. This is a red flag to me. I use f"" to directly include variables into my strings, instead of having to type cast them or, in your case, having to use multiple + to concatenate the variables and the strings.
Unnecessary return statements: Your return in your crawl method is unnecessary. Python automatically returns at the end of functions, so having that return isn't needed.

Updated Code

"""
Module Docstring:
A description of your program goes here
"""

import requests
import MySQLdb

from lxml import html

class AppCrawler:
    """
    Class for crawling a link, and gathering information about an app
    """
    def __init__(self, starting_url, depth):
        """
        AppCrawler Class Init Method

        :param starting_url: The starting url for the crawler
        :param depth: How deep the app crawler should go

        """
        self.starting_url = starting_url
        self.depth = depth
        self.current_dept = 0
        self.depth_links = []
        self.apps = []

    def crawl(self):
        """
        Crawls to find links connected to apps
        """
        app = self.get_app_from_link(self.starting_url)
        self.apps.append(app)
        self.depth_links.append(app.links)

        while self.current_depth < self.depth: #0 < 0
            current_links = []
            for link in self.depth_links[self.current_depth]:
                current_app = self.get_app_from_link(link)
                current_links.extend(current_app.links)
                self.apps.append(current_app)
            self.current_depth += 1
            self.depth_links.append(current_links)

    def get_app_from_link(self, link):
        """
        Gets the app from the passed `link`

        :param link: Link to the app

        """
        start_page = requests.get(link)

        tree = html.fromstring(start_page.text)

        name = tree.xpath('//div[@class="id-app-title"]/text()')[0]

        temp_addr = tree.xpath('//a[contains(@href,"mailto") and @class="dev-link"]/@href')[0]
        developer = temp_addr.split(':')

        price = tree.xpath('//button[@class="price buy id-track-click id-track-impression"]/span[2]/text()')
        temp_price = price[0].split()
        price = str(temp_price[0].lstrip('$'))
        if price == "Install":
            price = "0"
        templinks = tree.xpath("//a[@class='title']/@href")

        links = []
        length = len(templinks)
        domain = "https://play.google.com"
        for i in templinks:
            i = f"https://play.google.com{i}"
            links.append(i)
        app = App(name, developer[1], price, links)
        return app

class App:
    """
    Class to represent each app getting crawled
    """
    def __init__(self, name, developer, price, links):
        """
        App Class Init Method

        :param name: Name of the app
        :param developer: Developer of the app
        :param price: Price of the app
        :param links: list of links to the app

        """
        self.name = name
        self.developer = developer
        self.price = price
        self.links = links

    def __str__(self):
        """
        Returns a string representation of the app
        """
        return f"{self.name.encode('UTF-8')}|{self.developer.encode('UTF-8')}|{self.price.encode('UTF-8')}"

if __name__ == '__main__':

    conn = MySQLdb.connect("localhost","root","pA$$W0rd","app_database")
    cursor = conn.cursor()

    crawler = AppCrawler('https://play.google.com/store/apps/details?id=com.mojang.minecraftpe', 0)
    crawler.crawl()

    query = "INSERT INTO app_table (name,developer,price) values('%s','%s','%d ')"
    for app in crawler.apps:
        data = str(app).split('|', 2)
        price = float(data[2])
        try:
            cursor.execute(query % (data[0], data[1], price))
            print("OK")
            conn.commit()
        except:
            print("Failed")
            conn.rollback()
        print(data[0], data[1], price)


Inserting a node at a specific position in a linked list Given a node class like

class Node {
     int data;
     Node next;
  }

Provide an implementation to Insert a node at a specific position in a linked list

Node insert(Node head, int data, int position) {

    Node newNode = new Node();
    newNode.data = data;
    newNode.next = null;

    if(head == null && position != 0) {
        return head;
    } else if(head == null && position == 0) {
        head = newNode;
        return head;
    }

    if(position == 0) {
        newNode.next = head;
        head = newNode;
        return head;
    }

    Node current = head;
    Node previous = null;
    int i = 0;

    for(i = 0; i < position; i++) {
        previous = current;
        current = current.next;
        if(current == null)
            break;
    }

    newNode.next = current;
    previous.next = newNode;
    return head;

}

Can you please critic my code and provide your thoughts on where I should improve my code.

Avoid special cases

If position == null the new node becomes head no matter what, and you don't need to care what the head was before.

Similarly, the validity of position is determined in a traversal loop (if (current == null) condition); testing specifically for head == null && position != 0 is premature.

BTW, the code handles invalid position inconsistently. Yet another cause to avoid special cases.
Avoid naked loops

Every loop does an important job, and deserves a name. The loop

for(i = 0; i < position; i++)

serves a purpose of finding a node at the particular position. Factor it out into a method and name it properly.


Simple multithreaded chat server in Rust I'm trying to develop a simple Rust chat server. I'm not a Rust expert and come from Java and Kotlin.

This same server in Kotlin is:

import java.io.*
import java.net.*
import kotlin.concurrent.thread
import java.util.concurrent.ConcurrentHashMap

fun main(args: Array<String>) {
    val serv = ServerSocket(Integer.parseInt(args[0]))
    //val users = ConcurrentHashMap<String, PrintWriter>()
    val users = Collections.synchronizedMap(HashMap<String, PrintWriter>())

    while (true) {
        val s = serv.accept()
        thread {
            var sin = s.getInputStream().bufferedReader()
            val sout = PrintWriter(s.getOutputStream(), true)

            // read nick
            val nick = sin.readLine()
            users.put(nick, sout)

            sin.forEachLine {
                for (peer in users.values) {
                    if (peer == sout) continue
                    peer.println(it)
                }
            }
            users.remove(nick)
        }
    }
}

After many attempts I've come up with a working Rust implementation :

use std::env;
use std::io;
use std::io::Write;
use std::io::{LineWriter, BufReader, BufRead};
use std::net::{TcpListener, TcpStream};
use std::sync::{Arc, Mutex};
use std::collections::HashMap;
use std::thread;
use std::ops::DerefMut;

fn main() -> io::Result<()> {
    let args: Vec<String> = env::args().collect();
    let server_socket = TcpListener::bind(&*format!("localhost:{}", args[1]))?;
    let users: Arc<Mutex<HashMap<String, LineWriter<TcpStream>>>> = Arc::new(Mutex::new(HashMap::new()));

    for socket in server_socket.incoming() {
        let users = users.clone();
        thread::spawn(move || -> io::Result<()> {
            let socket = socket?;
            let socket_copy = socket.try_clone()?;
            let mut inp = BufReader::new(socket);
            let out = LineWriter::new(socket_copy);

            // read nick
            let mut nick = String::new();
            inp.read_line(&mut nick)?;
            nick.pop(); // discard '\n'
            let nick_copy = nick.clone();
            {
                let mut users = users.lock().unwrap();
                users.insert(nick, out);
            }

            for line in inp.lines() {
                {
                    let line = line?;
                    let mut users = users.lock().unwrap();
                    for (nick, peer) in users.deref_mut() {
                        if *nick == nick_copy { continue; }
                        writeln!(peer, "{}", line)?;
                    }
                }
            }

            // remove nick
            {
                let mut users = users.lock().unwrap();
                users.remove(&nick_copy);
            }
            Ok(())
        });
    }
    Ok(())
}

Too verbose for my taste.

I'm particularly dissatisfied with those clone ops. When to move? When to borrow?

I'd prefer to use for peer in users.values_mut() instead of for (nick, peer) in users.deref_mut() but then I've problems when comparing LineWriter references to discard sender.

I'd also prefer to use RwLock instead of Mutex.

Any pointers to simplify/enhance code will be greatly appreciated.

I made some changes to your code (see below). You can check the code for what I've done so far: I added some comments on it so you can compare it with your version.  I'll note some important points in here. Please ask if you have any questions; I will try to explain or we can improve the code together.

Propagating error to the main function from a spawned thread is not possible. It's only possible after joined to the main thread (you can use join handler then propagate your anonymous function's return value to the main).
Created ConnectionsSync struct as helper to get rid of Arc<RwLock<HashMap< initialization: it was looking too messy; also with this way you can add behaviors like (add_user, broadcast_message, remove_user, ... )
Incoming has Iterator implementation so I can use it as iterator and I used try_for_each to tell that I am going to apply fallible function, then it will be propagated to main.

Code

use std::collections::HashMap;
use std::env;
use std::io;
use std::io::{BufRead, BufReader, LineWriter, Write};
use std::net::{TcpListener, TcpStream};
use std::sync::{Arc, RwLock};
use std::thread;

type User = Arc<String>;
struct ConnectionsSync(Arc<RwLock<HashMap<User, LineWriter<TcpStream>>>>);
impl ConnectionsSync {
    fn new() -> Self {
        ConnectionsSync(Arc::new(RwLock::new(HashMap::new())))
    }
}

fn main() -> io::Result<()> {
    let port = env::args().nth(1).unwrap_or("8080".to_string()); //default port 8080
    let connections = ConnectionsSync::new();

    TcpListener::bind(&format!("localhost:{}", port))?
        .incoming()
        .try_for_each(|socket| -> io::Result<()> {
            let socket = socket?;
            let connections = connections.0.clone(); //increases ref count

            thread::spawn(move || {
                let (mut reader, writer) = socket
                    .try_clone()
                    .map(|socket1| (BufReader::new(socket1), LineWriter::new(socket)))
                    .unwrap();

                // read nick
                let mut nick = String::new();
                reader
                    .read_line(&mut nick)
                    .and_then(|_| Ok(nick.pop()))
                    .expect("Not able to read user nick");

                let current_user = Arc::new(nick);
                //add user
                connections
                    .write()
                    .unwrap()
                    .insert(current_user.clone(), writer);

                //read user input
                reader.lines().into_iter().for_each(|line| {
                    let line = line.expect("Not able to read user input");
                    connections
                        .write()
                        .unwrap()
                        .iter_mut()
                        .filter(|(other_user, _)| **other_user != current_user)
                        .for_each(|(_, peer)| {
                            writeln!(peer, "{} \t-> {}", current_user, line);
                        })
                });

                //discard user
                connections.write().unwrap().remove(&current_user);
            });

            Ok(())
        })
}

Edit for questions in comment section

  Can we get rid of nick.clone() and keep a simple reference? I guess not because cannot move out of borrowed content despite nick being
  immutable.

Actually cannot move out of borrowed content error is not related being immutable or mutable. Consider this; Your friend borrowed a phone from you and moved it's battery out physically to use somewhere else. Then your friend gave it your phone back to you without a battery. You would probably give an instant error. So due to safety, Rust watches your friend and warns: "Hey your friend is going to need that part in the future, so stop it!"

Besides the error you mentioned, another error will be occurred if we get rid of nick.clone() and keep a simple reference: We'll face with "borrowed value does not live long enough". This happens because we have a HashMap which it's reference counted, so we don't exactly know when it is going to die. If we put our nick's simple reference into HashMap, the compiler will tell that: "Hey HasMap is the owner of nick's reference but nick is gonna die in that block and HashMap will live after that, so what i am gonna do with this reference in the future ? ". We can solve this problem with 2 solution.

With cloning because we have 2 ownership of nick (HashMap, Thread Block). I believe you don't want this solution due to overhead of copying String.
With Smart Pointers Like Rc(Reference Counted), we will give nick's ownership to the Rc and share it across the HashMap and Thread. But Rc is not thread safe so we need to use Arc instead of it.i updated code with Arc version, there will be still cloning in code but it will just increase the reference count instead of copying the whole String.

  Can we filter on value references instead of key objects?

Sure you can do it simply you need to change your code like this:

.filter(|(_other_user, other_users_value)| other_users_value != current_users_value)

But in our case this will not work because Eq is not implemented for TCPStream, if you implement it by yourself it will work.

Edit: Added unsafe version with raw pointers instead of clone

let mut users = HashMap::new();

let nick = "SomeInput".to_string();
let nick_ptr = &nick as *const String;

users.insert(nick, "SomeValue".to_string());

unsafe {
    users.remove(&*nick_ptr);
    println!("{:?}", users.get(&*nick_ptr)); //output None
}


Loading a Protein Data Bank file into a numpy matrix Here is my code:

def read_Coordinates_Atoms2(fileName, only_CA = True):
    '''
    in : PDB file
    out : matrix with coordinates of atoms
    '''
    with open(fileName, 'r') as infile:
        for line in infile :
            if only_CA == True :
                if line.startswith('ATOM') and line[13:15] == 'CA':
                    try:    # matrix fill-up
                        CoordAtoms = np.vstack([CoordAtoms, [float(line[30:38]), float(line[38:46]), float(line[46:54])]]) # np.append
                    except NameError:  # matrix declaration
                        CoordAtoms = np.array([[line[30:38],line[38:46], line[46:54]]], float)
            else :
                if line.startswith('ATOM'):
                    try:    # matrix fill-up
                        CoordAtoms = np.vstack([CoordAtoms, [float(line[30:38]), float(line[38:46]), float(line[46:54])]]) # np.append
                    except NameError:  # matrix declaration
                        CoordAtoms = np.array([[line[30:38],line[38:46], line[46:54]]], float)
        return CoordAtoms

Is there a more efficient way to do this ? I mean, a way where I don't have to write twice the same lines? I think the code should look more like this :

def foo(file, condition2 = True):
    if condition1 and condition2 :
        # do lots of instructions
    elif condition1 :
        # do the same lots of instructions (but different output)

Seeing that both your blocks are identical, you can be able to merge them using boolean logic.

First thing is that, in each case, you perform line.startswith('ATOM') so put that first.

Second, either you have only_CA being True and you need 'CA' at line[13:15] too, or you have only_CA being False. In other words, you keep the line if either only_CA is False or 'CA' is at line[13:15].

This lets you rewrite your for loop as:

for line in infile:
    if line.startswith('ATOM') and (not only_CA or line[13:15] == 'CA'):
        try:    # matrix fill-up
            CoordAtoms = np.vstack([CoordAtoms, [float(line[30:38]), float(line[38:46]), float(line[46:54])]]) # np.append
        except NameError:  # matrix declaration
            CoordAtoms = np.array([[line[30:38],line[38:46], line[46:54]]], float)

You can also extract out the line parsing at it is somehow repeated:

for line in infile:
    if line.startswith('ATOM') and (not only_CA or line[13:15] == 'CA'):
        data = [line[30:38], line[38:46], line[46:54]]
        try:    # matrix fill-up
            CoordAtoms = np.vstack([CoordAtoms, [float(x) for x in data]]) # np.append
        except NameError:  # matrix declaration
            CoordAtoms = np.array([data], float)

But you can also simplify the whole thing by converting your data to float before the try and feeding np.array data of the correct type:

for line in infile:
    if line.startswith('ATOM') and (not only_CA or line[13:15] == 'CA'):
        data = [float(line[begin:end]) for begin, end in ((30, 38), (38, 46), (46, 54))]
        try:    # matrix fill-up
            CoordAtoms = np.vstack([CoordAtoms, [data]]) # np.append
        except NameError:  # matrix declaration
            CoordAtoms = np.array([data])


Advent of Code 2016 Day 1 "No Time for a Taxicab" I already solved it in JavaScript, where I'm more comfortable. I pretty much ported the algorithm from there.

I wanted to try the first Advent of Code puzzle in a new language and I picked Racket.

It's a taxicab geometry navigation problem. Given input R3, L5 you:

turn right 90 degrees
move forward 3 steps
turn left 90 degrees
move forward 5 steps

The goal: at the end of the instructions:

  how far are you from the start?

It's my first Racket program. I've read around Lisps but not written much. How could I improve it?

#lang racket

; convert "L1" to '(#\L 1)
(define (parse step)
  (let ([dir (string-ref step 0)]
        [distance (string->number (substring step 1))])
    (list dir distance)))

; given a direction (#\L or #\R) and the last heading 0-3,
; compute the heading after the next step
(define (get-heading dir last-heading)
  (define raw
    (match dir
      [#\L (+ last-heading 1)]
      [#\R (- last-heading 1)]))
  (modulo raw 4))

; compute the x coordinate after the next step
(define (get-x heading last-x distance)
  (match heading
    [1 (+ last-x distance)]
    [3 (- last-x distance)]
    [_ last-x]))

; compute the y coordinate after the next step
(define (get-y heading last-y distance)
  (match heading
    [0 (+ last-y distance)]
    [2 (- last-y distance)]
    [_ last-y]))

; iterate over the instructions, computing
; x, y and heading after each step
(define (walk steps)
  (define-values (x y _)
    (for/fold
        ([x 0] [y 0] [heading 0])
        ([step steps])
      (match-define (list dir distance) step)
      (let* ([next-heading (get-heading dir heading)]
             [next-x (get-x next-heading x distance)]
             [next-y (get-y next-heading y distance)])
        (values next-x next-y next-heading))))
    (+ (abs x) (abs y))
  )

;;;; input
(define input "R3, L5, R1, R2, L5, R2, R3, L2, L5, R5, L4, L3, R5, L1, R3, R4, R1, L3, R3, L2, L5, L2, R4, R5, R5, L4, L3, L3, R4, R4, R5, L5, L3, R2, R2, L3, L4, L5, R1, R3, L3, R2, L3, R5, L194, L2, L5, R2, R1, R1, L1, L5, L4, R4, R2, R2, L4, L1, R2, R53, R3, L5, R72, R2, L5, R3, L4, R187, L4, L5, L2, R1, R3, R5, L4, L4, R2, R5, L5, L4, L3, R5, L2, R1, R1, R4, L1, R2, L3, R5, L4, R2, L3, R1, L4, R4, L1, L2, R3, L1, L1, R4, R3, L4, R2, R5, L2, L3, L3, L1, R3, R5, R2, R3, R1, R2, L1, L4, L5, L2, R4, R5, L2, R4, R4, L3, R2, R1, L4, R3, L3, L4, L3, L1, R3, L2, R2, L4, L4, L5, R3, R5, R3, L2, R5, L2, L1, L5, L1, R2, R4, L5, R2, L4, L5, L4, L5, L2, L5, L4, R5, R3, R2, R2, L3, R3, L2, L5")
(define steps
  (map parse
    (string-split input ", ")))

;;;; run
(walk steps)

Generative approach
The idea of functionally generating the next value from the current value rather than mutating a location is consistent with good Racket practice.
Naming

Instead of 0 1 2 3 using the symbols 'north...it's not obvious what comes next without taking notes and working through the code and figuring out the implementation details...'south 'east 'west is more typical of Racket and other Lisps. The reason is that it aligns the level of abstraction in the program with the abstractions in the problem domain.

When I see the parameter dir in the procedure get-heading my first thought is that it's type is a directory. The code base starting with low level string parsing probably sets me up. The extra characters of directory are worth the added clarity.

Abstractions
As with Naming 1. the abstractions in the code don't closely follow the business logic of the problem statement. There's no reference to Easter Bunny Headquarters or Taxicab Distance in the code. If the specification changed from taxicab to crowflight distance, it's not immediately obvious how to change the code.
  (define (distance-to-easterbunny-headquarters x y)
    (+ (abs x) (abs y))

Makes the code more modular and expresses the 'business logic' in the underlying problem explicitly. Because Racket treats functions as data, higher order functions allow further expression of the business logic and decoupling within the implementation.
  (define (taxicab-distance x y)
    (+ (abs x) (abs y))
  (define (distance-to-easterbunny-headquarters x y distance-function)
    (distance-function x y)

Even in the absence of change, the code will be more readable. And by extension a reviewer will spend less time trying to map the problem statement into the code.
In Racket, unlike many other languages, the idiom is to err on the side of verbosity and explanation. The guiding document is How to Design Programs (first edition, second edition) often referred to as HtDP. The methodology includes lots and lots of comments...even for someone who thinks the Easter Bunny and self-documenting code share some common features. Code golf the method is not.
Data Structures
HtDP methodology starts with data structures. The data structures should reflect the problem domain / business logic. Conversely as a general rule, working with values is a often code smell in Racket. There are a few corner cases where it is necessary and perhaps a few existing code bases where doing so maintains consistency. Otherwise it is likely that data structures will produce more modular and easier to read code.
Being only parameters passed to procedures, the review code's data structures are ad hoc and diffuse. Programmer defined data types are an idiomatic alternative.
(struct position (x y orientation))
(struct instruction (rotation distance))

These programmer defined data types automatically provide functions like position-x and instruction-rotation that align with the business logic and require. During debugging, a programmer defined data type provides something explicit to inspect. As the code evolves it provides a single place to implement changes, a known type to pass among procedures, semantics for creating a list of easter bunny headquarters locations, or hash table to memoize intermediate locations.
Formatting
Racket also has a style guide. It is typical to limit line length to 102 characters. For example:
(define input "R3, L5, R1, R2, L5, R2, R3, L2, L5, R5, L4, L3, R5, L1, R3,
               R4, R1, L3, R3, L2, L5, L2, R4, R5, R5, L4, L3, L3, R4, R4,
               R5, L5, L3, R2, R2, L3, L4, L5, R1, R3, L3, R2, L3, R5")

is a typical way to handle long lists for improved readability.
The style guide also favors define over let and it's related forms. The advantages beyond readability include not having to debug against the differences between let let* let-rec.
Proximity
Explicitly naming is good. Limiting the scope of symbols lexically can avoid subtle bugs. The first definition in the code is parse it is not used until the last definition and it is only used there.
Nesting the definitions is probably appropriate here. It might also have made my confusion in Naming 2. less likely.
(define steps

  ; convert "L1" to '(#\L 1)
  (define (parse step)
    (let ([dir (string-ref step 0)]
          [distance (string->number (substring step 1))])
      (list dir distance)))

  (map parse
    (string-split input ", ")))