Deck of cards design Can someone please review this design I created for deck of cards in Java?

package prep.design;

import java.lang.reflect.Array;
import java.util.*;

/**
 * Created by rohandalvi on 5/14/16.
 */
public class CardDeck {

    private ArrayList<Card> cards;

    CardDeck(){
        cards = new ArrayList<>();
        for(Card.Suit s: Card.Suit.values()){
            for(Card.Rank r: Card.Rank.values()){
                cards.add(new Card(r,s));
            }
        }
    }

    public void print(){
        for(Card card: cards){
            System.out.println("Card: "+card.getRank()+" Suit: "+card.getSuit());
        }
    }

    public void shuffle(ArrayList<Card> c){
        if(c==null){
            Collections.shuffle(cards);
        }else{
            Collections.shuffle(c);
        }

    }

    public static void main(String[] args) {
        CardDeck cd = new CardDeck();
        cd.shuffle(null);
        //cd.print();

        cd.deal(5);
    }

    public void deal(int numberOfPlayers){
        ArrayList<Card> tempDeck = new ArrayList<>(cards);
        Map<Integer,List<Card>> playerDeck = new HashMap<>();
        //List<List<Card>> playerDeck = new ArrayList<>();
        shuffle(tempDeck);
        shuffle(tempDeck);

        int i=0;
        while(i!=52){
            int j = i%numberOfPlayers;
            List<Card> tempList;
            if(playerDeck.containsKey(j)){
                tempList = playerDeck.get(j);
            }else{
                tempList = new ArrayList<>();
            }
            tempList.add(tempDeck.get(i));
            playerDeck.put(j,tempList);

            i++;

        }

        System.out.println("Dealt");

        displayPlayerCards(playerDeck);

    }

    public void displayPlayerCards(Map<Integer,List<Card>> playerDeck){
        int i=0;
        for(Integer player: playerDeck.keySet()){
            List<Card> playerCards = playerDeck.get(player);

            System.out.println("Player "+i);
            for(Card c: playerCards){
                System.out.print("Rank: "+c.getRank()+" Suit: "+c.getSuit()+"\t");
            }
            System.out.println();
            i++;
        }
    }
}

class  Card{
    Suit s;
    Rank r;
    public  enum  Suit{
        Spade , Heart , Diamond , Clubs
    }

    public enum Rank{
        ACE(1) , TWO(2), THREE(3), FOUR(4), FIVE(5) , SIX(6), SEVEN(7), EIGHT(8), NINE(9), TEN (10), JACK(11), QUEEN (12), KING(13);

        int priority;
        Rank(int s) {
            priority = s;
        }

        public int getPriority(){
            return priority;
        }

        public Rank getRankByPriority(int p){
            switch (p){
                case 1: return Rank.ACE;
                case 2: return Rank.TWO;
                case 3: return Rank.THREE;
                case 4: return Rank.FOUR;
                case 5: return Rank.FIVE;
                case 6: return Rank.SIX;
                case 7: return Rank.SEVEN;
                case 8: return Rank.EIGHT;
                case 9: return Rank.NINE;
                case 10: return Rank.TEN;
                case 11: return Rank.JACK;
                case 12: return Rank.QUEEN;
                case 13: return Rank.KING;

                default: return null;
            }

        }
    }

    Rank getRank(){
        return r;
    }

    Suit getSuit(){
        return s;
    }

    Card(Rank r, Suit s){
        this.r = r;
        this.s = s;
    }

}

class cardComparator implements java.util.Comparator<Card.Rank>{

    @Override
    public int compare(Card.Rank o1, Card.Rank o2) {
        if(o1.getPriority() > o2.getPriority()){
            return 1;
        }else if(o1.getPriority() < o2.getPriority()){
            return -1;
        }else{
            return 0;
        }
    }

    @Override
    public Comparator<Card.Rank> reversed() {
        return null;
    }
}

Card class

Specify the access modifiers of the class and members properly.
Use proper names for members:

class  Card{
    Suit s;
    Rank r;

for example:

public class  Card{
    private Suit suit;
    private Rank rank;

print() method

It's better to only create a String representation of an object and return that. You override toString() method which will be used implicitly if the object has to be converted to a String.

It's good practice to override toString() for all custom classes, because this is where people will look for when searching for a String representation of the object. You called it print(), or was it info()? , who knows? Your future self? Probably not. toString() is the way to go.

Your current approach appears to be more convenient, but actually causes some trouble like duplicated code:

System.out.print("Rank: "+c.getRank()+" Suit: "+c.getSuit()+"\t");
System.out.println("Card: "+card.getRank()+" Suit: "+card.getSuit());

Card.toString() could look like this:

@Override
public String toString()
{
    return "rank: " + rank + "\t suit: " + suit;
}

CardDeck is pretty much a collection of a number of cards. CardDeck.toString() could look something like this:

@Override
public String toString()
{
    String result = cards.size() + " cards:" + System.lineSeparator();
    for (Card card : cards)
    {
        result = result.concat(card + System.lineSeparator());
    }

    return result;
}

Here's what both classes look like so var with a little example program:

Card.java

public class  Card
{
    private Suit suit;
    private Rank rank;

    public  enum  Suit
    {
        Spade , Heart , Diamond , Clubs
    }

    public enum Rank
    {
        ACE(1) , TWO(2), THREE(3), FOUR(4), FIVE(5) , SIX(6), SEVEN(7), EIGHT(8), NINE(9), TEN (10), JACK(11), QUEEN (12), KING(13);

        int priority;

        Rank(int s)
        {
            priority = s;
        }

        public int getPriority()
        {
            return priority;
        }

        public Rank getRankByPriority(int p)
        {
            switch (p)
            {
                case 1: return Rank.ACE;
                case 2: return Rank.TWO;
                case 3: return Rank.THREE;
                case 4: return Rank.FOUR;
                case 5: return Rank.FIVE;
                case 6: return Rank.SIX;
                case 7: return Rank.SEVEN;
                case 8: return Rank.EIGHT;
                case 9: return Rank.NINE;
                case 10: return Rank.TEN;
                case 11: return Rank.JACK;
                case 12: return Rank.QUEEN;
                case 13: return Rank.KING;

                default: return null;
            }
        }
    }

    Card(Rank rank, Suit suit)
    {
        this.rank = rank;
        this.suit = suit;
    }

    Rank getRank()
    {
        return rank;
    }

    Suit getSuit()
    {
        return suit;
    }

    @Override
    public String toString()
    {
        return "rank: " + rank + "\t suit: " + suit;
    }
}

CardDeck.java

import java.util.ArrayList;

public class CardDeck
{
    private ArrayList<Card> cards;

    CardDeck()
    {
        cards = new ArrayList<>();

        for(Card.Suit s: Card.Suit.values())
        {
            for(Card.Rank r: Card.Rank.values())
            {
                cards.add(new Card(r,s));
            }
        }
    }

    public static void main(String[] args)
    {
        CardDeck deck = new CardDeck();

        System.out.println(deck);
    }

    @Override
    public String toString()
    {
        String result = cards.size() + " cards:" + System.lineSeparator();
        for (Card card : cards)
        {
            result = result.concat(card + System.lineSeparator());
        }

        return result;
    }
}

result (excerpt):

$ java CardDeck
52 cards:
rank: ACE    suit: Spade
rank: TWO    suit: Spade
rank: THREE  suit: Spade
rank: FOUR   suit: Spade
rank: FIVE   suit: Spade
rank: SIX    suit: Spade
rank: SEVEN  suit: Spade
rank: EIGHT  suit: Spade
rank: NINE   suit: Spade
rank: TEN    suit: Spade
...

shuffle() method

A method is a block of code that operates on an object. Your shuffle() method might work on an optional parameter which is entirely independent from the CardDeck object. The part of the method that operates on an arbitrary ArrayList<Card> should be static.

public void shuffle(ArrayList<Card> c){
        if(c==null){
            Collections.shuffle(cards);
        }else{
            Collections.shuffle(c);
        }

    }

Thus splits into:

public static void shuffle(ArrayList<Card> cards)
{
    if(cards!=null)
    {
        Collections.shuffle(cards);
    }
}

public void shuffle()
{
    Collections.shuffle(cards);
}

The second one makes a lot of sense, but the static version does not. What's the point of calling this method directly when you already have Collections.shuffle()?

Also, the static version has nothing to do with the CardDeck class whatsoever. It should not be a member of CardDeck, but either Card or better a separate class with utility functionality named CardUtils for example. But again, there's no real point in having that functionality that does not add anything to Collections.shuffle(). Delete it.

deal() method

I have no idea what this is doing by only looking at it. There are so many card games and all have their own ways to deal cards. Does every player get the same number of cards? Or are all cards distributed to a number of players?

Trying your code it looks like the later is true.

The problem with this method is that it does not return anything. It only prints the piles of card out, without any chance to do anything with them. Instead, provide a useful return value you can work with.

One such return value could be CardDeck. There's a bit of functionality already in there, so why not reuse that? Add a constructor to be able to pass a Collection of cards to it:

CardDeck(Collection<? extends Card> cards)
{
    this.cards = new ArrayList<Card>(cards);
}

When dealing the cards, there's no real point in dealing them one by one. You know how many cards each player receives and should just get that many from the shuffled cards. Think about it like every player takes a certain number of cards from the deck.

public ArrayList<CardDeck> dealAllCards(int numberOfPlayers)
{
    ArrayList<CardDeck> playerHands = new ArrayList<>(numberOfPlayers);
    ArrayList<Card> shuffledDeck = new ArrayList<>(cards);

    int cardsPerPlayer = shuffledDeck.size()/numberOfPlayers; // to be subtracted by 1 after dealing remaining cards
    int remainingCards = shuffledDeck.size()%numberOfPlayers;

    Collections.shuffle(shuffledDeck);

    for (int player = 0; player < numberOfPlayers; ++player)
    {
        if (player == remainingCards)
        {
            --cardsPerPlayer; // all remaining cards dealed
        }

        CardDeck hand = new CardDeck(shuffledDeck.subList(0, cardsPerPlayer + 1));
        shuffledDeck.removeAll(hand.cards);

        playerHands.add(hand);
    }

    return playerHands;
}

The full code now looks like this:

Card.java did not change, see above

CardDeck.java

import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;

public class CardDeck
{
    private ArrayList<Card> cards;

    public static void main(String[] args)
    {
        CardDeck deck = new CardDeck();

        ArrayList<CardDeck> playerHands = deck.dealAllCards(5);

        for(CardDeck playerHand : playerHands)
        {
            System.out.println(playerHand);
        }
    }

    CardDeck()
    {
        cards = new ArrayList<>();

        for(Card.Suit s: Card.Suit.values())
        {
            for(Card.Rank r: Card.Rank.values())
            {
                cards.add(new Card(r,s));
            }
        }
    }

    CardDeck(Collection<? extends Card> cards)
    {
        this.cards = new ArrayList<Card>(cards);
    }

    public ArrayList<CardDeck> dealAllCards(int numberOfPlayers)
    {
        ArrayList<CardDeck> playerHands = new ArrayList<>(numberOfPlayers);
        ArrayList<Card> shuffledDeck = new ArrayList<>(cards);

        int cardsPerPlayer = shuffledDeck.size()/numberOfPlayers; // to be subtracted by 1 after dealing remaining cards
        int remainingCards = shuffledDeck.size()%numberOfPlayers;

        Collections.shuffle(shuffledDeck);

        for (int player = 0; player < numberOfPlayers; ++player)
        {
            if (player == remainingCards)
            {
                --cardsPerPlayer; // all remaining cards dealed
            }

            CardDeck hand = new CardDeck(shuffledDeck.subList(0, cardsPerPlayer + 1));
            shuffledDeck.removeAll(hand.cards);

            playerHands.add(hand);
        }

        return playerHands;
    }

    public void shuffle()
    {
        Collections.shuffle(cards);
    }

    @Override
    public String toString()
    {
        String result = cards.size() + " cards:" + System.lineSeparator();
        for (Card card : cards)
        {
            result = result.concat(card + System.lineSeparator());
        }

        return result;
    }
}

full example output:

$ java CardDeck
11 cards:
rank: FOUR   suit: Clubs
rank: FIVE   suit: Heart
rank: FOUR   suit: Spade
rank: ACE    suit: Heart
rank: EIGHT  suit: Heart
rank: SIX    suit: Heart
rank: JACK   suit: Spade
rank: EIGHT  suit: Diamond
rank: JACK   suit: Clubs
rank: QUEEN  suit: Clubs
rank: TWO    suit: Spade

11 cards:
rank: SEVEN  suit: Heart
rank: NINE   suit: Spade
rank: SEVEN  suit: Diamond
rank: SEVEN  suit: Clubs
rank: SIX    suit: Clubs
rank: ACE    suit: Clubs
rank: KING   suit: Heart
rank: SEVEN  suit: Spade
rank: FIVE   suit: Clubs
rank: FOUR   suit: Diamond
rank: TEN    suit: Diamond

10 cards:
rank: KING   suit: Clubs
rank: TEN    suit: Clubs
rank: ACE    suit: Diamond
rank: QUEEN  suit: Spade
rank: SIX    suit: Diamond
rank: FIVE   suit: Spade
rank: EIGHT  suit: Spade
rank: QUEEN  suit: Heart
rank: FOUR   suit: Heart
rank: KING   suit: Diamond

10 cards:
rank: THREE  suit: Clubs
rank: EIGHT  suit: Clubs
rank: THREE  suit: Diamond
rank: KING   suit: Spade
rank: TWO    suit: Clubs
rank: JACK   suit: Heart
rank: THREE  suit: Spade
rank: THREE  suit: Heart
rank: JACK   suit: Diamond
rank: TWO    suit: Diamond

10 cards:
rank: NINE   suit: Heart
rank: NINE   suit: Clubs
rank: TEN    suit: Heart
rank: ACE    suit: Spade
rank: NINE   suit: Diamond
rank: TEN    suit: Spade
rank: FIVE   suit: Diamond
rank: QUEEN  suit: Diamond
rank: TWO    suit: Heart
rank: SIX    suit: Spade

further improvements from the comments

Thanks @Boris!

  String.concat is conventionally written as +; this is never used in a loop - a StringBuilder (or StringWriter) is used instead.

That's right. CardDeck.toString() could look like:

@Override
public String toString()
{
    StringBuilder result = new StringBuilder(cards.size() + " cards:" + System.lineSeparator());
    for (Card card : cards)
    {
        result.append(card + System.lineSeparator());
    }

    return result.toString();
}

This is great, because it also helps readability. It's not just some String, but a StringBuilder, which is a bit more self-documenting.

Given that cards is immutable (no public acces and not private changes), this String could even be built beforehand and cached in a private member. As I don't think the toString() method will be called in such quantities that it would make a significant difference, I leave such and further optimisations of the method to the interested reader.

  Your deal method is very inefficient; List.removeAll will call remove on each of the elements, which is in turn going to loop through the List to find the element; this is O(n*k) - very bad. Instead use List.subList.clear() after you create the CardDeck for the player.

True, I was more concerned about being close to the real world situation that the code is modelling, that is: tacking a number of cards from a set of cards.

How to solve that problem? Not at all. They say it's clever to solve a problem, but wise to avoid it. Let's simply not remove anything at all.

Instead, move the parameters for subList(), which essentially has the same effect. The improved version of dealAllCards() could look like this:

public ArrayList<CardDeck> dealAllCards(int numberOfPlayers)
{
    ArrayList<CardDeck> playerHands = new ArrayList<>(numberOfPlayers);
    ArrayList<Card> shuffledDeck = new ArrayList<>(cards);

    int cardsPerPlayer = shuffledDeck.size()/numberOfPlayers; // to be subtracted by 1 after dealing remaining cards
    int remainingCards = shuffledDeck.size()%numberOfPlayers; // #cards is not a multiple of #players

    Collections.shuffle(shuffledDeck);

    int firstCardIndex = 0;
    int lastCardIndex = 0;

    for (int player = 0; player < numberOfPlayers; ++player)
    {
        if (player == remainingCards)
        {
            --cardsPerPlayer; // all remaining cards dealed
        }

        lastCardIndex = firstCardIndex + cardsPerPlayer + 1;

        playerHands.add(new CardDeck(shuffledDeck.subList(firstCardIndex, lastCardIndex)));

        firstCardIndex = lastCardIndex;
    }

    return playerHands;
}

shuffledDeck is just a local temporary helper to hold the shuffled cards, after all.


Infinite Monkey Theorem Python Comparison I wrote code to demonstrate the Infinite Monkey Theorem which states that if a monkey types at a keyboard for an infinite amount of time it will almost surely type a given text such as William Shakespeare. Here I've replaced the monkey with Python code. I have a sentence and let Python choose from the 26 letters of the alphabet and a space and have it randomly try to generate the chosen sentence.

import string
import random

def genString():
    letters = list(string.ascii_lowercase + " ")
    testList = [random.choice(letters) for letter in letters]
    testString = ''.join(testList)
    return testString

def score(n):
    score = 0
    test = "methinksit is like a weasel"
    if test == n:
        score = 100
    else:
        for letter in range(len(test)):
            if test[letter] == n[letter]:
                score = 1/27 + score
            else:
                score = 0/27 + score
    score = score * 100
    return score

def test():
    count = 0
    localScore = 0
    n = genString()
    guessString = ""
    while(localScore != 100):
        if score(n) > localScore:
            localScore = score(n)
            guessString = n
        if count%1000 == 0:
            print(localScore)
            print(guessString)
            print(count)
        n = genString()
        count = count + 1

However, for the test function I had a previous version which was:

def test2():
    count = 0
    localScore = 0
    n = genString()
    guessString = ""
    testList = [score(n)]
    while(100 not in testList):
        if max(testList) > localScore:
            localScore = max(testList)
            guessString = n
        if count%1000 == 0:
            print(localScore)
            print(guessString)
            print(count)
        n = genString()
        testList.append(score(n))
        count = count + 1

However, after testing it out I realized it was super slow. I'm guessing that the problem is that test2 has a runtime of O(n2) because of the loop and the max function and my previous test has a runtime of O(n). Is this true? Or is there another reason why it's super slow?

Python naming conventions

PEP 8 says that functions and variables should use snake_case so getString should be get_string.

Naming issues

localScore is the best score that you have for all the generations, name it best_score instead. Similarly: guessString -> best_string. I think count is ok. I would have used gen or generation but then again it's not really a genetic algorithm so there isn't really evolution so one could argue count is better than gen...

gen_string

I would say that it is pretty lucky that letter and "methinksit is like a weasel" have the same length. Give gen_string a length parameter. You don't need to convert letters to a list, and you don't need so many temporary variables. (I also prefer keyboard to letters since " " isn't a letter). Ultimately I came up with:

def genString(length):
    keyboard = string.ascii_lowercase + " "
    return ''.join((random.choice(keyboard) for _ in range(length)))

test

Keep your test information in test and don't let it leak out.

test = "methinksit is like a weasel"

Should be somewhere in your test function, not score. Pass a reference string to score.

I would advise printing out more information than just numbers, something like this, so you can tell what is going on:

        print("Iteration: " + str(count))
        print("Best score: " + str(best_score))
        print("Best strint: " + best_string)

score

This:

        else:
            score = 0/27 + score

Is unneeded. Just get rid of it. I believe also:

if test == n:
    score = 100

Is unneeded. (Although, there might be some weird floating point thing I'm not taking into consideration. Regardless, you can adjust for this.)

Don't have:

1/27

What should be in place of 27?

n is often assumed to be an integer, yet you use it as a string.

Organization

test doesn't really feel like a test, it feels like the main point of your application. Maybe just change it to main() and do something like:

if __name__ == "__main__":
    main()

If you think it is a test maybe move it into a testing file like test.py.


Overriding scrolling behavior for navigation elements I am new to JavaScript, and trying to ensure the i write the best possible code instead of "copy & paste".

I hope i am asking correctly, want to know if my code I have written is done correctly or could it be done better, the code works, so nothing wrong with the end result, just not sure if I'm on the right track.

$(document).ready(function() {
"use strict";

var menu_btn = $('a.menu-trigger');
var menu     = $('#main_nav > ul');
var w        = $(window).width();

// Adding the ScrollTo Function on the menu items

$('#main_nav > ul a[href^="#"],.logo a[href^="#"]').on('click', function(e) {

    var target = $( $(this).attr('href') );

    if(target.length ) {

        e.preventDefault();
        $('html, body').animate({
            scrollTop: target.offset().top
        }, 1000);
    }

    if (w < 768) {
        $('#main_nav > ul').css('display', 'none');
    }
});

// Create the Sticky Header

$(window).scroll(function() {
    if ($(this).scrollTop() > 1){
        $('.header-inner').addClass("sticky");
    }else{
        $('.header-inner').removeClass("sticky");
    }
});

// Mobile Menu Functionality

menu_btn.on('click', function(e) {
    e.preventDefault();
    menu.slideToggle ();
});

$(window).resize(function () {

    if (w > 768 && menu.is(':hidden')) {
        menu.removeAttr('style');
    }
});

});

One potential problem I see is that you initialize the w variable once at $(document).ready() time and then you use that value in several event handlers.  But the window width is a dynamic value that can be changed over time so your event handlers can be using a stale value for w.  You should probably be fetching $(window).width() anytime you need it so it is always correct.

You can also replace:

$('#main_nav > ul').css('display', 'none');

with this:

menu.hide();

since you've already cached the value of $('#main_nav > ul') and .hide() is a shortcut for .css('display', 'none');.

You could also probably replace this entire code section:

if (w < 768) {
    $('#main_nav > ul').css('display', 'none');
}

with a CSS rule using media queries.

menu.removeAttr('style'); is pretty harsh (removing all possible styles).  If you're just trying to show the menu again when the width has exceeded a desired value, then you can just do this:

$(window).resize(function () {
    if ($(window).width() > 768) {
        menu.show();
    }
});

Note, there is no need to check .is(':hidden') because calling .show() on something that is already visible just does nothing and .is(':hidden') is a slightly expensive operation because it has to check the visibility of every single parent object.


Dynamically mess images inside a container Well, i'm starting a function to 'mess' with a image gallery, i made an example

i wanna know if the way i'm doing is realy the BEST way, the function work perfect, but i fill that can be better, something more dynamic, like never flow over the edge, and mess with the number of images that user places inside the gallery, now i set as index 0 to 5.

Is there any easy way to improve this little function, make the code faster/smaller then that?

Any tips, be my guest.

jQuery

$("#ran").click(function () {
    mess(1, $(".holder img"));
});

function mess(type, gallery) {
    var getNumImg = gallery.length,
        winWidht = $("#galeria").width(),
        winheight = $("#galeria").height(),
        imgHeight = $("#content-wrapper img").height(),
        ftBottom = winheight - ($(".stats").height() + imgHeight + 150);

    $(".holder").width(winWidht / 2);

    gallery.each(function (index) {
        var randonLeft, randonRight, randonTop, randonTransform, maxIndex;
        var leftFt, topFt, transFt;
        var sign = "- ";
        var signRes = sign.charAt(Math.floor(Math.random() * sign.length));

        if (type == 1) {
            if (index == 0) {
                maxIndex = getNumImg;
                transFt = {
                    "from": 1,
                    "to": 30
                }
                topFt = {
                    "from": 1,
                    "to": ftBottom
                }
            } else if (index == 1) {
                leftFt = {
                    "from": 30,
                    "to": 40
                }
                transFt = {
                    "from": 1,
                    "to": 30
                }
                topFt = {
                    "from": 1,
                    "to": ftBottom
                }
            } else if (index == 2) {
                leftFt = {
                    "from": -30,
                    "to": -40
                }
                transFt = {
                    "from": 1,
                    "to": 30
                }
                topFt = {
                    "from": 1,
                    "to": ftBottom
                }
            } else if (index == 3) {
                leftFt = {
                    "from": -30,
                    "to": -40
                }
                transFt = {
                    "from": 1,
                    "to": 30
                }
                topFt = {
                    "from": 1,
                    "to": ftBottom
                }
            } else if (index == 4) {
                transFt = {
                    "from": 1,
                    "to": 30
                }
                topFt = {
                    "from": 1,
                    "to": ftBottom
                }
            } else if (index == 5) {
                leftFt = {
                    "from": 40,
                    "to": 50
                }
                transFt = {
                    "from": 1,
                    "to": 30
                }
                topFt = {
                    "from": 1,
                    "to": ftBottom
                }
            }
        }

        if (typeof leftFt != "undefined") {
            randonLeft = Math.floor(Math.random() * leftFt.to) + leftFt.from;
        }
        if (typeof transFt != "undefined") {
            randonTransform = Math.floor(Math.random() * transFt.to) + transFt.from;
        }
        if (typeof topFt != "undefined") {
            randonTop = Math.floor(Math.random() * topFt.to) + topFt.from;
        }

        // make it happen
        $(this).css({
            zIndex: maxIndex,
            left: randonLeft,
            top: randonTop,
            transform: "rotate(" + signRes + randonTransform + "deg)"
        });
    });
}

I think your code is good and more importantly it works. The main thing I would change is that huge if else statement. You repeat yourself a lot there. Here's another way you could do it:

Also here's your edited fiddle.

Acho que o seu trabalho está bom e mais importante ele funciona. A parte que chama minha atenção é o if/else enorme. Você se repete muito ali. Outra forma que você pode fazer é assim:

if(type === 1){
    //You had these every time, so you can declare them once here.
    //Você declara isso em todos os casos, mas só precisa fazer uma vez aqui.
    topFt = { "from" : 1, "to" : ftBottom };
    transFt  = { "from" : 1,  "to" : 30 };

    //I Took out all the repetitions and left the cases where we actually change something.
    //Tirei todos os casos em que você estava se repetindo e deixei somente os que realmente estão mudando algo.
    switch(index){
        case 0:
            maxIndex = getNumImg;
            break;
        case 1:
            leftFt   = { "from" : 30, "to" : 40 }
            break;
        case 2: //If index === 2
        case 3: //OR if index === 3
            leftFt   = { "from" : -30,  "to" : -40 }
            break;
        case 5:
            leftFt   = { "from" : 40,  "to" : 50 }
            break;
        //Add more cases if you want/need to.
        //Adicione mais casos aqui se precisar.
        default:
            break;
    }
}


Using Collectors.toList instead of forEach and a new ArrayList I have the following Java 8+ code (on Java 14).  I have a (terminal) forEach call and I am adding each mapped object to a newly created ArrayList.
Is there a more idiomatic way this could be rewritten using Collectors.toList() and potentially flatMap?
public List<SearchSuggestion> suggest() {
   List<Watchlist> watchlists = someService.findAll();
   List<SearchSuggestion> suggestions = new ArrayList<>();

   watchlists.forEach(watchlist -> watchlist.getWatchlistItems()
            .forEach(watchlistItem -> {
                suggestions.add(mapToSearchSuggestion(watchlistItem.getInstrument()));
            })
   );
   return suggestions;
}

private SearchSuggestion mapToSearchSuggestion(Instrument instrument) {
    return SearchSuggestion.builder()
            .symbol(instrument.getSymbol())
            .build();
}

This is one way using flatMap and Collectors.toList():
public List<SearchSuggestion> suggest() {
   return someService.findAll().stream()
            .flatMap(wl->wl.getWatchlistItems().stream())
            .map(wli->mapToSearchSuggestion(wli.getInstrument()))
            .collect(Collectors.toList());
}

However (since this is Code Review) the method mapToSearchSuggestion seems more appropriate to be in the class Instrument. If you can move it there then the function becomes:
public List<SearchSuggestion> suggest() {
    return watchlists.stream()
            .flatMap(wl ->wl.getWatchlistItems().stream())
            .map(WatchlistItem::getInstrument)
            .map(Instrument::mapToSearchSuggestion)
            .collect(Collectors.toList());
}

Instrument class:
public class Instrument {
    //...
    public SearchSuggestion mapToSearchSuggestion() {
        return SearchSuggestion.builder().symbol(getSymbol()).build();
    }
    //...
}


Proper use of class constants This question specifically relates to the use of the class constants ABOVE and BELOW in the sample code below.

I have a few different classes that look like this:

class MyClass(object):
    ABOVE = 1
    BELOW = 0

    def __init__(self):
        self.my_numbers = [1,2,3,4,5]

    def find_thing_in_direction(self, direction, cutoff):
        if direction == self.ABOVE:
            return [n for n in self.my_numbers if n > cutoff]
        else:
            return [n for n in self.my_numbers if n < cutoff]

my_class = MyClass()
my_var = my_class.find_thing_in_direction(MyClass.ABOVE, 3)

If I have a handful of classes scattered across different .py files that each have their own ABOVE and BELOW, should I extract these constants to somewhere, or is it better to keep the constants within their own classes?

Is there a more Pythonic way to do this instead of using these class constants?

I think the answer depends on the scope of your constants.

If you're trying to keep the constants the same across your handful of classes, i.e., you have many class definitions and they all use ABOVE and BELOW, and ABOVE and BELOW always have the same value, then you should pull them out. You could put them into a base class and create subclasses from that base class:

class myBaseClass(object):
    ABOVE = 1
    BELOW = 0

class mySubclass(myBaseClass):
    """ ABOVE and BELOW will be available here """
    pass

If the scope is greater than that, say module-level then put the definitions in the module __init__.py

Greater still?  Then maybe move them to a settings file.

Otherwise if the scope is where you currently have it then it's ok as-is.

The line:

my_var = my_class.find_thing_in_direction(MyClass.ABOVE, 3)

puzzles me because you're referring to the constant of the Class when the instance already has it itself. It's not wrong, just differs from my reaction to use the object's own attributes:

 my_var = my_class.find_thing_in_direction(my_class.ABOVE, 3)

If you go the way of a base class, have a look at ABCs


C++ Snake game using SFML libraby Here is my little snake game which I made in c++ using the SFML library. The game is very simple. I have also included a screenshot of how the GUI looks. The files which I am loading are 30x30 colored squares. Yellow is the background. Red represents the snake. White is the cherry/fruit/prize to increase the size of the snake

#include<SFML/graphics.hpp>
#include<windows.h>

using namespace sf;
struct position{
    int x;
    int y;
}snake[200];

char direction = 'r';
int length = 2;

void update(){
    for(int i = length-1;i > 0;i--){
        snake[i].x = snake[i-1].x;
        snake[i].y = snake[i-1].y;
    }
    if (direction == 'u') snake[0].y-=1;
    else if (direction == 'd') snake[0].y+=1;
    else if (direction == 'r') snake[0].x+=1;
    else if (direction == 'l') snake[0].x-=1;
}
int fruitx = 5;
int fruity = 5;

void genrand(){
    int rx = rand() % 20;
    int ry = rand() % 20;
    fruitx = rx;
    fruity = ry;
}
int main(){
    snake[0].x = 5;
    snake[0].y = 5;
    RenderWindow window(VideoMode(620,620),"Snake c++");
    Texture t;
    Texture t2;
    Texture t3;
    t3.loadFromFile("images/green.png");
    t2.loadFromFile("images/red.png");
    t.loadFromFile("images/yellow.png");
    Sprite bg(t);
    Sprite sn(t2);
    Sprite fruit(t3);
    const int size = 31;
    while(window.isOpen()){
        Event e;
        while(window.pollEvent(e)){
            if(e.type == Event::Closed)
                window.close();

            if(e.type == Event::KeyPressed){
                if(e.key.code == Keyboard::Key::Up && direction != 'd') direction = 'u';
                else if(e.key.code == Keyboard::Key::Down && direction != 'u') direction = 'd';
                else if(e.key.code == Keyboard::Key::Left && direction != 'r') direction = 'l';
                else if(e.key.code == Keyboard::Key::Right && direction != 'l') direction = 'r';
            }
        }
        Sleep(60);
        update();
        window.clear();
        for(int i = 0;i < 20;i++){
            for(int j = 0;j < 20;j++){
                bg.setPosition(i*size,j*size);
                window.draw(bg);
            }
        }

        for(int i = 0;i < length;i++) {
            if (snake[i].x < 0) snake[i].x+=20;
            if (snake[i].x > 19) snake[i].x-=20;
            if (snake[i].y < 0) snake[i].y+=20;
            if (snake[i].y > 19) snake[i].y-=20;

            sn.setPosition(snake[i].x*size,snake[i].y*size);
            if (i != 0){
                if(snake[i].x == snake[0].x && snake[i].y == snake[0].y)
                    length = 2;
            }
            if (snake[i].x == fruitx && snake[i].y == fruity){
                length+=1;
                genrand();
                fruit.setPosition(fruitx*size,fruity*size);
            }
            window.draw(sn);
        }
        window.draw(fruit);
        window.display();
    }

    return 0;
}

The snake is represented by an array of position objects.
When I detect any collision I just reset the length of the snake.

Use correct capitalization of file names
Be careful with upper case and lower case in filenames. While Windows might not treat filenames case sensitively, other operating systems might. The correct #include is:
#include <SFML/Graphics.hpp>

Avoid operating specific functions
You use Sleep(), which is not a standard C function, but rather Windows specific. Since you are using SFML, you can use sf::sleep() instead, like so:
sf::sleep(sf::milliseconds(60));

You can then also remove #include <windows.h>, and then your code compiles and runs on other platforms without errors.
Add error checking
Your code ignores errors trying to open the texture files. Make sure you report an error and exit if any of the required files cannot be found.
Split off more functionality into their own functions
You already created a function update() to handle moving the snake one frame. However, it would be nice to move more functionality out of main() into their own functions, so that the game loop in main() becomes something simple like:
while (window.isOpen()) {
    handle_input();
    update();
    draw();
    sleep(milliseconds(60));
}

Avoid moving the whole body each frame
Instead of moving the position of every element of snake[], use a std::deque to hold the positions, and inside update(), just pop the tail and add a new head:
#include <deque>

struct position {
    int x;
    int y;
};

std::deque<position> snake;

void update() {
    // Calculate the new head position
    position new_head = snake.front();
    if (direction == 'u') new_head.y--;
    else if ...;

    // Handle wraparounds here
    if (new_head.x < 0) new_head.x += ...;
    if (...) ...;

    // Remove the tail and add a new head
    snake.pop_back();
    snake.push_front(new_head);
}

Using std::deque, the container knows its own length, and this makes it easy to use range-for loops. For example, to draw the snake:
for (auto position: snake) {
    sn.setPosition(position.x * size, position.y * size);
    window.draw(sn);
}

Use struct position for all positions
You can also use struct position to store the coordinates of the fruit:
position fruit;

This makes the code more consistent, and if position would have added comparison operators, it would save you from manually comparing the x and y coordinates separately. Which brings me to:
Use sf::Vector2<int> for the positions
SFML has a handy class to store coordinates for you, so you can write:
std::deque<sf::Vector2<int>> snake;
sf::Vector2<int> fruit;

This type also has x and y member variables. But, it also overloads things like operator=(), so instead of comparing x and y coordinates manually, you can write:
if (snake.front() == fruit) {
    // Handle the snake eating the fruit
}

Code formatting
Your code has inconsistent formatting, sometimes there are spaces between operators and around braces, sometimes not. It doesn't really matter that much which code style you use, as long as you are consistent. Instead of manually fixing style issues, consider using a code formatting tool (either an external one like Artistic Style, or your editor's built-in code formatting functions) to keep your code looking tidy.


Parse a CSV file and return an object or array I recently published a JavaScript module for NPM which is also available on Github by the name of rawiki-parse-csv. The code takes raw CSV data and returns an array. The method has one option to include the first line as a header in which case an object is returned within the array. If the option is not set each line will be returned as an array.

The project includes a Chai test which inputs some sample CSV data and checks the returned output with both options specified. The tests pass without issue and use in an actual application appears to be faultless.

Could someone comment on any issues with this code, potentials for failure, incorrect structure or areas for improvement?

/*
The MIT License (MIT)
Copyright (c) 2016 Rawikitua Isherwood
Parse module creates an object or set of arrays from a csv file by splitting the text at each new
line and splitting each line at comma marks.*/

var parse = function (input, option){
    try
    {
        // output object
        var data = {},
        // output no columns array
        container = [],
        // output array
        records = [],
        // splits csv data at each new line
        lines =input.split(/\r\n|\r|\n/),
        // creates columns by splitting first line of csv
        columns = lines[0].split(',');
        // creates objects from csv data if column option included
        if (option === true){

            // loop through each line of csv file
            for (var l = 1; l <= lines.length-1; l++)
            {
                // splits each line of csv by comma
                var words = lines[l].split(',');

                // builds object based on column headers
                for (var cell in columns)
                {
                    data[columns[cell]] = words[cell];
                }
                // pushes object to output array
                records.push(data);
                // resets object in order to add another
                data = {};
            }
        }
        else {
            // creates nested arrays from csv data if column option omitted,false or not true
            for (var l = 0; l <= lines.length-1; l++)
            {
                // splits each line of csv by comma
                var words = lines[l].split(',');
                // creates objects from csv data
                for (var cell in words)
                {
                    container.push(words[cell]);
                }
                // push array to output array
                records.push(container);
                // reset array in order to add another
                container = [];
            }
        }
        // returns output array
        return records
    }
    catch(err){
        return err
    }
}

module.exports = parse

Concept

CSV is a somewhat loosely defined concept. The closest thing there is to a specification is RFC 4180.

This parser is rather naïve: it doesn't handle quoting, and as a result it cannot return data that contain strings with literal commas. Maybe that is good enough for your own use, but in my opinion it's not good enough for a publicly published library, at least not without a giant disclaimer.

For an example of a good CSV library that handles multiple dialects, take a look at Python's built-in library.

Implementation

The purpose of the option is not self-evident. A better name might be headerRow.  Also, the way you have nearly duplicated the two cases, you might as well write it as two functions instead.

The code crashes on empty input, instead of returning an empty data structure as I would expect.

Why do you catch an exception and convert it to a return value? That defeats the purpose of the exception mechanism, and forces the caller to handle the possibility of very weird "data" resulting from the parsing.

I'm not a fan of var declarations that span multiple lines, since it is easy to unintentionally write something different if you screw up the punctuation. ("use strict" would be a good practice for helping to detect such errors.) Your use is particularly bad because the intervening comment lines and the lack of additional indentation on the subsequent lines make it hard to read correctly.

l <= lines.length - 1 is not idiomatic: the standard way to write a counting for-loop in JavaScript is:

for (var i = 0; i < array.length; i++)

Why do you "reset" your temporary variables at the end of the loop for reuse in the next iteration? Why not just create it at the top of the loop?

Commenting every line of code is annoying. Most of your comments can just be eliminated.

Use semicolons consistently. You missed one at return records.


100DaysofSwiftUI - Project 1 - Challenge 1: Convert temperature units: Celsius, Fahrenheit and Kelvin This is my code to solve the challenge in 100 Days of SwiftUI by Paul Hudson. This is the first challenge in the series and I chose to convert the temperature units from C° -> F°, F° -> C°, C° -> K° and K°-> C°

I need to ask, is this code readable by you and if anything needs to be changed/modified in order to make more efficient?

import SwiftUI

struct ContentView: View {
    @State private var inputTemp = ""
    @State private var inputUnit = 0
    @State private var outputUnit = 0

    let inputUnits = ["C", "F", "K"]
    let outputUnits = ["C", "F", "K"]

    var convertedDegreeC2F: Int {
        guard let inputDegree = Int(inputTemp) else { return 0 }
        let convertC2F = Int(inputDegree/5*9+32)
        let convertF2C = Int((inputDegree-32)*5/9)
        let convertC2K = Double(inputDegree) + 273.15
        let convertK2C = Double(inputDegree) - 273.15

        if inputUnit == 0 && outputUnit == 1 {
           return convertC2F
        } else if inputUnit == 1 && outputUnit == 0 {
            return convertF2C
        } else if inputUnit == 0 && outputUnit == 2 {
            return Int(convertC2K)
        } else if inputUnit == 2 && outputUnit == 0 {
            return Int(convertK2C)
        }

        return 0
    }

    var body: some View {
        NavigationView {
            Form {
                Section {
                    TextField("Input Temperature", text: $inputTemp)
                        .keyboardType(.decimalPad)
                }
                Section(header: Text("Select input unit:" )) {
                    Picker("Input Unit", selection: $inputUnit) {
                        ForEach(0..<inputUnits.count) {
                            Text("\(self.inputUnits[$0])°")
                        }
                    }
                    .pickerStyle(SegmentedPickerStyle())
                }
                Section(header: Text("Select output unit:" )) {
                    Picker("Output Unit", selection: $outputUnit){
                        ForEach(0..<outputUnits.count) {
                            Text("\(self.outputUnits[$0])°")
                        }
                    }
                        .pickerStyle(SegmentedPickerStyle())
                }
                Section(header: Text("Temperature after conversion is:")) {
                    Text("\(convertedDegreeC2F)°")
                }
            }
        .navigationBarTitle("UConvert")
        }
    }
}

The conversion function convertedDegreeC2F() handles only four of the nine possible conversions between the three units. In particular, the conversion between identical input/output unit is not handled. Also the function name is misleading because the function is not about conversion from Celsius to Fahrenheit only.

The conversion between temperature units can be greatly simplified using  Measurement from the Foundation framework:

  A numeric quantity labeled with a unit of measure, with support for unit conversion and unit-aware calculations.

Here is a standalone example for demonstration:

let t = Measurement(value: 10.0, unit: UnitTemperature.celsius)
print(t.converted(to: .fahrenheit).value) // 49.99999999999585
print(t.converted(to: .kelvin).value)     // 283.15

The Fahrenheit value is not “exact” due to floating point rounding issues. But that does not matter, because displaying the value should be done with a formatter anyway. Here we can use a MeasurementFormatter:

  A formatter that provides localized representations of units and measurements.

Again a standalone example for demonstration:

let formatter = MeasurementFormatter()
formatter.unitOptions = .providedUnit

let t = Measurement(value: 10.0, unit: UnitTemperature.celsius)
print(formatter.string(from: t)) //10°C
print(formatter.string(from: t.converted(to: .fahrenheit))) // 50°F
print(formatter.string(from: t.converted(to: .kelvin))) // 283.15 K

The measurement formatter adds the correct unit symbol automatically, and also localizes the output correctly (e.g. “283,15 K” with a comma as fraction separator, where appropriate).

The conversion from the input field value to a floating point number should be done with a NumberFormatter() to handle localized input correctly.

With these preparations, the correct conversion between arbitrary temperature units becomes simple:

func convert(temperature: String, from inputUnit: UnitTemperature, to outputUnit: UnitTemperature) -> String? {
    let inputFormatter = NumberFormatter()
    guard let value = inputFormatter.number(from: temperature) else { return nil }

    let inputTemp = Measurement(value: value.doubleValue, unit: inputUnit)
    let outputTemp = inputTemp.converted(to: outputUnit)

    let outputformatter = MeasurementFormatter()
    outputformatter.unitOptions = .providedUnit
    return outputformatter.string(from: outputTemp)
}

In your ContentView you can then define the list of available units as

let units: [UnitTemperature] = [.celsius, .fahrenheit, .kelvin]

instead of using a list of strings. Both pickers can be created from this list (with the correct symbol for each unit), e.g.

Picker("Input Unit", selection: $inputUnit) {
    ForEach(0..<units.count) {
        Text(self.units[$0].symbol)
    }
}
.pickerStyle(SegmentedPickerStyle())

Finally, converting the input value and displaying it is now simply done with

Text(convert(temperature: inputTemp, from: units[inputUnit],
            to: units[outputUnit]) ?? "Invalid input")

A small performance improvement might be to create both formatters only once, e.g. by storing them as private variables in the compilation unit:

import SwiftUI

fileprivate let inputFormatter = NumberFormatter()
fileprivate let outputformatter: MeasurementFormatter = {
    let fmt = MeasurementFormatter()
    fmt.unitOptions = .providedUnit
    return fmt
}()

func convert(...)
// ...


Read through a log file to find serial numbers based upon prior information found This class takes a log file name(which must be of a specific format/order) and returns a list of serial numbers of those drives which contain particular type of error. It uses regular expressions to process the file line by line and all but one of the regular expressions searches using patterns shaped by previous matches.

I have little experience in object orientated programming and would like to better understand how I might structure code to achieve the below task. In particular, I'm unsure about my methods calling each other in sequence as they do.

import re

class SerialNumberFinder:
    """Finds serial numbers of drives with a corrupt filesytem (XFS ERROR)"""

    def __init__(self, filename):
        self.serial_numbers = []
        self.result = None
        self.match = None
        try:
            with open(filename) as f:
                # reverse order of lines otherwise serial number information
                # for a drive is processed before error type is identified
                self.lines = reversed(f.readlines())
        except FileNotFoundError:
            print("Cannot find file {}".format(self.filename))

    def get_next_line(self):
        """Return next line from self.lines if lines, otherwise return None"""
        try:
            line = next(self.lines)
        except StopIteration:
            return None
        else:
            return line

    def get_serial_numbers(self):
        """Return a list of serial numbers"""
        self.find_error()
        return self.serial_numbers

    def find_error(self):
        """Use a regex to find mention of a specific type of error in a line"""
        while not self.match:
            line = self.get_next_line()
            if not line:
                return
            self.match = re.match("XFS ERROR (\[sd[a-z]\])", line)
        self.result = self.match.groups()[0]
        self.match = None
        self.find_bus()

    def find_bus(self):
        """Find mention of bus using identifier from find_error"""
        while not self.match:
            line = self.get_next_line()
            if not line:
                return
            self.match = re.match("(sd \S+) {}.*".format(re.escape
                                                         (self.result)), line)
        self.result = self.match.groups()[0]
        self.match = None
        self.find_serial()

    def find_serial(self):
        """Find a serial number using identifier from find_bus"""
        while not self.match:
            line = self.get_next_line()
            if not line:
                return
            self.match = re.match("{} \(SERIAL=([^)]*)\)".format(self.result),
                                  line)
        self.serial_numbers.append(self.match.groups()[0])
        self.match = None
        self.find_error()

if __name__ == "__main__":
    s = SerialNumberFinder("EXAMPLE_LOG.log")
    nums = s.get_serial_numbers()
    print(nums)

Output:

['WW11111', 'ZZ12345']

Example log file:

unrelated log messages
sd 0:0:0:0 Attached Disk Drive
unrelated log messages
sd 0:0:0:0 (SERIAL=ZZ12345)
unrelated log messages
sd 0:0:0:0 [sda] Options
unrelated log messages
XFS ERROR [sda]
unrelated log messages
sd 2:0:0:1 Attached Disk Drive
unrelated log messages
sd 2:0:0:1 (SERIAL=ZZ67890)
unrelated log messages
sd 2:0:0:1 [sdb] Options
unrelated log messages
sd 3:0:1:8 Attached Disk Drive
unrelated log messages
sd 3:0:1:8 (SERIAL=WW11111)
unrelated log messages
sd 3:0:1:8 [sdc] Options
unrelated log messages
XFS ERROR [sdc]
unrelated log messages

This looks like a very complex class to me. All you really want to do is read all lines of an error log and keep a dictionary mapping hardware busses to drives, where each drive has a name (like sda), a bus and a serial. If you encounter an xfs error for one of these drives, return its serial number.

This can be accomplished by defining a basically empty Drive class and filling it successively. You don't even need the SerialNumberFinder class, one function is enough:

import collections

class Drive:
    """Object to hold all information about a drive."""
    pass

def get_serial_numbers(file_name):
    """Return a list of serial numbers of drives with XFS errors"""
    drives = collections.defaultdict(Drive)
    try:
        with open(file_name) as f:
            for line in f:
                bus = re.match(r"(sd \S+) (\[sd[a-z]\]).*", line)
                if bus:
                    bus_name, device_name = bus.groups()
                    drives[bus_name].name = device_name
                    continue

                serial = re.match(r"(sd \S+) \(SERIAL=([^)]*)\)", line)
                if serial:
                    bus_name, serial = serial.groups()
                    drives[bus_name].serial = serial
                    continue

                error_disk = re.match(r"XFS ERROR (\[sd[a-z]\])", line)
                if error_disk:
                    device_name, = error_disk.groups()
                    for drive in drives.values():
                        if drive.name == device_name:
                            yield drive.serial
    except FileNotFoundError:
        print("Cannot find file {}".format(file_name))
        raise

if __name__ == "__main__":
    nums = get_serial_numbers("EXAMPLE_LOG.log")
    print(list(nums))

Note that it is often recommended to compile your regexes, i.e do something like:

bus_re = re.compile(r"(sd \S+) (\[sd[a-z]\]).*")
...
bus = bus_re.match(line)

However, Python already does this for you in the background. So whenever a new regex is encountered, it is compiled and this compiled regex is used for all future appearances of this regex. The only time when you should do this is, if you have more than 100 regexes (the default cache size), if you want to take the performance hit of compiling it once way before you run the actual code or if you want to give a regex a nice understandable name.


Puzzle game solver via Backtracking I'm relatively new to Haskell and I'd like to get feedback on the style of my program. Specifically:

Coding Style: Can any parts be written in a more concise or more readable way?
Misuse: Are there any parts that are generally should be avoided, or any typical beginner mistakes? (Basically the do's and don't's)

The game

The program solves a puzzle that is also known as binoxxo: There is a 2n x 2n square grid that is partially filled with X and O. The goal filling each cell of the grid with an X or O such that the result satisfies three conditions:

In each column and each row, the same symbol cannot occur more than twice in a row. (I.e. OXXOXO is ok, however OXXXOO is not.)
Each row and each column have exactly n Xs and n Os.
No two rows are equal and no two columns are equal.

(An online version can be found here.)

The program

The following module consists of three parts: First we define the new type to describe the grid, with the type constructors X,O and finally E (for an empty cell). Then we have two functions: isValid checks whether all three rules hold so far, and backtrack which actually solves the puzzle. It does so by placing an X and O in the first E spot of the given puzzle checking the validity of the new grids, and if that succeeds, recursing a step deeper. This function finds all the possible solutions, but due to the lazy evaluation we can use take 1 $ backtrack myBoard to get only one and finish a little bit quicker.

module Binoxxo (isValid, backtrack, exampleBoard, Cell (..), Board) where

import Control.Applicative
import Data.List

data Cell = E | X | O -- E = empty
  deriving (Eq,Show)

type Board = [[Cell]]

exampleBoard = -- for solving: call "backtrack exampleBoard"
  [[X,X,E,E],
   [E,E,E,E],
   [O,E,E,X],
   [O,O,E,X]]

-- backtracking
backtrack :: Board -> [Board]
backtrack b
  | isFull b = [b] --board has no more empty cells
  | otherwise = nub $ concat $ map backtrack validBoards
  where
    isFull b = not $ E `elem` concat b
    newBoards = generateAllBoards b :: [Board]
    validBoards = filter isValid newBoards

    generateAllBoards :: Board -> [Board] -- adds one new X/O in the position of a E
    generateAllBoards b = concat $ map assembleBoards (prefixRowSuffix b)
      where
        prefixRowSuffix :: [a] -> [([a],a,[a])]-- [1,2,3,4] ->  [([],1,[2,3,4]), ([1],2,[3,4]), ([1,2],3,[4]), ([1,2,3],4,[])]
        prefixRowSuffix b = zip3 (inits b) b (drop 1 $ tails b)

        assembleBoards :: ([[Cell]],[Cell],[[Cell]]) -> [Board]
        assembleBoards (front,m,back) = take 2 -- we only need to place X and O in the first occurence of E, because one of them MUST be correct
          [front ++[f++[x]++b]++ back |
            (f,E,b)<-prefixRowSuffix m,x<-[X,O]]

-- validity check (implement the three rules)
isValid :: Board -> Bool
isValid b = and $
    [and . map checkNeighbours, checkDupli, and . map checkCount] -- each of these get applied to all normal and transposed board
     <*> [rows, cols]
  where
    rows = b :: Board
    cols = transpose b :: Board

    -- we cannot have three consecutive X or O
    checkNeighbours :: [Cell] -> Bool
    checkNeighbours (a:b:c:xs) =
       let this = not $ any ((&& a==b && b==c) . (c==)) [X,O]
           rest = checkNeighbours (b:c:xs)
       in this && rest
    checkNeighbours _  = True

    -- we cannot have two equal rows/columns
    checkDupli :: Board -> Bool
    checkDupli b = check $ filter (all (/=E)) b -- only check full rows for duplicates
      where check (x:xs) =
              (not $ x `elem` xs)  && check xs
            check [] = True

    -- if row is of length n, we can have at most n/2 X and O
    checkCount :: [Cell] -> Bool
    checkCount xs = notTooMany O && notTooMany X
      where
        len = length xs
        notTooMany xo = len >= 2 * length (filter (==xo) xs)

isFull fires when assembleBoards returns an empty list; we'd expect to be able to ask that question only once. newBoards and validBoards do not deserve names - if you want the reader to be able to tell what the value means, comments are more appropriate.

Most of the rest of backtrack is about descending into a nested data structure and changing a small part, which lens specializes in: traverse . traverse . filtered (==E) descends into the board, then each of its elements, then each of their E cells. holesOf gives you, roughly speaking, the positions of the targets in the original board - it separates the board into a Cell and a Cell -> Board for each target. peek lets you forget there was an E.

import Control.Comonad.Representable.Store (peek)
import Control.Lens (holesOf, filtered)

backtrack :: Board -> [Board]
backtrack b = case setFirstE b of
  -- board has no more empty cells
  Nothing -> [b]
  -- we only need to place X and O in the first occurence of E, because one of them MUST be correct
  Just setter -> nub $ concatMap backtrack $ filter isValid $ map setter [X,O]

-- Punches the first E out of the board, if any.
setFirstE :: Board -> Maybe (Cell -> Board)
setFirstE = listToMaybe . map (flip peek) . holesOf (traverse . traverse . filtered (==E))


A pattern to destructively extract items from an array I want to efficiently (few intermediate objects) and neatly (few lines and easy to read) filter an array, removing and returning rejected items. So like a call to delete_if except instead of returning the remaining items, returning the deleted items.

Here's some code that works:

ary = (0..9).to_a
odds = ary.select {|i| i%2==1 }
ary -= odds

or:

ary = (0..9).to_a
(_,ary),(_,odds) = ary.group_by {|i| i.odd? }.sort {|a,b| a[0] ? 1 : -1 }

But I can't help but think there should be a better way...

Ideally something more like: odds = ary.delete_and_return(&:odd?)

Array.partition returns an array of two arrays - one where the elements returned true to the condition, and one that returned false.

So, a single liner for your need would be:

odds, ary = (0..9).to_a.partition(&:odd?)
=> [[1, 3, 5, 7, 9], [0, 2, 4, 6, 8]]

(&:odd?) acts exactly like writing { |x| x.odd? } which will return true if the number is, well, odd...


A small extension to the reCaptcha library I have been writing a small extension to the reCaptcha library.

class Captcha
{
    private $config;

    public $error;

    public function __construct(array $config)
    {
        include './library/recaptchalib.php';
        $this->config = $config;
    }

    public function render($error = null, $use_ssl = false)
    {
        return recaptcha_get_html($this->config['recaptcha']['public_key'],
            $error, $use_ssl);
    }

    public function check()
    {
        $resp = recaptcha_check_answer ($this->config['recaptcha']['private_key'],
                                    $_SERVER["REMOTE_ADDR"],
                                    $_POST["recaptcha_challenge_field"],
                                    $_POST["recaptcha_response_field"]);
        if (!$resp->is_valid) {
            $this->error = $resp->error;
            return false;
        }
    }
}

What do you think? Is it anything to have? Anything I can improve/change?

The $error field should be private (Why use getters and setters?). Currently users of the Captcha class can modify the value of the $error field directly which. It's not a good idea to allow them to do this.
What's the point of returning with false in the check function when the response is not valid while if it's valid the function does not return with true?
The render and check functions seems somehow temporarily coupled. See: Clean Code (by Robert C. Martin), G31: Hidden Temporal Couplings

References:

Effective Java Second Edition, Item 38: Check parameters for validity (I know that this is a Java book but this chapter applicable to PHP too.)
Function/method argument validation standards


Function that strips trailing '0's from a float I'm writing an embedded application in C, and at one point I need to convert a float into an ASCII representation of that float, so I can send the ASCII over a serial port. The protocol the serial port is listening over doesn't like trailing
'0's, so I'm writing a function that will remove all of the unnecessary '0's at the end of the ASCII string. Here is what my expected inputs and outputs look like:

#input           #output
300.000      --> 300.
300.01400    --> 300.014
300.1200     --> 300.12
300.12345678 --> 300.12345   #Only 5 decimal places of precision

Here is the function:

int float_to_str(float f, char *output, int buffer_size)
{
    int buf_len = snprintf(output, buffer_size, "%0.5f", f);

    //Strip trailing '0's from response
    bool seen_point = false;
    int real_len = 0;
    int index = 0;
    while (index < buf_len && index < buffer_size)
    {
        if (output[index] == '.')
            seen_point = true;
        if (output[index] != '0' || !seen_point)
            real_len = index + 1;
        index++;
    }

    return real_len;
}

The return value is the length of the string stuffed into output, which will never be more than buffer_size.

Is this well written and easy to understand? Am I overlooking edge-cases? How about the choice of algorithm, is there a simpler way to do this that I'm overlooking?

My thoughts are as follows:

Since you are leveraging snprintf, you have already limited the string size to buffer_size, so the additional condition in the while loop should not be needed.
Since there is no validation, I'm assuming the data has been validated before and that the value will never exceed buffer_size with, which allows for the following simplification:

int float_to_str(float f, char *output, int buffer_size)
{
    int buf_len = snprintf(output, buffer_size, "%0.5f", f);
    if(buf_len>buffer_size) return -1 ; // updated to add validation
    //Strip trailing '0's from response
    while (buf_len>0)
    {
        if (output[buf_len-1] == '0') // is the last char a zero?
        {
            output[buf_len-1]=0;  // null terminate the string
            buf_len--;
        }
        else break;  // if this is not a zero, we are either at the . or a non zero digit
    }
    return buf_len;
}

I'm not sure your code will return the correct result if the input is 300.00014, my initial thought is your output would be 300.00.


Resharper: Unit of measure is redundant Resharper complain if you have code like this:

border-width: 0px 1px 1px 0px;

and insists that units should be removed from zeroes as they are redundant. They are redundant, that is absolutely true, but I don't like inconsistency that is introduced by removal of units. You get code like this

border-width: 0 1px 1px 0;

which is pretty hideous in my opinion. So, what do you guys think? Should I remove redundant units, or should I disable this warning?

0cm, 0px or 0x corresponding to 0, it doesn't need unit if it's of the same order of magnitude. 0px * 10cm is the same than 0 * 10cm.
In this case border-width corresponding to length's order of magnitude it's not necessary to precise the units.

Moreover, for my part, I never specifies the things which are default. Whatever the language. I think it's more readable and elegant. It's my opinion because some people I met prefers specify everything.

I hope my english is not so bad .


Brute-force perfect-number algorithm I whipped a reasonably fast brute-force perfect number finding function in Haskell, after seeing a similar thing in mathematica.

It works in under 3 seconds when I search up to 1000, but 10000 is too much.

isDivisor :: Integral a => a -> a -> Bool
isDivisor n a = mod n a == 0

sumDivisors :: Integral a => a -> a
sumDivisors a =
   foldr (\n -> if isDivisor a n then (+ n) else id) 0 [1..(a `div` 2)]

isPerfect :: Integral a => a -> Bool
isPerfect n = n == sumDivisors n

-- The main prime-finding function:
upUntil :: Integral a => a -> [a]
upUntil n = filter isPerfect [1..n]

It certainly isn't the fastest, or the most elegant, but I'm unsure how I can improve it.

I would like to concentrate on the isDivisor function in particular. Is there a more efficient way of checking for divisibility?

Use backticks

Backticks allow you to put the function name between the values instead of before:

isDivisor n a = mod n a == 0

becomes:

isDivisor n a = n `mod` a == 0

Give less general names

The name upUntil tells nothing about perfect numbers, in a tiny script like this this is no problem, but the habit of meningful names will play yo your advantage in  larger scripts

Do not write wrong comments

Comments don't run, but you are not justified to write wrong comments

-- The main prime-finding function:

The function does not find primes.


jQuery array of functions validation I am attempting to validate a set of business rules in order for a user to successfully add to cart. The rules vary from requiting to add/decline a requested add-on to not having more then 60 items in your shopping cart.

For my end goal, I need to be able to have several dozen rule sets and if any return false then execute the needed function and prevent cart submit.

Separation of the rules logic seemed like a great thing so that I do not clutter my plugin or ajax call it's self, but I wonder if there is a better way because I tend to over engineer things.

JS Fiddle example

Here is the basic object:

var req = {
    type: function(name){
        switch(typeof name){
            case "function":
                return "function";
            break;
            case "object":
                if($.isArray(name)){
                    return "array";
                }else{
                    return "object";
                };
            break;
            case "string":
                return "string";
            break;
            case "number":
                return "number";
            break;
            case '':
            case "undefined":
            default:
                return "undefined";
            break;
        };
    },
    check: function(v){
        var valid = new Array();

        if(this.type(v) === "array"){
            if(v.length > 0){
                for(var i = 0; i < v.length; i++){
                    if(this.type(v[i]) === "function"){
                        valid.push(v[i]());
                    };
                };
            }else{
                return true;
            };
        }else{
            if(this.type(v) === "function"){
                valid.push(v());
            };
        };

        if(valid.indexOf(false) > -1){
            return false;
        }else{
            return true;
        };
    }
};

Here is how the basic functions are pushed into the array:

var addHelper = {
    invoke: function(type, fun){
        gb[type].push(fun);
    }
};

addHelper.invoke("validate",
    function(){
        if(parseInt($("#fakeQty").val()) > 59){
            var qtyMsg = "For bulk or commercial orders, please call our online support";
            alert(qtyMsg);
            return false;
        }else{
            return true;
        };
    }
);

Each function is stored in its own object, but for space sake I just simple passed it directly.

The type function is definitely overengineered in this case because you only need two values. The entire first snippet boils down to a simple function and the logic is instantly observable.

var req = {
    check: function(v){
        if (typeof v === "function") {
            return e();
        }
        if (typeof v === "array" || $.isArray(v)) {
            return v.every(function(e) {
                return typeof e !== "function" || e();
            });
        }
        return true;
    }
}

Note, there's no need for semicolons after closing curly braces of if, for, so if anything it shows you don't understand the automatic semicolon insertion rules.


Extract a character at particular place in a string I have a function:

Private Function getbyte(s As String, ByVal place As Integer) As String
If place < Len(s) Then
place = place + 1
getbyte = Mid(s, place, 1)
Else
getbyte = ""
End If
End Function

For extracting a character at a particular place in a string, how can I shorten this or any alternate for the same purpose?

According to MSDN, this function is already implemented:

' Code from MSDN
'
Dim myString As String = "ABCDE"
Dim myChar As Char
' The value of myChar is "D".
myChar = myString.Chars(3)

A review of your code

Please indent your code.
I would choose another, more meaningful name for your function: getCharAt or getByteFromString. It is now clear that the function operates on a string.
I would use the clearer parameter name str instead of s.
"Compress":

place = place + 1
getbyte = Mid(s, place, 1)

' change to
getbyte = Mid(s, place + 1, 1)

The first version is definitely not wrong or ugly, but why use an extra line if we could express the same logic in the same clear manner?
You could use If to shorten your code:

Private Function getbyte(str As String, ByVal place As Integer) As String
  getbyte = If(place < Len(str), Mid(str, place + 1, 1), "")
End Function


Checking if variable is defined before accessing its properties I didn't write this but I noticed it in our codebase at work.

var data = object && object.attribute && object.attribute.data ? object.attribute.data : {};

I get that the programmer is trying to check that the variable object.attribute.datais defined before using it, but is seems like so much repeated code. What's the best way to assign data the value of object.attribute.data if it's defined, otherwise giving it a value of {}?

Are you looking for this?

var data = object && object.attribute && object.attribute.data || {};

Alternatively, you could create a simple function to safely index objects that may not be defined, such as:

function resolve(value) {
    return function(key) {
        return key ? resolve((value || {})[key]) : value;
    };
}

You'd use it like this:

var data = resolve(object)('attribute')('data')() || {};

If anything in the chain does not exist, it will return undefined. This might be useful if you have deeply nested and unpredictable objects, such as an object resulting from parsing JSON data from a remote resource.

http://jsfiddle.net/VHcdq/1/


Algorithm to find substring in a string Can this be improved?

static int find(string term, string text)
    {
        int found = -1;
        int termIndex = 0;

        for (int textIndex = 0; textIndex < text.Length; textIndex++)
        {
            if (term[termIndex] == text[textIndex])
            {
                if (termIndex == term.Length-1)
                    return found;
                if (termIndex == 0)
                    found = textIndex;
                termIndex++;
            }else
            {
                termIndex = 0;
                found = -1;
            }
        }
        return found;
    }

I also don't code in C#, but if I'm right the following is the same in Java:

public static int find(final String term, final String text) {
    int found = -1;
    int termIndex = 0;

    for (int textIndex = 0; textIndex < text.length(); textIndex++) {
        if (term.charAt(termIndex) == text.charAt(textIndex)) {
            if (termIndex == term.length() - 1) {
                return found;
            }
            if (termIndex == 0) {
                found = textIndex;
            }
            termIndex++;
        } else {
            termIndex = 0;
            found = -1;
        }
    }
    return found;
}

Unfortunately, it does not seem to work. Here are some test cases:

assertEquals("#0", 3, find("de", "abcde")); // OK
assertEquals("#1", 1, find("a", "ababaa")); // fails, returns -1
assertEquals("#2", 1, find("ba", "ababaa")); // OK
assertEquals("#3", 2, find("abaa", "ababaa")); // fails, returns -1
assertEquals("#4", 2, find("abaa", "ababaacc")); // fails, returns -1


Selecting a table with multiple ID on MySQL using PHP PDO I have made a MySQL script to query a table with a set of id using an IN clause and after some reading I found out that there are security issues with it like prone to SQL injections and the likes.

Are there other ways to write this code with a more secure way? Although I'm using PDO, is that really enough?

 public function GetAllCustomerFilter($search, $cat_id)
 {
   $stmt = $this->db->prepare("SELECT * from users_cards WHERE cat_id IN ($cat_id) AND username LIKE :search ORDER BY id DESC LIMIT 60";
   $stmt->bindParam(":search", $search);
     if($stmt->execute()) {
       $products =  $stmt->fetchAll(PDO::FETCH_ASSOC);
     }
 }

Data is received from an Ajax:

    loadProductsListPerCat = function(form,data_serve) {

    var data_fed = new Array();
    $.each(data_serve, function(i,obj)
    {
        data_fed[i] = obj.cat_id;
    });
    var raw = data_fed.join();
    var encoded = encodeURIComponent(raw);
    var formData =  "data_serve="+encoded+"&"+ $(form).serialize();
    var requestURL = 'searchUsers';
    console.log(formData);
    $.ajax({
        url: requestURL,
        data: formData,
        crossDomain: true,
        dataType: 'json',
        cached: false,
        async: true,
        success: function(data) {
            console.log(data);

        },
        complete: function() {
            if(isAppend) {
                $('.btn-toggle-load-more-users').attr('disabled', false);
            }
        },
        error: function(XMLHttpRequest, textStatus, errorThrown) {

        }
    });
};

The variable data_serve is taken by another jQuery function on which it counts input elements that has data-on set to true:

var PCatVal = (function(){
    var cats = new Array();

    $("#main_cat").find(".option_").each(function(index) {
        $(this).removeClass("option_");
        $(this).addClass("ctrls option_"+index);

        $(".option_"+index).on("click", function(e){
            var opt = $(this).data('parentname');
            var opt_id = $(this).val();
            var data_serve = [];
            //array check if exists
            var idx = $.inArray(opt_id, cats);
                if (idx == -1) {
                   cats.push(opt_id);
                } else {
                    cats.splice(idx, 1);
                }
           //loop the array so to encode to a json format
            $.each(cats, function(i,obj){
              data_serve.push({"cat_id": obj});
            });
            //Switch for the category buttons
            var swtch_var = $(this).attr('data-on');

                if(swtch_var == 0)
                {
                  swtch_var = 1;
                }
                else
                {
                  swtch_var = 0;
                }
                $(this).attr('data-on',swtch_var);

loadUsers($('#frmSearchUsers'),'',data_serve);

           e.preventDefault();
        });
    });
});

This is how the $_GET parameters are received:

    public function searchUser()
    {
     $cat_id = (isset($_GET['data_serve']) && !empty($_GET['data_serve'])) ? $_GET['data_serve'] : null;
$search_filter = (isset($_GET['search_filter']) && !empty($_GET['search_filter'])) ? $_GET['search_filter'] : null;

     $products = $this->model->GetAllCustomerFilter($search_filter, $cat_id);
    return $products;
    }

When I changed from the mysqli_functions to PDO with prepared statements, I had the same question. I found the answers to this question to be very usefull: https://stackoverflow.com/questions/134099/are-pdo-prepared-statements-sufficient-to-prevent-sql-injection.

Let's say you have messaging functionality and someone types SQL injection code in the title field when sending a message. You use a prepared statement, and the SQL injection code gets saved as the title. the SQL injection code didn't get executed, you have been saved from 1st order SQL injection.

Now let's say you have a function to reply to messages that uses the previous message title and adds "RE: " before it. If this reply function assumes the new title is safe since it's not coming from a user input field and doesn't use a prepared statement on the reply title, the SQL injection code will get executed. This is called 2nd order SQL injection.

You're 1st order safe if you use prepared statements for queries requiring user input. You're 2nd order safe if you use prepared statements for every query.


Optimal code without using for, while and if I presented a code solution to a startup as part of their interview polity.

The problem statement is to find the frequency of words occurring in a sentence.  I got the code rejected saying, "Evaluating code is always subjective, but people generally leave a "fingerprint" with their design approach and the nature of their code (comments, tests, multiple solution paths, validation). The hiring manager has a high bar, and has rejected many working solutions because he didn't like the approach or the number of trees used or linear scaling vs. ^2 scaling."

I have pasted my solution.  The hiring manager specified not to use collection API.  I would like some opinions as to how I would have improved the code.

import java.io.*;
import java.util.*;

class FreqWord {
    public static void main(String[] args) throws IOException {
        BufferedReader br = new BufferedReader(new
        InputStreamReader(System. in ));
        String str[] = new String[30];
        StringTokenizer st;
        int i = 0, j, k, ctr, size = 0;
        System.out.println(" Enter the String :");
        String str1 = br.readLine();
        st = new StringTokenizer(str1, " \n");
        while (st.hasMoreTokens()) {
            str[i] = st.nextToken();
            size++;
            i++;
        }
        System.out.println("\n-------------");
        System.out.println("\nWord" + "\t" + "freq");
        System.out.println("\n-------------");
        for (i = 0; i < size; i++) {
            ctr = 0;
            for (j = i; j < size; j++) {
                if (str[j].equals(str[i])) ctr++;
            }
            for (k = i - 1; k >= 0; k--) {
                if (str[k].equals(str[i])) {
                    ctr = 0;
                    break;
                }
            }
            if (ctr != 0) {
                System.out.println(str[i] + "\t" + ctr);
            }
        }
        System.out.println("\n------------");
    }
}

I am not a fan of this type of interview process... but ... you can assume that you need to differentiate yourself from the other candidates in some way.

Your solution may work, but does not show much in the way of 'clever' algorithms. You do the minimum to get the answer right, but not in the best way.

Things I think you could do better are:

Do not load the entire document in to memory. It may be large. Instead, use a mechanism for counting the words in a streaming way. It will save memory.
What about handling mixed-case words "The quick brown fox jumped over the sleeping dog" has the word 'the' twice, note the words 'The' and 'the' once each.
Consider a faster method for processing the data than a linnear scan through the words. A binary search and adding data to a sorted array would differentiate you.

EDIT: I took the liberty of copying and running the code, and it does not work. I changed the code to be:

BufferedReader br = new BufferedReader(new
    InputStreamReader(new FileInputStream(new File("src/FreqWord.java"))));

and this produced the result:

 Enter the String :

-------------

Word    freq

-------------
import  1
java.io.*;  1

------------

This is obviously not correct, so I will take a moment and write what I consider to be a 'good' solution. This will also answer the comment below about the Binary Search.

EDIT 2: Here's an answer that demonstrates the three issues I mentioned (just the main method), and as a bonus it gets the right answer.... :

public static void main(String[] args) throws IOException {
    // show you know the Java7 language changes...
    try (BufferedReader br = new BufferedReader(new InputStreamReader(
            new FileInputStream(new File("src/FreqWord.java"))))) {
        int wordcount = 0; // the number of unique words we have

        String [] wordvalues = new String[32];
        int [] counts = new int[32];

        String line = null;
        // Using regular expressions is a differentiator
        final Pattern pattern = Pattern.compile("\\w+");
        while ((line = br.readLine()) != null) {
            // Only keep one line at a time in memory.
            Matcher matcher = pattern.matcher(line);
            while (matcher.find()) {
                // use toLowerCase() to show a grasp of the problem
                String word = matcher.group().toLowerCase();
                // use BinarySearch to order the words, which is faster than a scan.
                int ip = Arrays.binarySearch(wordvalues, 0, wordcount, word);
                if (ip < 0) {
                    // word we have not yet seen.
                    ip = -ip - 1;
                    if (wordcount == wordvalues.length) {
                        // grow the word array by about 25% each time we need to.
                        wordvalues = Arrays.copyOf(wordvalues, wordcount + 1 + (wordcount >>>2));
                        counts = Arrays.copyOf(counts, wordvalues.length);
                    }
                    // insert the word in sorted order.
                    System.arraycopy(wordvalues,  ip, wordvalues,  ip+1, wordcount-ip);
                    System.arraycopy(counts, ip, counts, ip+1, wordcount-ip);
                    wordvalues[ip] = word;
                    wordcount++;
                }
                counts[ip]++;
            }
        }
        // output the words in alphabetical order.
        System.out.println("\n-------------");
        System.out.println("\nWord" + "\t" + "freq");
        System.out.println("\n-------------");
        for (int i = 0; i < wordcount; i++) {
            System.out.println(wordvalues[i] + "\t" + counts[i]);
        }
        System.out.println("\n------------");
    }
}


Sum of all paths between all pairs of nodes in a tree I'm trying to solve a competitive programming problem. It basically gives a undirected graph (tree-like: no multiple paths between two nodes...) and asks for the sum of all possible paths between any pair of nodes in the graph  (each path must be counted only once, in other words, if you have already counted the path from A to B, you shouldn't count the path from B to A).
To solve this, I've tried to remove each edge (separating the tree in 2 components) and count the number of vertices in one of the components (the count of nodes of the second component can be derived from the count found in the first).
The total weight that a edge will contribute is: (the number of paths that pass by it = first_component_size * second_component_size) * (the weight of the edge).
Input description:

The input is composed of several instances (the number of instances is given on the first line of input).
The first input row of each instance contains an integer N (1 ≤ N ≤ 10000), representing the number of nodes. The nodes are enumerated from 1 to N.
Each one of the following N-1 rows contains three integers A, B and C (1 ≤ A, B ≤ N, 1 ≤ C ≤ 50), indicating that the edge that connects the nodes A and B has length C.

Output description:

For each instance print the sum of lenghts of the paths that connects all the pairs of nodes. The answer must be in MOD 1300031.

#include <cstdio>
#include <vector>
#include <utility>

const int MOD = 1300031;

using edge = std::pair<int, int>;
using adj_list   = std::vector<edge>;
using graph  = std::vector<adj_list> ;

struct myEdge {
    int from, to, weight;
};

std::vector<bool> visited;
int count_nodes_on_component(graph &g, int v) {
    int count_of_nodes = 1; //1 for itself
    adj_list &adj = g[v];
    for(auto &e : adj) {
        if(!visited[e.first]) {
            visited[e.first] = true;
            count_of_nodes += count_nodes_on_component(g, e.first);
            visited[e.first] = false;
        }
    }
    return count_of_nodes;
}

int main ( void ) {
    int test_cases;
    scanf("%d", &test_cases);
    while (test_cases--) {
        int total_nodes_in_graph;
        scanf("%d", &total_nodes_in_graph);

        visited.resize(total_nodes_in_graph + 1);

        graph g(total_nodes_in_graph + 1);
        std::vector<myEdge> vector_all_edges;
        for (int i = 1, node1, node2, weight; i < total_nodes_in_graph; ++i) {
            scanf("%d %d %d", &node1, &node2, &weight);
            g[node1].push_back( {node2, weight} );
            g[node2].push_back( {node1, weight} );
            vector_all_edges.push_back( {node1, node2, weight} );
        }

        long long total_sum = 0;
        for (auto &e : vector_all_edges) {
            visited[e.from] = visited[e.to] = true;
            int cc = count_nodes_on_component(g, e.to);
            total_sum = (total_sum + cc * (total_nodes_in_graph - cc) * e.weight) % MOD;
            visited[e.from] = visited[e.to] = false;
        }

        printf("%lld\n", total_sum);
    }
}

This code takes a lot of time with huge graphs. Is it possible to exploit something to make it run faster?

\$O(n^2)\$ time complexity

Your current algorithm has \$O(n^2)\$ time complexity because for each edge, you count the number of vertices on one side of the edge, which is a linear time operation.

However, the program is pretty fast.  I ran it on three large test cases.  The first was 10000 nodes arranged in a line, the second was 10000 nodes arranged in a star, and the third was 10000 nodes arranged in a star, but with the edges listed with reverse direction.  Here were the results:

Line : 1.15 sec
Star : 0.01 sec
Star2: 2.30 sec

The first star was quick because the program always counted the outside (i.e. 1 node always).  The second star was slow because the program always counted the inside (n-1 nodes always).  So you could consider these the best and worst possible test cases.

Alternative strategy

The problem can be solved in \$O(n)\$ time.  Instead of picking a random edge and having to count one side, if you picked an edge connected to a leaf node, you automatically know that the count on the leaf side is 1.  There is always at least 1 leaf node in the graph, because the graph is a spanning tree with no cycles.  If you then remove the leaf node from the graph, there must still be at least 1 leaf node, so you can keep finding a leaf node and removing it until there are no nodes left.

A small complication arises after you find the first leaf node and remove it from the tree.  The node it was attached to must now remember that it was connected to the leaf, in order to keep the distance calculations correct.  In other words, if leaf A is attached to node B and leaf A is removed, then node B must now count as 2 nodes.  If node B is later removed, it must add its 2 nodes to some other node, etc.

So the pseudocode is this:

Find a node with 1 edge only.  Call this node A and the node it is attached to B.
Add to the total distance this amount: A.count * (totalNodes - A.count) * edge.weight.
Add A.count to B.count.
Remove the edge from A and B.
Go back to step 1.  If B only has 1 edge, use B as the next node.

You can find all the leaf nodes in \$O(n)\$ time because:

In the beginning there are a fixed number of leaf nodes.
If you scan the nodes array from beginning to end, you will find these initial leaf nodes in \$O(n)\$ time.
Any other nodes that become leaf nodes are found in step 5 above and can be handled without scanning.

Similarly, finding and removing edges can be done in amortized \$O(n)\$ time if you handle the edge lists correctly.  One key point is that even though you need to search through edge lists, there are only \$n\$ total edges, so that total time spent searching through edge lists is limited to \$O(n)\$.

Sample \$O(n)\$ program

Here is the program I wrote in C with the alternative strategy.  It solved all three test cases in 0.01 seconds.

Note: I did edge removal in a somewhat nonstandard way, where I marked the edge invalid instead of removing it from the edge list.  I wrote another version where I kept the edges in a doubly linked circular list and I actually removed the edge from the list when needed.  Both versions are just as fast, so if anyone wants to see the other version, just ask.

#include <stdio.h>
#include <stdint.h>
#include <stdlib.h>

const int MOD = 1300031;

typedef struct Edge {
    struct Edge *next;
    int          to;
    int          weight;
} Edge;

typedef struct Node {
    int         numConnected;
    int         numEdges;
    Edge       *edges;
} Node;

int main(void)
{
    int   numTests = 0;
    Node *nodes    = NULL;
    Edge *edges    = NULL;

    scanf("%d", &numTests);
    for (int i=0; i<numTests; i++) {
        int numNodes = 0;
        int e        = 0;

        scanf("%d", &numNodes);
        nodes = (Node *) calloc(numNodes, sizeof(*nodes));
        edges = (Edge *) calloc(2*numNodes, sizeof(*edges));

        // Read in list of edges.  For each edge, we create two Edge
        // structures and add one to each endpoint Node.
        for (int j = numNodes - 1; j > 0; j--) {
            int from   = 0;
            int to     = 0;
            int weight = 0;
            scanf("%d %d %d", &from, &to, &weight);
            from--;
            to--;

            edges[e].to       = to;
            edges[e].next     = nodes[from].edges;
            edges[e].weight   = weight;
            nodes[from].edges = &edges[e];
            nodes[from].numEdges++;
            e++;

            edges[e].to       = from;
            edges[e].next     = nodes[to].edges;
            edges[e].weight   = weight;
            nodes[to].edges   = &edges[e];
            nodes[to].numEdges++;
            e++;
        }

        int      search = 0;
        int      cur    = -1;
        uint64_t dist   = 0;

        for (int nodesLeft = numNodes-1; nodesLeft > 0; nodesLeft--) {
            Edge *edge;

            // Either use the node known to have 1 edge, or find the next
            // node that has 1 edge.
            if (cur == -1) {
                while (nodes[search].numEdges != 1) {
                    search++;
                    if (search >= numNodes) {
                        printf("ERROR, reached end of nodes\n");
                        exit(1);
                    }
                }
                cur = search++;
            }

            // Find the one valid edge in the list of edges.
            for (edge = nodes[cur].edges; edge != NULL; edge = edge->next) {
                if (edge->to != -1)
                    break;
            }
            if (edge == NULL) {
                printf("ERROR, reached end of edges\n");
                exit(1);
            }

            // The edge is going from cur <-> to, where cur is a leaf node.
            // We can calculate the distance used by the edge because we know
            // how many nodes are attached to cur.  That number is 1 for cur
            // itself plus nodes[cur].numConnected which is the number of
            // previous leaf nodes that we removed that are connected to cur.
            int to       = edge->to;
            int curNodes = nodes[cur].numConnected + 1;

            dist += (numNodes - curNodes) * curNodes * edge->weight;

            // Since we are removing the leaf node cur, we add its nodes
            // to the node it is attached to.
            nodes[to].numConnected += curNodes;

            // If the new node has 1 edge, use it, otherwise set cur to
            // negative to search for another node with 1 edge.
            if (--nodes[to].numEdges == 1)
                cur = to;
            else
                cur = -1;
            nodes[cur].numEdges = 0;

            // Delete other side of edge by marking its to field as -1.
            // This is needed so that when we search through the other
            // node's edges for its one remaining edge, we will skip this one.
            // We can find the paired edge by looking at its array index
            // because always created edges in consecutive pairs.
            int edgePairIndex = (edge - &edges[0]) ^ 0x1;
            edges[edgePairIndex].to = -1;
        }

        // printf("%lld\n", dist);
        printf("%lld\n", dist % MOD);
        free(edges);
        free(nodes);
    }
}


C++ Node class template I'm working on building understanding of data structures in general, and specifically linked lists in this example. I'm also trying to increase my ability with templates.

In cross-referencing dozens of examples, I've blended what I've seen into the following node template "Node.h" file:

/*
11 May 2015
VS 2015 Community
Author: Diche Bach
Node template header file.
*/
#ifndef NODE_H
#define NODE_H

#include<iostream>

template<class tNode>
class Node
{
    friend std::istream& operator>>(std::istream& str, tNode& data)
    {
        return str >> data_;
    }
    friend std::ostream& operator<<(std::ostream& str, tNode const& data)
    {
        return str << data_ << " ";
    }
    public:
      Node(tNode data, tNode *link);

    private:
      tNode data;
      tNode *next;
};

#endif /* NODE_H */

The goal is that this Node template be as modular and 'resusable' as possible. Thus, the same node template could be used with a LinkedList template, a DoubleLinked List template, a Stack template, etc.

With only two semesters of C++ under my belt, I'm pushing my envelope, and I appreciate suggestions and comments.

The use of the friend mechanism is something I've never done before, but it seems salient to the goals, so I particularly appreciate comments on that portion.

In the case of linked lists, the pointer to the next value should be a pointer to another Node instance, not a pointer to another data entry. This would allow you to walk the entire list node-by-node.

You have defined methods for using a stream to read/write the node's value, but what if I just want to access the value do something else with it that doesn't involve a stream? A getter like T& getValue(); and/or const T& getValue() const should be mandatory. This will also allow you to extract the stream operators outside of the class and be able to implement them without friend.

When it comes to templates T is preferred as the typename insteam of tNode which can be easily mistaken for a specific class instead of a template.


List a collection of webcorners public function getStuff($brand)
{
    $web=FALSE;
        if($this->getWebcorners()):
            foreach ($this->getWebcorners() as $webcorner):
                if(strtolower($webcorner->getBrand()->getName())== $brand):
                    return $webcorner;
                endif;
                if($webcorner->getBrandId()==NULL):
                    $web=$webcorner;
                endif;
            endforeach;
        endif;
    return $web;
}

the function is suppose to list a collection of webcorners and return the first that matches a brand, then the one that is null (in this order).  There's a possibility to have many other webcorners according to the brand (but no matching the one that is passed by parameter to the function).

Just cleaned up your method a little: (Guard condition, better variable name, less nesting)

public function getStuff($brand)
{
    if(!$this->getWebcorners()) return false;
    $fallback=false;
    foreach ($this->getWebcorners() as $webcorner) {
        if(strtolower($webcorner->getBrand()->getName())== $brand) return $webcorner;
        if($webcorner->getBrandId()==NULL) $fallback=$webcorner;
    }
    return $fallback;
}

If you want to return the first item without a brand you have to add the $fallback==false condition to your second if. Otherwise you use the last suitable item.