Creating a New User Account I feel like this maybe to much for one method...  This method lives inside of a service class which is called by an interface. It dumps data into three different tables within a database.

public UserAccount CreateAccount(NewAccount newAccount)
{
  if (!AccountAlreadyExists(newAccount.UserName))
  {
    string salt = StringCipher.GenerateCypher();
    UserAccount account = new UserAccount()
    {
      UserId = newAccount.UserName,
      PasswordHash = CreatePasswordHash(newAccount.Password),
      Lock = true,
      Created = DateTime.UtcNow
    };
    User user = new User()
    {
      UserId = newAccount.UserName
    };
    UserContact contact = new UserContact()
    {
      ContactTypeId = ContactType.EmailPrimary,
      Contact = StringCipher.Encrypt(newAccount.UserEmail, salt),
      UserId = account.UserId,
      Active = true,
      Created = DateTime.UtcNow
    };
    db.Entry(account).State = EntityState.Added;
    db.Entry(contact).State = EntityState.Added;
    db.Entry(user).State = EntityState.Added;
    db.SaveChanges();
    return account;
  }
  return null;
}

any suggestions on how to slim this down or streamline?

I personally think this is fine if the concept of creating a UserContact, User is directly tied to creating a UserAccount, i.e. one cannot live without the other.

That is because the abstraction of the method CreateAccount() hides away the implementation details of what needs to be done (insertion of data into 3 tables).

As you already have this abstraction you are now in the postion to potentially refactor the CreateAccount without effecting the rest of your code. Excellant!

So, as an option, I might consider refactoring the service layer itself so that the CreateAccount() method is a bit more streamlined, as you put it.

public UserAccount CreateAccount(NewAccount newAccount)
{
  if (!AccountAlreadyExists(newAccount.UserName))
  {
    UserAccount account = CreateUserAccount(newAccount);
    User user = CreateUser(newAccount);
    UserContact contact = CreateContact(newAccount);

    db.Entry(account).State = EntityState.Added;
    db.Entry(contact).State = EntityState.Added;
    db.Entry(user).State = EntityState.Added;

    db.SaveChanges();

    return account;
  }

  return null;
}

private UserAccount CreateUserAccount(NewAccount newAccount)
{
    string salt = StringCipher.GenerateCypher();

    return new UserAccount()
    {
      UserId = newAccount.UserName,
      PasswordHash = CreatePasswordHash(newAccount.Password),
      Lock = true,
      Created = DateTime.UtcNow
    };
}

private User CreateUser(NewAccount newAccount)
{
    return new User()
    {
      UserId = newAccount.UserName
    };
}

private UserContact CreateContact(NewAccount newAccount)
{
    return new UserContact()
    {
      ContactTypeId = ContactType.EmailPrimary,
      Contact = StringCipher.Encrypt(newAccount.UserEmail, salt),
      UserId = newAccount.UserName,
      Active = true,
      Created = DateTime.UtcNow
    }
}

This first lot of refactoring lead me to realise that newAccount is passed to each of the 3 methods.  I thought at this stage perhaps a class for these actions might make sense so that the common parameter is extracted to a class level variable.

public UserAccount CreateAccount(NewAccount newAccount)
{
  if (!AccountAlreadyExists(newAccount.UserName))
  {
    var accountBuilder = new AccountBuilder(newAccount);
    Account account = accountBuilder.CreateAccount();

    db.Entry(account.UserAccount).State = EntityState.Added;
    db.Entry(account.Contact).State = EntityState.Added;
    db.Entry(account.User).State = EntityState.Added;

    db.SaveChanges();

    return account;
  }

  return null;
}

internal class UserAccountBuilder
{
    private readonly NewAccount _newAccount;

    public UserAccountBuilder(NewAccount account)
    {
        _newAccount = account;
    }

    public Account CreateAccount()
    {
        return  new Account(
            CreateUserAccount(),
            CreateUser(),
            CreateUserContact()
        );
    }

    private string GetUsername()
    {
        return _newAccount.UserName;
    }

    private UserAccount CreateUserAccount()
    {
        string salt = StringCipher.GenerateCypher();

        return new UserAccount()
        {
          UserId = GetUsername(),
          PasswordHash = CreatePasswordHash(_newAccount.Password),
          Lock = true,
          Created = DateTime.UtcNow
        };
    }

    private User CreateUser()
    {
        return new User()
        {
          UserId = GetUsername()
        };
    }

    private UserContact CreateContact()
    {
        return new UserContact()
        {
          ContactTypeId = ContactType.EmailPrimary,
          Contact = StringCipher.Encrypt(_newAccount.UserEmail, salt),
          UserId = GetUserName(),
          Active = true,
          Created = DateTime.UtcNow
        }
    }
}

public class Account
{
    public UserAccount Account { get; private set; }
    public User User { get; private set; }
    public UserContact Contact { get; private set; }

    public Account(
        UserAccount userAccount,
        User user,
        UserContact userContact)
    {
        Account = userAccount;
        User = user;
        Contact = userContact;
    }
}

  NOTE:  This might be overkill but what I was going for here was
  extracting the persistance criteria out from the creation of the
  account itself.  Hence you could use this class to unit test creation
  of accounts without mocking or worrying about the db persistance layer
  which is contained in the service class layer.


Printing RPG game character properties I have written this code representing a class hierarchy of character classes, and a world class to print their properties.

Is anything unclear about this code?  Am I utilizing good practices?

class Human{
    protected String name;
    protected int health;
    protected int armorLevel;
    protected int magicLevel;
    protected int experience;
    protected int level;
        Human(){
        name = "Andrew The Magic";
        health = 100;
        armorLevel = 1;
        magicLevel = 1;
        experience=0;
        level =1;
        }

    Human(String name){
        this(); //calling main constructor
        this.name = name;
    }

void fight() {}
void levelUp() {}
void showAbility() { System.out.println("Name: "+name+" HP: "+health+" ARMOR: "+armorLevel+" MAGIC: "+magicLevel+" EXP: "+experience+" LVL: "+level); }
void yell(){    System.out.println("IM A CHARACTER ;-(");   }
}

//--------------------------------------------------------------
class Warrior extends Human{

    Warrior(){
        super();
    }

    Warrior(String name){
        super(name);
    }

    void fight(){
        System.out.println("I attack with sword");
        experience +=10;
        if (experience >= 20 ){ levelUp();  }
        }

    void levelUp(){
        System.out.println("LEVEL UP!");
        health +=100;
        armorLevel +=25;
        level++;
    }

    void yell(){
        System.out.println("IM A GLORIOUS WARRIOR "+name);
    }
}
//--------------------------------------------------------------

public class World{

    static void greeting(Human p){
        p.yell();
    }

        public static void main(String[] args){
          Warrior w = new Warrior("Noname the Warrior");
            w.showAbility();
            w.fight();
            w.showAbility();
            w.fight();
            w.showAbility();

            greeting(w);
   }
}

Is anything unclear about this code?

The purpose of this code is unclear.

  Do I break any good-programming rules?

Some bad practices that immediately jump into the eye:

Formatting is horrible
The parameterless constructor of Warrior is pointless and can be safely omitted
Methods with empty bodies, such as fight and levelUp in Human should at least have a comment explaining why the implementation is left empty
showAbility is poorly named:

None of the attributes are abilities: name is certainly not an ability, and the other fields are questionable too
"show" is a bit ambiguous. Show where? In a pop-up, maybe? I'd use the word "print" instead.

Maybe printStats would be better.
It's probably not a big concern for your programming exercise, but for the record, print statements inside a program are generally frowned upon. The preferred way is to use a logging framework instead. (Again, this is just for the record, since you asked about bad practices.)

Instead of Warrior inheriting from Human,
consider using interfaces.
For example you could have Warrior and Mage interfaces,
so that you can have mighty mage-warriors.
With inheritance this would be impossible.
Using interfaces will lead to a more extensible design.


Probe request capturing with Scapy This is a probe request packet sniffer using python/scapy. It sniffs Dot11ProbeReq packets and display the ssid, MAC of device and manufacturer name from the probe requests. Can also output the data to a log file if desired. Right now I feel the script is a bit slow and I can see it slow down from the first time I wrote it. It works but I know the code is nowhere as efficient as it could be. Can you point out my mistakes and let me know how I can write more efficient code?

#!/usr/bin/env python
# import all the needed libraries
import sys
from netaddr import *
import logging
logging.getLogger("scapy.runtime").setLevel(logging.ERROR)
from scapy.all import *
from subprocess import *
import datetime
import time

# clear the console
call(["clear"])

# set date-time parameters
today = datetime.date.today()
d=today.strftime("%d, %b %Y")
tf=time.strftime(" %H:%M")
t=time.strftime(" %H:%M:%S")

# define variables
clients = []
uni = 0
mach = []
manu =[]

# our packet handler
def phandle(p):
    global uni
    if p.haslayer(Dot11ProbeReq):
        mac = str(p.addr2)
        if p.haslayer(Dot11Elt):
            if p.ID == 0:
                ssid = p.info
                if ssid not in clients and ssid != "":
                    clients.append(ssid)
                    maco = EUI(mac)
            macf = maco.oui.registration().org
            print len(clients),mac+" ("+macf+") <--Probing--> "+ssid
            if args.log:
                f.write (str(len(clients))+" "+mac+" ("+macf+") //"+" <--Probing--> "+ssid+"\n")
                if mac not in mach:
                            mach.append(mac)
                            uni+=1
            elif mac not in mach:
            mach.append(mac)
                        uni+=1

# our main function
if __name__ == "__main__":
    import argparse
    parser = argparse.ArgumentParser(description='PyRobe Help')
    parser.add_argument('interface', action="store", help="specify interface (ex. mon0)", default=False)
    parser.add_argument("-l","--log", dest="log",action="store_true", help="print log file")
    args = parser.parse_args()
    if args.log:
        f = open("ProbeLog"+str(today)+str(tf)+".txt","w")
        sniff(iface=args.interface,prn=phandle, store=0)
            print ("\n")
            print "Unique MACs: ",uni
        f.write ("\nUnique MACs: "+str(uni))
        f.write ("\nScan performed on: "+str(d)+" at"+str(t))
        f.close()
        print "Log successfully written. Exiting!"
    else:
        sniff(iface=args.interface,prn=phandle, store=0)
        print "\nSuccessfully Exited! No log file written."

I am basically looking for pointers on:

Handling command line arguments better
Calculating and assigning required memory/avoid leaks
Shortening the code to make it faster to execute

I am not looking for someone to edit my script and spoon feed me the solution, just point me in the right direction!

My Python version: 2.7.3 (default, Mar 13 2014, 11:03:55) [GCC 4.7.2]
My linux version: Distributor ID:Kali Description:Kali GNU/Linux 1.1.0 Release:1.1.0 Codename:moto Linux version 3.18.0-kali3-amd64 gcc version 4.7.2 Debian 4.7.2-5) ) #1 SMP Debian 3.18.6-1~kali2 (2015-03-02)

Code style

Always mention what exactly you are importing - never use import * - I don't know these libraries and when I read this for the first time I need to find all possible places where these methods can be from: call, sniff. If you want to write code which other people can read - always import exactly what you are using:

from netaddr import *

Be consistent when you are importing: import argparse is imported in main function which is really unnecessary - import on top with other imports.

Also  - this is picky but it is more readable to have imports in alphabetical order and not mixed with other code like logging.getLogger("scapy.runtime").setLevel(logging.ERROR)

String formatting is awful and not-readable:

Instead of:

str(len(clients))+" "+mac+" ("+macf+") //"+" <--Probing--> "+ssid+"\n"

Use format method:

"{0} mac {1}".format(len(clients), macf)

Or - even more readable:

"{length} mac {macf}".format(
    length=len(clients),
    macf=macf,
 )

Much more readable and is actually a standard.

Formatting (indentations) is a bit off - difficult to understand some pieces.

Performance

You have mach is list and you are constantly searching in it. Search in list is O(n). Make is set or dict - will speed it up - search performance becomes O(1).

mach = []
# this check is expensive
if mac not in mach:

Same comment  - for clients list.

Here - remove condition, just leave else - or maybe your formatting is misleading here and I didn't understand logic correctly:

elif mac not in mach:


Add elements to collection conditionally Is there a smarter way to write this code?  I used to add all the elements in the collection constructor, something like this:

new OrderWizardStateCollection([new OrderWizardState(..), new OrderWizardState(..)..])

but now, since I have this one conditional addition, I'm writing it this way:

this.state_collection = new OrderWizardStateCollection();
this.state_collection.add(new OrderWizardState({ state: 'basket', phase: this.phase_collection.findWhere({ phase: 'basket' }) }));
this.state_collection.add(new OrderWizardState({ state: 'account', phase: this.phase_collection.findWhere({ phase: 'account' }) }));
if ( _.has(this.regions, "lp") ) this.state_collection.add(new OrderWizardState({ state: 'loyaltypoints', phase: this.phase_collection.findWhere({ phase: 'account' }) }));
this.state_collection.add(new OrderWizardState({ state: 'delivery', phase: this.phase_collection.findWhere({ phase: 'delivery' }) }));
this.state_collection.add(new OrderWizardState({ state: 'payment', phase: this.phase_collection.findWhere({ phase: 'payment' }) }));
this.state_collection.add(new OrderWizardState({ state: 'placeorder', phase: this.phase_collection.findWhere({ phase: 'placeorder' }) }));

You could do a loop over the elements that you are always adding:

for (var info in ["basket", "account", "delivery", "payment", "placeorder"] {
    this.state_collection.add (new OrderWizardState ({ state: info, phase: this.phase_collection.findWhere ({ phase: info }) }));
}

And to add the special one, you use your if statement and insert it at a specific index:

if ( _.has (this.regions, "lp")) {
    var state = new OrderWizardState({ state: 'loyaltypoints', phase: this.phase_collection.findWhere({ phase: 'account' }) })
    this.state_collection.insert (2, state);
}


Minify and send HTML response in Go I need to have some static HTML pages on a project I am making with Go. I used the http.FileServer, but I noticed it was slower than rendering with html/templates, and I didn't like seeing the .html at the end of the routes. So I went with this solution:

// home.go
package pages

import "strings"

var Home string

func init() {
    html := `
        <!DOCTYPE html>

                <title>Page Title</title>

Home Page

    `

    htmlArray := strings.Fields(html)
    htmlJoined := strings.Join(htmlArray, " ")
    Home = strings.Replace(htmlJoined, "> <", "><", -1)
}

and:

// main.go
package main

import (
    "net/http"
    "io"

    "my_project/router"
    "my_project/pages"
)

func main() {
    router.On("GET", "/", home)
    http.HandleFunc("/", router.Handle)
    http.ListenAndServe(":8080", nil)
}

func home(w http.ResponseWriter, r *http.Request) {
    io.WriteString(w, pages.Home)
}

This all seems to be working well. If I understand correctly, the minification takes place when the app starts, and not on each request.

Is there anything wrong with this approach?

The Home variable can be modified by any package. Pretty dangerous. Maybe use a private variable (or a struct that contains a private string field) and a getter method.
You're assuming that any HTML page won't be modified semantically by your transformation. This is correct for your minimalistic example, but not in the general case. For example, within
 or  tags, whitespace matters. There are probably many other ways this could go wrong. You should probably use a minifier package like this one instead.

Edit: actually, your strings.Replace(htmlJoined, "> <", "><", -1) will probably have undesired effects even for very simple HTML pages. Consider:

this pretty simple example.

If you delete the space between  and , it effectively deletes the visible space between "simple" and "example" when you display the page. All the more reason to use a minifier library.


Robots files generation - simplifying foreach code I'm trying to figure out if there's a way I can simplify this code which is used to generate a robots.txt file with numerous rules.  Different files/folders are separated in separate arrays because they're applied differently but in some cases they are applied the same.  In these cases, rather than write out a separate foreach for each one, this is what I'm trying to simplify:

<?php

public function generateRobotsFile($rbfile, $smfile) {
    $tab = array();
    $tab['SemBot'] = array('Googlebot',"bingbot\nCrawl-delay: 5",'MSNBot');

    $tab['Lang'] = array('eb');
    $tab['Files'] = array('address.php','cart.php');
    $tab['Folder'] = array('classes','docs','themes');

    fwrite($writeFd, "\nUser-agent: *\n");
    foreach ($tab['Lang'] as $Lang) {
        fwrite($writeFd, 'Disallow: ' . __PS_BASE_URI__ . $Lang . "\n");
    }
    foreach ($tab['Folder'] as $Folder) {
        fwrite($writeFd, 'Disallow: ' . __PS_BASE_URI__ . $Folder . "\n");
    }

    foreach ($tab['SemBot'] as $SemBot) {
        fwrite($writeFd, "User-agent: ' . $SemBot . "\n");
        foreach ($tab['Files'] as $Files) {
            fwrite($writeFd, 'Disallow: ' . __PS_BASE_URI__ . $Files . "\n");
        }
        foreach ($tab['Folder'] as $Folder) {
            fwrite($writeFd, 'Disallow: ' . __PS_BASE_)URI__ . $Folder . "/\n");
        }
    }

The arrays must remain separated but in some cases they are looped through the same disallow rule so I'm reaching for a foreach array in array but haven't quite grasped it yet.

If number of lines is your primary concern, then really all we can do with this is merge your array creation lines, and use array_merge() to create temporary arrays which you can loop around, like so:

public function generateRobotsFile($rbfile, $smfile)
{
    $tab = array('SemBot' => array('Googlebot', "bingbot\nCrawl-delay: 5", 'MSNBot'),
                 'Lang'   => array('eb'),
                 'Files'  => array('address.php', 'cart.php'),
                 'Folder' => array('classes', 'docs', 'themes'));
    fwrite($writeFd, "\nUser-agent: *\n");
    foreach(array_merge($tab['Lang'], $tab['Folder']) as $disallow)
    {
        fwrite($writeFd, 'Disallow: ' . __PS_BASE_URI__ . $disallow . "\n");
    }
    foreach($tab['SemBot'] as $SemBot)
    {
        fwrite($writeFd, "User-agent: " . $SemBot . "\n");
        foreach(array_merge($tab['Files'], $tab['Folder']) as $disallow)
        {
            fwrite($writeFd, 'Disallow: ' . __PS_BASE_URI__ . $disallow . "\n");
        }
    }
}


Formatting small string use example I am trying to format my code properly. I have already read the programming standards document and I believe that my code follows these rules. I have also used the CtrlShiftF keys in Eclipse which should format the code automatically.

Could someone tell me if this looks ok?  It would be nice to get some feedback back and to see if it is being created automatically. I already know that the program does work. Any help would be great.  If someone knows a better way to format the code, it would be great to hear it as well.

public class UniString {
    public static void main(String[] args) {
        char[] det = { '\u20ac', '1', '1', '8' };
        // char[]det is an array of characters.

        for (char a : det) {
            /*
            * the for loop takes in the char values and prints them off in
            * order the "det" array enters into the for loop  through "det" ,
            * goes into char a and can be written to the console through
            * System.out.println(a).
            */
            System.out.print(a);
        }
        System.out.println();

        char[] name = { 'j', 'o', 'e' };
        // This is another char array.

        String nameString = new String(name);
        /*
        * The char array is made into a String by loading "name" into a String
        * class
        */

        String changeCase = nameString.toUpperCase();
        /*
        * The String "nameString" is changed to Upper case letters. The string
        * is then stored as "caseChange".
        */

        System.out.println(changeCase);
        // "caseChange" is printed out.
    }
}

I think all the comments should be removed. The point of a comment is to explain why you wrote this code, not what the code does. If anyone reads the code, they will be able to understand what it does without reading the comments. When you write comments, always ask yourself : "Will this comment help me understand why I wrote this code 5 years ago?" (Or.. 2 weeks if you have a bad memory like mine), if you can't answer, you probably don't need comments!

The formatting looks perfect in my opinion.


Checking validity of NxN Sudoku Given a multi-dimensional array representing a board in Sudoku, the function should be able to return whether the sudoku is valid or not.

Examples:

var goodSudoku = [
  [7,8,4, 1,5,9, 3,2,6],
  [5,3,9, 6,7,2, 8,4,1],
  [6,1,2, 4,3,8, 7,5,9],

  [9,2,8, 7,1,5, 4,6,3],
  [3,5,7, 8,4,6, 1,9,2],
  [4,6,1, 9,2,3, 5,8,7],

  [8,7,6, 3,9,4, 2,1,5],
  [2,4,3, 5,6,1, 9,7,8],
  [1,9,5, 2,8,7, 6,3,4]
];

var badSudoku = [
  [1,1,1, 1,1,1,  2,2,2],
  [5,3,9, 6,7,2, 8,4,1],
  [6,1,2, 4,3,8, 7,5,9],

  [9,2,8, 7,1,5, 4,6,3],
  [3,5,7, 8,4,6, 1,9,2],
  [4,6,1, 9,2,3, 5,8,7],

  [8,7,6, 3,9,4, 2,1,5],
  [2,4,3, 5,6,1, 9,7,8],
  [1,9,5, 2,8,7, 6,3,4]
];

validSudoku(badSudoku); // false;
validSudoku(goodSudoku); // true;

Specific questions

How can I improve my function? What am I doing bad/good?
Is my algorithm efficient? Advice on it?

Here is my code

function validSudoku(data) {
    var valid = true,
        temp = [],
        data,
        side,
        slot;

    // Check wrong size
    if (data[0].length !== data.length) valid = false;

    // slot*slot
    slot = Math.sqrt(data.length);

    // Verifiy horizontal
    data.forEach(function(arr) {
        valid = valid && arr.every(function(val, i) { return arr.indexOf(i + 1) > -1; });
    });

    // Verifiy vertical lines
    data.forEach(function(arr, i) {
        temp  = data.map(function(val) { return val[i]; });
        valid = valid && arr.every(function(val, i) { return temp.indexOf(i + 1) > -1; });
    });

    // Verifiy boxes
    for (var i = 0; i < slot; i++) {

        data.forEach(function(val, e) {
            side  = val.slice(slot * i, slot * i + slot);
            temp  = temp.concat(side);

            if ((e+1) % slot == 0 && e > 0) {
                for (var j = 1; j <= data.length; j++)
                    if (temp.indexOf(j) < 0) valid = false;
                temp = [];
            }

        });

    }
    return valid;
}

There are a few problems with your validation, and the code itself could be a bit neater.

First up, your code will try to validate impossible puzzles. Sudoku puzzles need to be square-sided You check that the puzzle is square, but your slot-size is not valid. It assumes a side that is a perfect square (4, 9, 16, etc.).

Additionally, you only check the first row is the right length, I suspect your code will validate as true if one of the rows has an extra element (with a carefully-chosen value).

In multiple places you set valid = false, and in those places you should just return false. You know the puzzle is not valid, so why continue checking?

Finally, it would be nice if you could make the checking generic. There are 3 things to check:

all expected elements in each row are present
all expected elements in each column are present
all expected elements in each mini-square are present.

I would try to put that together as a collection of functions for each check:

var extractions = [
    {   name: "rows",
        get:  function(data,span,block,member){
                  return data[block][member];
              }
    },
    {   name: "cols",
        get:  function(data,span,block,member) {
                  return data[member][block];
              }
    },
    {   name: "minis",
        get:  function(data,span,block,member) {
                  var coloff = (block % span) * span;
                  var rowoff = (block / span) * span;
                  return data[rowoff + (member / span)][coloff + (member % span)];
              }
    },
]

function isvalid(data) {
    var size = data.length;
    var slot = Math.sqrt(size);
    for (var block = 0; block < size; block++) {
        if (data[block].length != size) {
            return false;
        }
        for (var ext = 0; ext < extractions.length; ext++) {
            var tmp = [];
            for (var member = 0; member < size; member++) {
                tmp.push(extractions[ext].get(data, slot, block, member));
            }
            for (var i = 1; i <= size; i++) {
                if (tmp.indexOf(i) < 0) {
                    return false;
                }
            }
        }
    }
    return true;
}


Sliding effect with bounce I have used animate.css and jQuery as external libs. Is there a better way of doing what I have done with jQuery?

DEMO

$(document).ready(function () {

    $('#test li').on('click', function () {

        var className = $(this).attr('class');
        $('#test2 > li').each(function () {

            var prodName = $(this).data('prod');

            if (prodName == className) {

                $('#test2 > li').removeClass('bounceInRight').addClass('bounceOutRight');
                $this = $(this);
                setTimeout(function () {
                    console.log(this);
                    $('#test2 > li').addClass('hide')
                    $this.removeClass('hide bounceOutRight').addClass('bounceInRight');
                }, 400);

            }
        });
    })
});
ul#test > li {
    display: inline-block;
    padding: 10px;
    cursor: pointer;
}
#test {
    position: fixed;
    top: 0;
    left: 0;
}
#test2 {
    position: fixed;
    top: 0;
    right: 0;
    list-style: none;
}
#test2 > li {
    background: yellow;
    width: 350px;
    height: 600px;
    /* transition: height 0.3s ease, width 0.2s ease;*/
    overflow: hidden;
    margin-top: -30px;
    padding-top: 30px;
    margin-right: -50px;
}
.hide {
    display: none;
}

<link rel="stylesheet" type="text/css" href="https://cdnjs.cloudflare.com/ajax/libs/animate.css/3.2.6/animate.min.css">

1

2

3

1

2

3

Your demonstration could benefit from better semantic markup.  In particular, the numbers on the left should be treated as navigation links, whose target is the item to be animated.  If you remove all JavaScript and CSS, the functionality degrades gracefully.

An additional benefit is that the link directly identifies the item to be manipulated, so that you don't have to locate the element yourself using a var prodName = $(this).data('prod'); if (prodName == className) … check.

You don't need to define a .hide { display: none; } rule in your CSS.  The jQuery $.hide() and $.show() methods can do that job.

Take care to localize $this.

$(document).ready(function () {
    $('nav > ul > li > a').on('click', function () {
        $('li.animated').hide();
        $($(this).attr('href')).each(function () {
            var $this = $(this);
            $this.show()
                 .removeClass('bounceInRight')
                 .addClass('bounceOutRight');
            setTimeout(function () {
                $this.removeClass('bounceOutRight')
                     .addClass('bounceInRight');
            }, 400);
        });
    })
});
nav > ul > li {
    display: inline-block;
    padding: 10px;
}
nav > ul > li > a {
    color: black;
    text-decoration: none;
}
ul {
    position: fixed;
    top: 0;
    right: 0;
    list-stlye: none;
}
nav > ul {
    right: auto;
    left: 0;
}
li.animated {
    background: yellow;
    width: 350px;
    height: 600px;
    /* transition: height 0.3s ease, width 0.2s ease;*/
    overflow: hidden;
    margin-top: -30px;
    padding-top: 30px;
    margin-right: -50px;
}

<link rel="stylesheet" type="text/css" href="https://cdnjs.cloudflare.com/ajax/libs/animate.css/3.2.6/animate.min.css">

1

2

3

1

2

3


A tiny util to converts an SO URL to revised version I am trying to write a util that converts an SO URL to revised version, for example, given this url:

'https://stackoverflow.com/q/57449318/10731613'

this util returns:

'https://stackoverflow.com/posts/57449318/revisions'

here is the code

import re
url = input()
if len(url)<10:
    url = 'https://stackoverflow.com/q/57449318/10731613'
url.replace(re.findall('\/\d{3,9}', url)[-1], '', 1).replace('/q', '/posts', 1)+'/revisions'

It works, and I would like some code review.

I'd suggest making this a function - while as a oneliner, it's good as is, when we start scaling it, adding in more cases, etc., it becomes a mess that requires rearranging for even the tiniest changes.

But first, let's start off with the changes I'd suggest.

First off, I'd like to slightly adjust your regex to handle URL parameters:

\/\d{3,9}(\/?\?([\-\w%*.~]+=[\-\w%*.~]+(&*))*)?$

The explanation is below. Also, to make it a bit easier for us later, we'll wrap this all in a capture group (simply surround with brackets).

(  # Start main set
  \/\d{3,9}  # Find any '/' character followed by 3-9 numbers
  (  # Start 'Parameter' set, which matches any query parameters
    \/  # Match '/' character...
      ?  # ... optionally
    \?  # Match escaped '?' character
    (  # Start 'Parameter List' set
      [\-\w%*.~]  # Character set, according to HTML5 spec (parameter name)
                +  # Match as many characters as possible
      =  # Followed by equals
      [\-\w%*.~]  # Character set, according to HTML5 spec (parameter value)
                +  # Match as many characters as possible
      (&*)  # Optionally match any '&' characters on the end.
    )  # End 'Parameter List' set
      *  # 'Parameter List' set is optional, but can also be repeated infinite times.
  )  # End 'Parameter' set
    ?  # Parameter set is optional
)
  $  # The match should be at the end of the string

It's fine that we're matching the query parameters with the last set of numbers (which we're going to delete), i.e it's not a problem that we delete the query parameters, because using the same parameters on a different page would be fairly useless.

Next, I'd put all of your code in a function block:

def convertURLToRevisionsFromQ(url):

Note how we're taking url as an argument, not as a user input.

The next step is to implement some basic error-checking, to throw some ValueErrors. Error handling manually is always superior to either catching built-in errors, or, even worse, not doing anything about them. So, we'll write an error handling helper function inside our main function, like so:

def throwInvalidInputError(err):
  raise ValueError("Invalid URL type! Error: " + err)

We can now do some error checking right off the bat, before we go on with processing the URL:

question_path_search = re.search('(\/q(uestions)?\/)', url)

if "stackoverflow.com/" not in url:
  throw_invalid_input_error("URL not from Stack Overflow.")
if len(url) < 23 or not question_path_search:
  throw_invalid_input_error("URL not from a Stack Overflow question path.")

You can see that we are also accepting /questions/ as valid, since we'll add support for those paths too in a moment.

The next change I'd suggest is to add support for URLs which don't necessarily have both number sequences, e.g https://stackoverflow.com/q/57449318 as opposed to https://stackoverflow.com/q/57449318/10731613. We're also going to bring in our Regex here, since we're finally going to start matching the ending. The only difference is, the 'ending' bit we need to remove will count as either only the query parameters, in the case of just one number sequence, or the last number sequence and the query parameters, in the case of two number sequences. So, we'll do the following:

matched = re.search('((\/?\?([\-\w%*.~]+=[\-\w%*.~]+(&*))*)?$)', url)
ending = matched[0]

if not matched:
  throw_invalid_input_error("Could not find question IDs in valid format!")

if len(re.findall('\/\d{3,9}', url)) > 1:
  matched = re.findall('(\/\d{3,9}(\/?\?([\-\w%*.~]+=[\-\w%*.~]+(&*))*)?$)', url)
  if not matched:
    throw_invalid_input_error("Could not find question IDs in valid format!")

  ending = matched[-1][0]

if len(matched) > 1:
  throw_invalid_input_error("Malformed URL")

Now, it's mostly just your code, except we'll add in one more flexibility: allowing both /q and /questions. To do this, we'll just use the question_path_search variable, which we defined earlier. Grab the first capture group from it, and that will be the version of /q vs /questions we'll use:

question_path = question_path_search[0]

url = url.replace(ending, '', 1)\
  .replace(question_path, '/posts/', 1)\
  + 'revisions'

Finally, we'll just do return url.

So, here's our final code, with a bit of testing code at the bottom:

import re

def convertURLToRevisionsFromQ(url):
  def throw_invalid_input_error(err):
    raise ValueError("Invalid URL type! Error: " + err)

  question_path_search = re.search('(\/q(uestions)?\/)', url)

  if "stackoverflow.com/" not in url:
    throw_invalid_input_error("URL not from Stack Overflow.")
  if len(url) < 23 or not question_path_search:
    throw_invalid_input_error("URL not from a Stack Overflow /q question path.")

  matched = re.search('((\/?\?([\-\w%*.~]+=[\-\w%*.~]+(&*))*)?$)', url)
  ending = matched[0]

  if not matched:
    throw_invalid_input_error("Could not find question IDs in valid format!")

  if len(re.findall('\/\d{3,9}', url)) > 1:
    matched = re.findall('(\/\d{3,9}(\/?\?([\-\w%*.~]+=[\-\w%*.~]+(&*))*)?$)', url)
    if not matched:
      throw_invalid_input_error("Could not find question IDs in valid format!")

    ending = matched[-1][0]

  if len(matched) > 1:
    throw_invalid_input_error("Malformed URL")

  question_path = question_path_search[0]

  url = url.replace(ending, '', 1)\
    .replace(question_path, '/posts/', 1)\
    + 'revisions'

  return url

res = convertURLToRevisionsFromQ(input("Enter Stack Overflow URL: "))
print(res)

Not so 'tiny' any more, but it's definitely more adaptable, and will probably work with few adjustments to whatever similar problem you throw at it. Now, it's more generally a URL path redirector, than just some ultra-specific code to solve an ultra-specific problem.


Sum of 2 numbers in an array I have this problem to solve:

Given an integer \$z\$ and an array \$a\$ of distinct integers sorted in ascending order, find two numbers, \$x\$ and \$y\$, belonging to the array such that \$x + y = z\$. All this has to be done with \$O(n)\$ time performance in the worst case and \$O(1)\$ space.

I tried to jot down a trivial solution:
public static boolean find(int[] a, int z) {
    for (int i = 0; i < a.length; i++) {
        for (int j = i + 1; j < a.length; j++) {
            if (a[i] + a[j] == z)
                return true;
        }
    }
    return false;
}

However, it is clearly O(n^2) and I cannot find a better algorithm.
Can you help me?

Since there's no real review to do and we're apparently just posting solutions... here's a simple O(n) time O(1) space one:
public static boolean find(int[] a, int z) {
    int i = 0, j = a.length - 1;
    while (i < j) {
        int sum = a[i] + a[j];
        if (sum == z)
            return true;
        if (sum < z) i++; else j--;
    }
    return false;
}

Explanation: Try the ends, i.e., add the smallest and largest value. If the sum is right, bingo. If the sum is too small, then the smallest value is useless (it would need to be added with a number larger than the largest). If the sum is too large, then the largest value is useless. So remove the useless one. But since actually removing it, i.e., creating an array without it, would be expensive, just work with indexes marking the ends of the still potentially useful part of the array.


Debug merge sort and validate comparison counters I am posting the code for my merge sort to see if I can get folks offer suggestions as to where to make comparisons or if I am making the correctly already.

I will post my heap sort and quick sorts in other posts.  I actually have them all in a package that runs them together but separating them out for clarity.

package AlgorithmComparison;

import java.util.*;
import java.math.*;

public class AlgCompareApp
{
  public static void main(String[] args)
  {
  int mSortCnt = 0;
  int numbersNeeded = 5000;  // Number in the array
  double total = logb(numbersNeeded,2);
  double logN = numbersNeeded * total;
  int loopCount = 50;  // Change this for the Loop through each sort

  System.out.print("n log(n) for " + numbersNeeded + " is ");
  System.out.printf(" %.2f",logN);
  System.out.println();
  System.out.println("Merge Sort");
    for(int l = 0; l < loopCount; l++)
    {
    Random rng = new Random();
    // Create initial Array List of numbers
    ArrayList<Integer> arrlist = new ArrayList<Integer>();

    // Populate Initial Array List with numbers and no duplicates
    for (int i = 0; i < numbersNeeded; i++)
    {
      while (true)
      {
        Integer next = rng.nextInt(numbersNeeded * 2);
        if (!arrlist.contains(next))
        {
          arrlist.add(next);
          break;
        }
      }
    }

    // Create QuickSort Int Array from ArrayList
    int[] mergeSortArray = new int[arrlist.size()];
    for (int i = 0; i < arrlist.size(); i++)
    {
      mergeSortArray[i] = arrlist.get(i);
    }
    //System.out.print("\n---------------Merge Sort---------------\n");
    MergeSortRun mgsrt = new MergeSortRun();
    mgsrt.MergeSortRun(mergeSortArray, 0, mergeSortArray.length - 1);
    mgsrt.displayList();
    mSortCnt = mSortCnt +mgsrt.getCounterOne();
      }
    System.out.println("Averages");
    System.out.println((mSortCnt / loopCount));
    System.out.println("Percentage of n log(n)");
    System.out.printf("%.2f",((mSortCnt / loopCount)/logN));
    System.out.println();
  }

  public static double logb( double a, double b )
{
return Math.log(a) / Math.log(b);
}
}

package AlgorithmComparison;

import java.util.*;

public class MergeSortRun
{
    private int counterOne = 0;
   int[] displayList;

  public void MergeSortRun(int array[], int lo, int n)
  {
    int low = lo;
    int high = n;
    if (low >= high)
    {
      return;
    }
    int middle = (low + high) / 2;
    MergeSortRun(array, low, middle);
    MergeSortRun(array, middle + 1, high);
    int end_low = middle;
    int start_high = middle + 1;
    while ((lo <= end_low) && (start_high <= high))
    {
      if (array[low] < array[start_high])
      {
        low++;
      } else
      {
        int Temp = array[start_high];
        for (int k = start_high - 1; k >= low; k--)
        {
          array[k + 1] = array[k];
        }
        array[low] = Temp;
        low++;
        end_low++;
        start_high++;
        //counterOne++;
      }
      counterOne++;
    }
    // counterOne++;
    displayList = Arrays.copyOf(array, n + 1);
  }

  public void displayList()
  {
//    for (int i = 0; i < displayList.length; i++)
//    {
//      System.out.print(displayList[i] + " ");
//    }
    System.out.println(counterOne);
  }

  public int getCounterOne()
  {
    return counterOne;
  }
}

My entire java package is zipped up and can be downloaded from here.

Here's some suggestions on what you can do differently to get a better result (both performance counters and code style).

The MergeSortRun class breaks many rules for naming conventions in Java.... well, actually the class name is OK (MergeSortRun), but the method (also called MergeSortRun) is very confusing, because only the constructor (which you don't have coded up) is supposed to have that name.

Rename the MergeSortRun method to something simple like 'sort'.

Additionally, the variables 'counterOne' and 'displaylist' are not necessary... if it were me, I would have 'counterOne' returned from the sort method, and the displaylist variable is completely dead anyway. Doing this will allow you to make the sort method static, of the form:

public static final int sort(int array[], int lo, int n) {...}

The 'int' return value is the number of comparisons you want to count.

Now, you have to question what your counter should be. There are two key items that tend to impact performance of sorts. The one is comparisons, and the other is data-moves (or swaps).

Since your input data is an int[] array, the comparisons between values are just as expensive as any other int based comparison. Thus, I think you should potentially be counting much more. The easiest way to do it is to show you what I think it should look like:

public int sort(int array[], int lo, int n) {
    int low = lo;
    int high = n;
    int compares = 1;
    if (low >= high) {
        return compares;  // NOTE - 1 comparison (low >= high)
    }
    int middle = (low + high) / 2; // technically should be: (low + high) >>> 1
    compares += sort(array, low, middle);
    compares += sort(array, middle + 1, high);
    int end_low = middle;
    int start_high = middle + 1;
    while ((lo <= end_low) && (start_high <= high)) {
        compares += 3; // two in 'while' loop above, and one in 'if' below.
        if (array[low] < array[start_high]) {
            low++;
        } else {
            int Temp = array[start_high];
            compares++; // entry in to the for loop
            for (int k = start_high - 1; k >= low; k--) {
                array[k + 1] = array[k];
                compares++; // exit from the for loop
            }
            array[low] = Temp;
            low++;
            end_low++;
            start_high++;
        }
    }
    return compares;
}


Stock product and lot duplicate finder I have this function that searches for stock moves that have the same product, and if it finds them then it searches for lots in those lines, and if it finds the same lots then UserError is raised.
The thing is I think this method can be rewritten in a more elegant way. Maybe someone can help me with that.
def check_for_duplicates(self):
    exist_product_list = {}
    duplicates = {}
    for line in picking.move_lines:
        if line.product_id.id in exist_product_list:
            duplicates[line.id] = line.product_id.id
            for product, line_id in exist_product_list.items():
                if product == line.product_id.id and line.id != line_id:
                    duplicates[line.id] = line.product_id.id
                    duplicates[line_id] = product
        exist_product_list[line.product_id.id] = line.id
    duplicate_lots = []
    if duplicates:
        for line in self.env['stock.move'].browse(duplicates.keys()):
            lots = line.mapped('lot_ids')
            for lot in lots:
                if lot in duplicate_lots:
                    raise UserError(
                        _('You have few lines with same product %s and same lot in it') % line.product_id.name)
                duplicate_lots.append(lot)

Avoid similar variable names
There's line.product_id.id, line_id and line.id. This is very confusing. Try to avoid having variable names that are too close to each other.
Simplifying the code
You can indeed simplify the code somewhat. First, you want to find out if there are multiple lines that reference the same product. To do that, you want to have a dict that maps from product IDs to a list of lines. The easiest way to do this is to use a defaultdict(list):
from collections import defaultdict

def check_for_duplicates(self):
    product_lines = defaultdict(list)

    # Build a map of which lines reference which products
    for line in picking.move_lines:
        product_lines[line.product_id.id].append(line.id)

Then you can go over that map to check if any product occurs on more than one line:
    # Find duplicates
    for product, lines in product_lines.items():
        if len(lines) > 1:
            # Report lines with duplicate products here
            ...

I'm not sure how to improve on the part that checks for duplicate lots, I don't know what type self.env[...] is and what the browse() function does, but perhaps the above code will help you simplify that as well.


Expense-sharing calculation Friends A, B, C, D go for a trip.
They spend on various expenses.
Cost of the expense is shared.

Example :

A spends 100 for breakfast for A, B, C and D
D spends 500 for cab for B and C
B spends 300 for lunch for A, B and C

Write a program to calculate how much each should get or each should give to one another. App should be scalable that number of friends can change

For the above problem I have written the below code.  I need help to know whether I have written quality code utilising an optimal OOP design.

public class SpendDetect
{

    Map<String, PayDetails> outputMap = new HashMap<String, PayDetails>();

    public void getPayDetails(String payer, int amount, List<String> beneficiary) {

        List<String> consolidatedBenificiary = beneficiary;
        int dividableCount = consolidatedBenificiary.size();
        int amountToBePaid = 0;

        /**
         * Check if the payer is also a beneficiary
         */

        boolean IfPayerIsBenificiary = checkIfPayerIsBenificiary(payer,
                beneficiary);

        /**
         * If the payer is beneficiary remove the payer from the list of
         * beneficiaries
         */

        if (IfPayerIsBenificiary) {

            consolidatedBenificiary.remove(payer);
        }

        /**
         * Calculate the amount to be payed back by the benificiary
         */

        amountToBePaid = amount / dividableCount;

        /**
         * List the details to be paid by the beneficiary to the payer
         */
        for (String benificiaryIndividual : beneficiary) {
            System.out.println(benificiaryIndividual + " should be paying "
                    + amountToBePaid + " to " + payer);
        }
    }

    /**
     * Method to check if the payer is a benificiary
     *
     * @param payer
     * @param beneficiary
     * @return
     */
    public boolean checkIfPayerIsBenificiary(String payer,
            List<String> beneficiary) {
        boolean IfPayerIsBenificiary = false;

        if (beneficiary.contains(payer)) {
            return true;
        }

        return IfPayerIsBenificiary;

    }

    /**
     * Set whether the amount is payable
     *
     * @param isPayable
     * @param amount
     * @param payDetails
     * @return
     */

    public PayDetails setPayableOrToBePayed(boolean isPayable, int amount,
            PayDetails payDetails) {
        if (isPayable) {
            payDetails.Payable = payDetails.Payable + amount;
        } else {
            payDetails.toBePayed = payDetails.toBePayed + amount;
        }

        return payDetails;
    }

    public static void main(String[] args) {
        SpendDetect spd = new SpendDetect();
        List<String> benificiaryList = new ArrayList<String>();
        benificiaryList.add("A");
        benificiaryList.add("B");
        benificiaryList.add("C");
        benificiaryList.add("D");

        spd.getPayDetails("A", 100, benificiaryList);
        benificiaryList.clear();

        benificiaryList.add("B");
        benificiaryList.add("C");
        spd.getPayDetails("D", 500, benificiaryList);
        benificiaryList.clear();

        benificiaryList.add("A");
        benificiaryList.add("B");
        benificiaryList.add("C");
        spd.getPayDetails("B", 300, benificiaryList);

    }
}

Comments

Inline commend are usually no JavaDoc comment (/* vs /**).
Please check your comments, if they add any new information, that is not covered by the method or variables names, e.g:

    /**
     * If the payer is beneficiary remove the payer from the list of
     * beneficiaries
     */

    if (IfPayerIsBenificiary) {

        consolidatedBenificiary.remove(payer);
    }

could be a oneliner to improve (opinion!) readability:

  if (ifPayerIsBenificiary) consolidatedBenificiary.remove(payer);

As mentioned by the other reviewers, and I can only stress this point: comments are not used to explain what you are doing but why.

Variables/Method

Variables starts lower case in java.

In java it is not that common to define/initialize variables at the beginning of the method. Just do it when you need them. (e.g amountToBePaid)

Naming a method getPayDetails and return void is confusing. Just call it print...

Generics

List<String> benificiaryList = new ArrayList<String>();

Since Java 7 you can omit the second <String> and use the diamond operator <>

Simplification

checkIfPayerIsBenificiary could be written as:

public boolean checkIfPayerIsBenificiary(String payer, List<String> beneficiary)
{
    return beneficiary.contains(payer);
}

If this don't need to be public, just inline the one usage. Your variables is telling everything IfPayerIsBenificiary in that place.

Unused code

setPayableOrToBePayed and outputMap are not used. If there is no code missing in your question, just delete it.

consolidatedBenificiary is redundant and can be inlined.

(to be continued after you completed your code)


Faster Way of Comparing Generic Sets The following extension method is the limiting factor in the performance of an application I am developing, according to Visual Studio 2012 Performance Analysis profiler.

Is there a faster way to compare two collections for set equality?

public static bool SetEqual<T>(this IEnumerable<T> source, IEnumerable<T> other, IEqualityComparer<T> comparer = null)
{
    if (source == null) {
        throw new ArgumentNullException("source");
    }

    if (other == null) {
        throw new ArgumentNullException("other");
    }

    ICollection<T> sourceCollection = source as ICollection<T> ?? source.ToArray();
    ICollection<T> otherCollection = other as ICollection<T> ?? other.ToArray();

    if (sourceCollection.Count != otherCollection.Count) {
        return false;
    }

    if (sourceCollection.Except(otherCollection, comparer).Any()) {
        return false;
    }

    if (otherCollection.Except(sourceCollection, comparer).Any()) {
        return false;
    }

    return true;
}

It seems you are iterating the sets a lot with the Except method and possibly the ToArray.

Consider iterating the sets at most once:

public static bool SetEqual<T>(this IEnumerable<T> source, IEnumerable<T> other, IEqualityComparer<T> comparer = null)
{
    if (source == null)
        throw new ArgumentNullException("source");

    if (other == null)
        throw new ArgumentNullException("other");

    if (source is ICollection &&
        other is ICollection &&
        ((ICollection)source).Count != ((ICollection)other).Count)
        return false;

    HashSet<T> set;
    if (comparer == null)
        set = new HashSet<T>(source);
    else
        set = new HashSet<T>(source, comparer);

    return set.SetEquals(other);
}

Source IEnumerable implementation.


Go command-line tool to do some simple byte manipulation I have some files containing 16-bit words in big-endian order, and a tool to process them that assumes little-endian order. I therefore need to swap the order of each pair of bytes in my files.

This is almost a one-liner in Python (data[0::2], data[1::2] = data[1::2], data[0::2]). However, since I need to do this manipulation often, I decided to write a standalone tool to do it. Also, for learning purposes, I decided to write this simple tool as my first Go program.

I look forward to any comments you may have about this code:

byteswap16.go

package main

import "fmt"
import "io/ioutil"
import "os"

func parseArgs(args []string) (string, string, error) {
    if len(args) != 3 {
        return "", "", fmt.Errorf("Usage: %s infile outfile", args[0])
    }

    return args[1], args[2], nil
}

func byteswap16(data []byte) ([]byte, error) {
    length := len(data)
    if length%2 != 0 {
        return nil, fmt.Errorf("Length is not a multiple of 2 bytes")
    }

    swapped := make([]byte, length)
    for i := 0; i < length; i++ {
        swapped[i] = data[i^1]
    }

    return swapped, nil
}

func run() error {
    infile, outfile, err := parseArgs(os.Args)
    if err != nil {
        return err
    }

    data, err := ioutil.ReadFile(infile)
    if err != nil {
        return err
    }

    swapped, err := byteswap16(data)
    if err != nil {
        return fmt.Errorf("%s: %v", infile, err)
    }

    if err := ioutil.WriteFile(outfile, swapped, 0666); err != nil {
        return err
    }

    return nil
}

func main() {
    if err := run(); err != nil {
        fmt.Fprintf(os.Stderr, "Error: %v\n", err)
        os.Exit(1)
    }
}

A couple of comments of my own:

As with the Python, the core algorithm is very little code, but it's written very differently - an explicit for loop over individual bytes rather than higher-level manipulation of many values at a time. I think I like the explicit loop more, because it's a more general solution - changing to byteswap32 wouldn't require much more than changing i^1 to i^3.
A common complaint about Go, but I don't really like the repetitive if err != nil in run. However, it does provide an obvious place to wrap err to add context, as is done after the call to the byteswap16 function. If exceptions were used instead, the code would be more concise but the error message would be worse, because I don't think I would catch and re-throw to add context.

In general, would this be considered good-quality, idiomatic Go code?

Several things come to mind right away:

I wouldn't make a custom parseArgs function, I'd use flag.Parse. For just a pair of positional arguments it might be overkill but it makes it trivial to add more options, usage output, etc. And either way, it's better to only access os.Args from main
Instead of fmt.Fprint to os.Stderr followed by os.Exit it's more idomatic to just use log.Fatal (or Fatalf or Fatalln).
Avoid reading and writing entire files with ioutil if you can do work while streaming data. If the files are guarenteed to be small it's just a bad habit but if someone ever runs your tool on gigabytes or terabytes of data they'll be in for unpleasant surprise.
In the same vein, I'd make your function API steam-able. Either by implementing an io.Reader wrapper or by a function that takes an io.Writer and an io.Reader (like io.Copy).

You could also implementing it as a transform.Transformer; but that's probably overkill, although perhaps an interesting exercise.
Typically such command line tools in unix act as pipes, that is, if not given file arguments they read from stdin and write to stdout. Either you only work with stdin/stdout and make callers use shell redirection if they want to work to work on files or you make the filenames optional. Just initialise an io.Reader (or io.ReadCloser) variable to os.Stdin and an io.WriteCloesr variable to os.Stdout. Then when processing the arguments, if provided, open the relevant files and change the variables. Your choice if you use positional arguments as you've done or option arguments (as dd does and as I did below). Make sure to check for errors closing the destination.
By the way, on unix you can use the standard dd command to do the byte swapping for you with the conv=swab operand:

dd conv=swab if=someinput_file of=someoutput_file

using files, or in a pipe like this:

bigendian_command | dd conv=swab | littleendian_command

A quick stab at implementing it this way gave me the following.

(Also available at https://gist.github.com/dchapes/9d795a04e471319abbc5ff016afbbee9
the gist also has your version as revision 1, and a version using a function with an io.Copy like signature as revision 2).

swab.go:

package main

import (
    "flag"
    "fmt"
    "io"
    "log"
    "os"
)

func main() {
    log.SetPrefix("swab: ")
    log.SetFlags(0)
    infile := flag.String("in", "", "input `path`, blank for stdin")
    outfile := flag.String("out", "", "output `path`, blank for stdout")
    flag.Usage = func() {
        fmt.Fprintf(flag.CommandLine.Output(),
            "Usage: %s [options]\n", os.Args[0],
        )
        flag.PrintDefaults()
    }
    flag.Parse()
    if flag.NArg() > 0 {
        flag.Usage()
        os.Exit(2) // To match the exit code flag.Parse uses.
    }

    var src io.ReadCloser = os.Stdin
    var dst io.WriteCloser = os.Stdout
    if *infile != "" {
        f, err := os.Open(*infile)
        if err != nil {
            log.Fatal(err)
        }
        src = f
    }
    // Closing the input isn't strictly required in main
    // nor for stdio, but it's a good habit. No need to
    // check any error; we rely on Read reporting errors of interest.
    defer src.Close()

    if *outfile != "" {
        f, err := os.Create(*outfile)
        if err != nil {
            log.Fatal(err)
        }
        dst = f
    }

    if _, err := io.Copy(dst, NewSwabReader(src)); err != nil {
        // Not this calls os.Exit so no defers get run
        // and we don't close the output either, not
        // an issue from main.
        log.Fatal(err)
    }
    if err := dst.Close(); err != nil {
        log.Fatal(err)
    }
}

type SwabReader struct {
    r        io.Reader
    b        byte // extra byte, not yet swapped
    haveByte bool // true if b is valid
    err      error
}

// NewSwabReader returns an io.Reader that reads from r
// swapping adjacent bytes. The trailing odd byte, if any,
// is left as-is.
func NewSwabReader(r io.Reader) *SwabReader {
    return &SwabReader{r: r}
}

func (sr *SwabReader) Read(p []byte) (n int, err error) {
    if len(p) == 0 || sr.err != nil {
        return 0, sr.err
    }
    i := 0
    if sr.haveByte {
        // Copy in the previous saved byte.
        p[0] = sr.b
        i = 1
        //sr.haveByte = false // not strictly required
    }
    n, sr.err = sr.r.Read(p[i:])
    n += i
    p = p[:n]
    for i := 1; i < len(p); i += 2 {
        p[i-1], p[i] = p[i], p[i-1]
    }
    // Remove and save any non-swapped trailing odd byte.
    if sr.err == nil {
        if sr.haveByte = (n&1 != 0); sr.haveByte {
            n--
            sr.b = p[n]
            //p = p[:n] // not strictly required
        }
    }
    return n, sr.err
}

And a simple test,
swab_test.go:

package main

import (
    "io"
    "strings"
    "testing"
    "testing/iotest"
)

var readFilters = []struct {
    name string
    fn   func(io.Reader) io.Reader
}{
    {"", nil},
    {"DataErrReader", iotest.DataErrReader},
    {"HalfReader", iotest.HalfReader},
    {"OneByteReader", iotest.OneByteReader},
    //{"TimeoutReader", iotest.TimeoutReader},
}

func TestSwab(t *testing.T) {
    const sz = 32<<10 + 1
    cases := []struct{ in, out string }{
        {"", ""},
        {"a", "a"},
        {"ab", "ba"},
        {"abc", "bac"},
        {"abcd", "badc"},
        {strings.Repeat("\x01\x80", sz) + "x",
            strings.Repeat("\x80\x01", sz) + "x"},
    }
    var dst strings.Builder
    var r io.Reader
    for _, rf := range readFilters {
        for _, tc := range cases {
            r = strings.NewReader(tc.in)
            if rf.fn != nil {
                r = rf.fn(r)
            }
            dst.Reset()
            //t.Logf("swabbing %s %.16q", rf.name, tc.in)
            //r = iotest.NewReadLogger("<<< src", r)
            n, err := io.Copy(&dst, NewSwabReader(r))
            if err != nil {
                t.Errorf("swab on %s %q failed: %v",
                    rf.name, tc.in, err,
                )
                continue
            }
            if want := int64(len(tc.out)); n != want {
                t.Errorf("swab on %s %q returned n=%d, want %d",
                    rf.name, tc.in, n, want,
                )
            }
            if got := dst.String(); got != tc.out {
                t.Errorf("swab on %s %q\n\tgave %q\n\twant %q",
                    rf.name, tc.in, got, tc.out,
                )
            }
        }
    }
}


Private methods and properties in jQuery "lightweight start" pattern For non-plugin code, I generally use the module pattern, but I'm working on a jQuery plugin and decided to try out the "lightweight start" pattern. The problem is, I find myself needing $.proxy when calling private methods to ensure the this context.

Is there a better way to structure the code in order to avoid $.proxy, while ensuring the private methods have access to the init() method's this context? Where exactly should I define private properties?

Jsfiddle example without $.proxy ... also have a jsfiddle example using $.proxy

(function($, window, document, undefined) {
var pluginName = "myPlugin",
    defaults = {
        opt1: "#opt1",
    };

function Plugin(element, options) {
    this.element = $(element);
    this.options = $.extend( {}, defaults, options);
    this._defaults = defaults;
    this._name = pluginName;
    this.init();
}

Plugin.prototype.init = function() {
    this.counter = 1;
    $("div").each(privateMethod1);
    $("body").on("click", this.options.opt1, event1);
};

function privateMethod2(i, div) {
    // "this" context is Window, not Plugin
    $(div).append(i);
    this.element.append(this.counter);
};

function privateMethod1(i, el) {
    // "this" context is Window, not Plugin
    this.counter += 1; // undefined, but defined in init()
    privateMethod2(i, el);
};

function event1(ev, el) {
    this.counter += 1; // undefined, but defined in init()
    privateMethod1(this.counter, el);
};

$.fn[pluginName] = function(options) {
    return this.each(function() {
        if (!$.data(this, "plugin_" + pluginName)) {
            $.data(this, "plugin_" + pluginName, new Plugin(this, options));
        }
    });
};
}(jQuery));

Edit:

What I learned. This pattern appears to actually make all methods private. This is because the if (!$.data(this, "plugin_" + pluginName)) { has no else block to handle elements that already have a reference to the plugin. To allow a method to be called, check out https://stackoverflow.com/a/14128485/536734.

You could use a closure to create your own proxy.

Plugin.prototype.init = function() {
    var self = this;
    rows.each(function (groupIndex, group) {
        self.initGroup(groupIndex, group);
    });
    this.container.on("click", this.options.addBtnId, function (ev) {
        self.addGroup(ev);
    });
};

Why are initGroup and addGroup using $.proxy? They are already being called with the correct this context, and rebinding to the same context changes nothing.

Edit: Instead of using plain functions to implement "private methods", promote them to proper object methods and mark them private using JSDoc. This will save you those extra calls to $.proxy.

/** @private */
Plugin.prototype.initGroup = function(groupIndex, group) {
    this.addFieldIds(groupIndex, group);
};

/** @private */
Plugin.prototype.addGroup = function(ev) {
    this.initGroup(this.maxGroupIndex, newGroup);
};

In my opinion, forcing developers to jump through hoops just to hide private methods is a net loss.


Hackerrank challenge - Dictionaries and Maps in Erlang This question is really similar to an existing one in Ruby. So the task is also the same: Day 8: Dictionaries and Maps!

The problem is that on Hackerrank my solution only passes the first and last testcases. All the other ones between them are timing out (>12s!). I tried to use map instead of dict and also read the queries first into a list and process them separately as was suggested in the discussion. I have a feeling it goes to infinite recursion because I'm doing the line read/strip wrong.

-module(solution).
-export([main/0]).

main() ->
    { ok, [N]} = io:fread("", "~d"),
    Dict = read_dictionary(N, dict:new()),
    queries(Dict).

read_dictionary(0, D) -> D;
read_dictionary(N, D) ->
    Name = stripped_next_line(),
    Number = stripped_next_line(),
    read_dictionary(N-1, dict:store(Name, Number, D)).

queries(Dict) ->
    queries(Dict, io:get_line("")).
queries(_, eof) -> true;
queries(Dict, Name) ->
    find_number_of(strip_line(Name), Dict),
    Next = io:get_line(""),
    queries(Dict, Next).

find_number_of(Name, Dict) ->
    out(dict:find(Name, Dict), Name).

out({ok, Number}, Name) -> io:format("~s=~s~n", [Name, Number]);
out(error, _) -> io:format("Not found~n").

strip_line(Line) -> string:strip(Line, both, $\n).
stripped_next_line() -> strip_line(io:get_line("")).

There are two things you can do differently that would help:

read all the input at once, and then process it
store values such that they can be output directly, without formatting

The code below first reads all the input in chunks of 4096 characters, accumulating a list of chunks. It then reverses the list, flattens the list into a large string, and splits the entire string on newlines. After that, it converts the first element of the resulting list to an integer N, and then processes N names and numbers from the remainder of the list by storing them in a map. The value stored isn't just the phone number, but the required output string "<name>=<number>\n". Once all the entries have been processed, the rest of the input is queries, which are then processed one-by-one against the map. Query processing uses maps:get/3 with the string "Not found" used as the default for lookups that fail. This allows the return value from maps:get/3 to be unconditionally output directly to stdout.

-module(solution).
-export([main/0]).

main() ->
    queries(store(read_all())).

read_all() ->
    read_all([], io:get_chars("", 4096)).
read_all(In, eof) ->
    [H|T] = string:tokens(lists:flatten(lists:reverse(In)), "\n"),
    {list_to_integer(H), T};
read_all(In, Data) ->
    read_all([Data|In], io:get_chars("", 4096)).

store(Data) ->
    store(Data, #{}).
store({0, Queries}, M) ->
    {Queries, M};
store({N, [Name,Number|T]}, M) ->
    store({N-1, T}, maps:put(Name, lists:flatten([Name,"=",Number,"\n"]), M)).

queries({[], _}) ->
    ok;
queries({[Query|Queries], M}) ->
    io:put_chars(maps:get(Query, M, "Not found\n")),
    queries({Queries, M}).

I created an input file containing 9000 entries to be stored and used it to measure this solution to be about 150 times faster than solutions that read the input line by line, as measured by running the solution on the command line in a bash shell like this:

time erl -noshell -s solution main -s init stop < input

and subtracting out the overhead of starting and stopping the Erlang VM, measured like this:

time erl -noinput -s init stop

On my machine, this overhead is ~1.20s. The original solution for input with 9000 entries and 3 lookups takes ~6.25s, while the solution I've posted here runs in ~1.23s, which means it imposes hardly any overhead over that of the VM startup and shutdown.


XQuery compare order of two sequences I have written a function in XQuery 3.0 (using BaseX) which tests a base sequence against a test sequence, with the goal of determining if the items in the test sequence appear in the same order as the base sequence.

This is best illustrated with an example

E.G.
let $baseSequence := ('a','b','c'..'x','y','z')

Tested against

('a','b','c') => true appears in the same order in $baseSequence

('a','b','a') => false is not in the same order in $baseSequence

('c','h','x','y') => true appears in the same order in $baseSequence

I would like to know if there are any improvements on the function I have written for this, or is there a better way of doing this?

declare function local:testOrder($baseSequence, $testSequence, $position) {
    let $itemPosition :=
    if(count($testSequence) = 0) then (32768) else (index-of($baseSequence, head($testSequence)))
  return
    if ($itemPosition > $position) then (
      local:testOrder($baseSequence, tail($testSequence), $itemPosition)
    ) else (
      count($testSequence) = 0
    )
};

(:Test cases:)
let $baseSequence := ('a','b','c','d','e','f','g','h','i','j','k','l','m','n','o','p','q','r','s','t','u','v','w','x','y','z')
let $testAgainst := ('a','c','e','h','w')
return local:testOrder($baseSequence, $testAgainst, 0)

Would return true

let $baseSequence := ('a','b','c','d','e','f','g','h','i','j','k','l','m','n','o','p','q','r','s','t','u','v','w','x','y','z')
    let $testAgainst := ('f','d','l','m','n')
    return local:testOrder($baseSequence, $testAgainst, 0)

Would return false

There is a more concise possible approach if you, first, convert the test sequence into the corresponding positions in the base sequence, then, substract each position to the next one (replace "ge 0" with "gt 0" if duplicates are not allowed):

declare function local:testOrder2($baseSequence, $testSequence) {
    let $positions := $testSequence ! index-of($baseSequence, .)
    return min(for $pos at $index in tail($positions) return $pos - $positions[$index]) ge 0
};

Your recursive approach will stop at the first disorder detection so, depending on inputs, it might be faster!


Binary search that gets ALL matching results I am implementing a binary search to retrieve all matches of a specified value. Instead of a regular binary search that would just return the first match, I want to get that first match AND all matches above and below it in the set of objects.

For example, I have an array of objects that are sorted by the name property of each object in alphabetical order, and the first letter of that name property is what I use for comparison of the value provided.

The object would look like this (using fake values):

[
    { name: 'Apple',      id: 1, ...},
    { name: 'Apple',      id: 2, ...},
    { name: 'Basketball', id: 3, ...},
    { name: 'Basketball', id: 4, ...},
    { name: 'Cooking',    id: 5, ...},
    { name: 'Cooking',    id: 6, ...},
    { name: 'Dinosaurs',  id: 7, ...},
    ...
]

The list goes on all the way to Z, and each set of letters has probably 100+ results so there is a big set of objects here.

My code achieves what I want, but I feel as though I wrote way more code than I needed, but I've already refactored a few times and can't seem to shrink it much more.

A simple for loop could have achieved this for me and shrunk the code down to like 10 lines, but it would be essentially looping through a set of 2000+ items even when it only needs the first 100..

Here's the code (the main function is _binarySearchAll at the bottom):

getBrands(letter) {
    let brands = this.brands,
        _firstLetter = brand => {
            if ('hasOwnProperty' in brand && brand.hasOwnProperty('name')) {
                return brand.name.slice(0, 1).toLowerCase();
            }

            return false;
        },
        _traverse = (up, arr, i) => {
            let store = [],
                direction = up ? 1 : -1;

            while (_firstLetter(arr[i]) === letter) {
                store.push(arr[i]);

                i = i + direction;

                if (typeof arr[i] === 'undefined') {
                    break;
                }
            }

            return store;
        },
        _each = (arr, callback) => {
            let i = 0,
                len = arr.length - 1;

            while (++i < len) {
                callback(arr[i]);
            }
        },
        _compare = (brand, letter) => {
            let brandLetter = _firstLetter(brand);

            if (brandLetter === letter) {
                return 0;
            } else if (brandLetter < letter) {
                return 1;
            } else if (brandLetter > letter) {
                return -1;
            }
        },
        _all = (arr, i) => {
            let store = [],
                down = false,
                up = true;

            _each(_traverse(down, arr, i - 1), match => {
                store.push(match);
            });

            _each(_traverse(up, arr, i + 1), match => {
                store.push(match);
            });

            store.push(arr[i]);

            return store;
        },
        _binarySearchAll = (arr, val, compare) => {
            let start = 0,
                stop = arr.length - 1,
                mid = (start + stop) >> 1;

            while (start < stop) {
                let result = compare(arr[mid], val);

                if (result === 0) {
                    return _all(arr, mid);
                } else if (result > 0) {
                    start = mid + 1;
                } else if (result < 0) {
                    stop = mid - 1;
                }

                mid = (start + stop) >> 1;
            }
        };

    return _binarySearchAll(brands, letter, _compare);
}

Any help would be appreciated. If you want to test out the function, copy and paste it into Chrome Dev tools (or any other JS playground that supports ES6) and just past a similar array of objects as provided in my example. Try to have like 20 of each letter, then call getBrands('a') or some other letter, and you'll get an array of objects all starting with the first letter 'a'.

Unless I wrongly understood your question (like if you want to use binary search), it seems you merely look for the fastest solution.

I was curious to try with what I found the simplest method, using a filter:

function getBrandsSimple(letter) {
  return brands.filter( item => item.name.charAt(0).toLowerCase() == letter);
}

Then I created a big array of 3000 items from 15 brands with 200 ids each, and tested your solution getBrands against the simple one above getBrandsSimple.
The results are:

not surprisingly the time needed by getBrands differ considerably depending on the choosen letter (and also depending on the array order, see below)
obviously, at the opposite the time needed by getBrandsSimple is rather constant
in the worst case I noticed, getBrandsSimple needs less than 2x the time needed by getBrands
in other cases, getBrands may need till 4x the time needed by getBrandsSimple

In additon, getBrands often returns wrong results.
From the definition of my test array, each case should return exactly 200 items, while getBrands frequently returns only 196 items, and sometimes only 1.

Moreover, I added an option to shuffle the array: then getBrands returns most often only 1 result or no result at all (undefined)!
I didn't dive into your code to look for what turns wrong.

You may check all that using this snippet:

var brandsList = [
      'Apple',
      'Basketball',
      'Cooking',
      'Dinosaurs',
      'ES6',
      'Features',
      'Golf',
      'Highlighting',
      'Informatics',
      'JSON',
      'KiloBytes',
      'Lambda',
      'More...',
      'Nothing',
      'Original',
    ],
    filler = new Array(200).fill(''),
    brands = brandsList.reduce( (result, brandName) =>
      result.concat(
        filler.map((item, index) => {
          return {name: brandName, id: index + 1};
        })
      )
    , []);

getBrandsSimple = function(letter) {
  return brands.filter( item => item.name.charAt(0).toLowerCase() == letter);
}

getBrands = function(letter) {
    let brands = this.brands,
        _firstLetter = brand => {
            if ('hasOwnProperty' in brand && brand.hasOwnProperty('name')) {
                return brand.name.slice(0, 1).toLowerCase();
            }
            return false;
        },
        _traverse = (up, arr, i) => {
            let store = [],
                direction = up ? 1 : -1;
            while (_firstLetter(arr[i]) === letter) {
                store.push(arr[i]);
                i = i + direction;
                if (typeof arr[i] === 'undefined') {
                    break;
                }
            }
            return store;
        },
        _each = (arr, callback) => {
            let i = 0,
                len = arr.length - 1;
            while (++i < len) {
                callback(arr[i]);
            }
        },
        _compare = (brand, letter) => {
            let brandLetter = _firstLetter(brand);
            if (brandLetter === letter) {
                return 0;
            } else if (brandLetter < letter) {
                return 1;
            } else if (brandLetter > letter) {
                return -1;
            }
        },
        _all = (arr, i) => {
            let store = [],
                down = false,
                up = true;
            _each(_traverse(down, arr, i - 1), match => {
                store.push(match);
            });
            _each(_traverse(up, arr, i + 1), match => {
                store.push(match);
            });
            store.push(arr[i]);
            return store;
        },
        _binarySearchAll = (arr, val, compare) => {
            let start = 0,
                stop = arr.length - 1,
                mid = (start + stop) >> 1;
            while (start < stop) {
                let result = compare(arr[mid], val);
                if (result === 0) {
                    return _all(arr, mid);
                } else if (result > 0) {
                    start = mid + 1;
                } else if (result < 0) {
                    stop = mid - 1;
                }
                mid = (start + stop) >> 1;
            }
        };
    return _binarySearchAll(brands, letter, _compare);
}

function exec(funcName, func, letter) {
  console.time(funcName);
  result = func(letter);
  console.timeEnd(funcName);
  if (result) {
    // console.log(result.length + ' items:', result);
    console.log(result.length + ' items');
  } else {
    console.log(result);
  }
}

function shuffle(a) { // (thanks to http://stackoverflow.com/a/6274381/3415269)
    var j, x, i;
    for (i = a.length; i; i--) {
        j = Math.floor(Math.random() * i);
        x = a[i - 1];
        a[i - 1] = a[j];
        a[j] = x;
    }
}

var letter = prompt('Enter a letter', 'j');
if (confirm('Shuffle the brands array?')) {
  shuffle(brands);
}
exec('getBrands', getBrands, letter);
exec('getBrandsSimple', getBrandsSimple, letter);


Validate the order of a List by custom strings I have a test where I have to validate the order of items in a List based on a pre-defined hierarchy that is being given to me as part of my assignment. Here it is below.

Cat
Dog
Horse
Elephant

The items in the list would only ever be those four types, but the size of list is not limited. The list will be passed in as part of the method. There can be multiple of each or none at all. The below two list would be valid:

Cat
Cat
Dog
Elephant

This would not.

Elephant
Dog
Horse
Cat

Below is what I have so far, and I feel as though there is a much more elegant solution. In particular, I would like to minimize the amount of bools and if statements I am using.

public bool ValidateAnimalOrder(List<string> listOfAnimals)
{
    bool catExists = false;
    bool dogExists = false;
    bool horseExists = false;
    bool elephantExists = false;

    for (int x = 0; x < listOfAnimals.Count; x++)
    {
        if (listOfAnimals[x] == "Cat")
        {
            catExists = true;
            if(dogExists || horseExists || elephantExists)
            {
                return false;
            }
        }
        if (listOfAnimals[x] == "Dog")
        {
            dogExists = true;
            if (horseExists || elephantExists)
            {
                return false;
            }
        }
        if (listOfAnimals[x] == "Horse")
        {
            horseExists = true;
            if (elephantExists)
            {
                return false;
            }
        }
        if (listOfAnimals[x] == "Elephant")
        {
            elephantExists = true;
        }

    }

    return true;
}

Dictionary

I'd prefer to store animal names and their "values" in a dictionary.

private static readonly Dictionary<string, int> AnimalValues = new Dictionary<string, int>
        {
            { "Cat", 1 },
            { "Dog", 2 },
            { "Horse", 3 },
            { "Elephant", 4 }
        };

Then the validation method will look like:

public static bool IsValidAnimalOrder(IEnumerable<string> animalNames)
{
    int prevValue = 0;
    foreach (string animalName in animalNames)
    {
        int curValue;
        if (!AnimalValues.TryGetValue(animalName, out curValue))
        {
            // If the animal's name is absent in the dictionary.
            return false;
        }
        if (curValue < prevValue)
        {
            // If the current animal's value less than the previous one.
            return false;
        }
        prevValue = curValue;
    }
    return true;
}

switch

Alternative way is to use the switch operator instead of a dictionary as follows:

public static bool IsValidAnimalOrder(IEnumerable<string> animalNames)
{
    int prevValue = 0;
    foreach (string animalName in animalNames)
    {
        int curValue;
        switch (animalName)
        {
            case "Cat":
                curValue = 1;
                break;
            case "Dog":
                curValue = 2;
                break;
            case "Horse":
                curValue = 3;
                break;
            case "Elephant":
                curValue = 4;
                break;
            default:
                // If the animal's name is not listed above.
                return false;
        }
        if (curValue < prevValue)
        {
            // If the current animal's value less than the previous.
            return false;
        }
        prevValue = curValue;
    }
    return true;
}

IEnumerable<T> problem

As @Mat's Mug♦ mentioned, IEnumerable<T> doesn't guarantee the order of its elements. So you could change argument type to IList<T> or IReadOnlyList<T>. But this will reject another enumerations with the specified order: IOrderedEnumerable<T> and IOrderedQueryable<T>.
As a workaround you could make the method above private and declare public overloads accepting all the suitable enumeration types above as argument:

public static bool IsValidAnimalOrder(IReadOnlyList<string> animalNames)
{
    return IsValidAnimalOrder((IEnumerable<string>)animalNames);
}

public static bool IsValidAnimalOrder(IOrderedEnumerable<string> animalNames)
{
    return IsValidAnimalOrder((IEnumerable<string>)animalNames);
}

public static bool IsValidAnimalOrder(IOrderedQueryable<string> animalNames)
{
    return IsValidAnimalOrder((IEnumerable<string>)animalNames);
}


simple transaction process I ask if the following code need some refactoring :

public static int InsertBonus(DataTable dt, int month, int year)
{

    int affectedRow = -1;
    using (IfxConnection con = new IfxConnection(ConfigurationManager.ConnectionStrings["xxx"].ToString()))
    {
        using (IfxTransaction tran = con.BeginTransaction())
        {
            try
            {
                StringBuilder cmdTxt = new StringBuilder();

                cmdTxt.Append(" DELETE FROM rh7uiw WHERE bonus_mon = ? AND bonus_year = ? ");
                using (var myIfxCmd = new IfxCommand(cmdTxt.ToString(), con))
                {
                    myIfxCmd.CommandType = CommandType.Text;
                    myIfxCmd.Parameters.Add("bonus_mon", IfxType.Integer);
                    myIfxCmd.Parameters.Add("bonus_year", IfxType.Integer);
                    if (con.State == ConnectionState.Closed)
                    {
                        con.Open();
                    }
                    myIfxCmd.Parameters[0].Value = ((object)month) ?? DBNull.Value;
                    myIfxCmd.Parameters[1].Value = ((object)year) ?? DBNull.Value;
                    affectedRow = myIfxCmd.ExecuteNonQuery();
                }

                cmdTxt.Length = 0;
                cmdTxt.Append(" INSERT INTO rh7uiw (emp_num,bonus_year,bonus_mon,bonus_value,bonus_no,rr_code) VALUES(?,?,?,?,?,?) ");

                using (var myIfxCmd = new IfxCommand(cmdTxt.ToString(), con))
                {
                    myIfxCmd.CommandType = CommandType.Text;
                    myIfxCmd.Parameters.Add("emp_num", IfxType.Integer);
                    myIfxCmd.Parameters.Add("bonus_year", IfxType.Integer);
                    myIfxCmd.Parameters.Add("bonus_mon", IfxType.Integer);
                    myIfxCmd.Parameters.Add("bonus_value", IfxType.Integer);
                    myIfxCmd.Parameters.Add("bonus_no", IfxType.Integer);
                    myIfxCmd.Parameters.Add("rr_code", IfxType.Integer);
                    foreach (DataRow r in dt.Rows)
                    {
                        if (con.State == ConnectionState.Closed)
                        {
                            con.Open();
                        }
                        myIfxCmd.Parameters[0].Value = (r["emp_num"]) ?? DBNull.Value;
                        myIfxCmd.Parameters[1].Value = (r["year"]) ?? DBNull.Value;
                        myIfxCmd.Parameters[2].Value = (r["month"]) ?? DBNull.Value;
                        myIfxCmd.Parameters[3].Value = (r["bonus_al"]) ?? DBNull.Value;
                        myIfxCmd.Parameters[4].Value = 1;
                        myIfxCmd.Parameters[5].Value = 1;
                        affectedRow = myIfxCmd.ExecuteNonQuery();
                    }
                }
                tran.Commit();
                con.Close();
                con.Dispose();
                return affectedRow;

            }
            catch
            {
                tran.Rollback();
                throw;
            }
        }
    }
}

I totally agree with Guffa, but I want to add some things: I don't like so long methods, because they are really hard to read, especially for other people which might maintain you code in the future. So I would create some private methods with meaningful names. So everything within the try block, I would change to something like this (pseudocode):

try{
   ExcecuteDelete(parameters, ...)
   ExcecuteInsert(parameters,...)
}

So you have three short instead of one long method.
The query text ("DELETE FROM rh7uiw WHERE bon...") seems to be constant, so I would like to make it a private static constant class variable.

Furthermore, don't use any prefix like my, our and so on, as far as they don't have a special meaning in the context they are used for variable names. Simply call them ifxCommand, command or better: deleteCommand and insertCommand.

By the way, why don't you use a updateCommand? If the affected rows are 0 after a update, you know you have to insert new stuff. But in most cases you have only one database query, instead of always two.


LeetCode: Sum of Two Integers C# https://leetcode.com/problems/sum-of-two-integers/

  Calculate the sum of two integers a and b, but you are not allowed to
  use the operator + and -.

  Example 1:

  Input: a = 1, b = 2 Output: 3 Example 2:

  Input: a = -2, b = 3 Output: 1

using Microsoft.VisualStudio.TestTools.UnitTesting;

namespace MathQuestions
{
    /// <summary>
    /// https://leetcode.com/problems/sum-of-two-integers/
    /// </summary>
    [TestClass]
    public class SumOfTwoIntegersTest
    {
        [TestMethod]
        public void ExampleTest()
        {
            int a = 1;
            int b = 2;
            int expected = 3;
            Assert.AreEqual(expected, GetSum(a,b));
        }

        [TestMethod]
        public void ExampleTest2()
        {
            int a = 5;
            int b = 7;
            int expected = 12;
            Assert.AreEqual(expected, GetSum(a, b));
        }

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

            while (b != 0)
            {
                int carry = a & b;
                a = a ^ b;
                b = carry << 1;
            }

            return a;

        }
    }
}

Please review for performance

Your code is pretty much perfect, the only problem is that you chose to sacrifice some performance in most cases (non 0 operands) for the sake of making the rare case faster. Here's a slight improvement based on the assumption that most of the times none of the operands will be 0.

The trick is to keep it efficient in the case of 0 without wasting time on special checks that don't help the actual calculation. If the carry is 0 then you know you're done. Storing the output (a ^ b) in the same variable removes the need for an if statement for returning b instead of a.

Also 1 bitwise operation and assignment is saved by shifting the carry at the start of the loop, so there is no extra unused shift at the end. (note: it would slow the function down in case of overflow, but that's a rare case and it's expected that the programmer will avoid overflow anyway)

public int GetSum(int a, int b)
{
    int carry = a & b;
    a = a ^ b;

    while (carry != 0)
    {
        b = carry << 1;
        carry = a & b;
        a = a ^ b;
    }

    return a;
}


getRandom() - Randomize array and get result I've written a function that allows you to pass an array of values, which is then shuffled and returned based on a parameter $preserveArray.

This is the code:

function getRandom($arr, $preserveArray = false, $shuffles = 1) {
    if(empty($arr)) return false;
    if($preserveArray) {
        $tmp = array();
        foreach($arr as $key => $value) {
            $tmp[] = array('key' => $key, 'value' => $value);
        }
        $arr = $tmp;
    }
    for($x = 1; $x <= $shuffles; $x++) {
        if(!shuffle($arr)) {
            return false;
        }
    }
    if($preserveArray) {
        $tmp = array();
        foreach($arr as $value) {
            $tmp[$value['key']] = $value['value'];
        }
        $arr = $tmp;
        return $arr;
    }
    return $arr[0];
}

So you can pass any array as the first parameter that you want randomized, then the 2nd parameter determines how the data is returned, then the 3rd parameter tells the function how many times to shuffle the array.

I've written this because I think it will be useful for people on StackOverflow, I see a lot of questions trying to figure out how to get a random value from an array.

The $preserveArray parameter tells the function to either only return a single array element if false, or return the whole array (with keys in-tact) if true.

As you can see in my code, I have to check for the 2nd parameter twice to do actions before and after the actual array shuffle. I wanted to see if anyone had any advice or knows of any ways I can improve my code.

I've been posting a lot of my code here lately, and I really enjoy how much I'm able to learn from answers. Thanks in advance!

This function shouldn't be needed.

I have a feeling that you wrote it for some imaginary people and without any particular use case in mind. And out of the false assumptions too. Neither more than one shuffle is required and you never need to shuffle an associative array. So just raw shuffle() is enough.

And you can always use $arr[array_rand($arr)]; instead of getRandom($arr); to get just a single value.


Finding longest substring containing k distinct characters Below is my method to find the longest substring having k distinct characters.
I have used Set to keep track of distinct characters.

public static int maxLengthSubstring(String str,int k) {
    int i=0;
    int j=i;
    int max=0;
    //use set to keep track of distinct characters
    Set<Integer> set=new TreeSet<>();
    char arr[] = str.toCharArray();
    int len = arr.length;
    while(i<len && j<len){
        int c=arr[j];
        set.add(c);
        //if the number of distinct characters exceed the given limit
        //then compute the length of substring containing them
        if(set.size()>k){
            int substrLen=Math.abs(i-j);
            if(substrLen>max){
                //update the length of maximum substring found
                max=substrLen;
            }
            //move to the next character in the array
                i=i+1;
                j=i;
            //clear the set containing previously found distinct characters
                set.clear();
                continue;
            }
        //if set contains the exact number for distinct characters
        //then store the length of the substring and move to next iteration
        if(set.size()==k){
            int substrLen = Math.abs(i-(j+1));
            if(substrLen>max){
                //update the length of maximum substring found
                max=substrLen;
            }
        }
        j++;
    }
    return max;
}

The maximum length of input string can be up to million characters. The above method works fine for smaller strings but slows down if input string is large. How can I tweak my logic to improve running time for large strings?

A few test cases:

Input:

str="zxzxzxzxzx cvcvcvcvcvcvcvcvcv"
k=2
Output=18

Input:

str="aaaaaaaa abcabc aaaabbbbaaaabbbbbbabaa"
k=3
Output=23 (longest substring in this case is " aaaabbbbaaaabbbbbbabaa" including space character)

//use set to keep track of distinct characters
Set<Integer> set=new TreeSet<>();

Use spaces, they're free. According to conventions, there should be spaces around each operator. If you want to use them more sparingly, it's your choice, but think about how they separate things. You code looks like composed of three things

Set<Integer>   set=new   TreeSet<>();

which is rather confusing.

You write "keep track of distinct characters" and declare Set<Integer>. Why not Set<Character>?

Don't use TreeSet unless you need it. HashSet is faster.

Don't call it set, when you can call it e.g. foundCharacters.

char arr[] = str.toCharArray();

Naming.

while(i<len && j<len){

Spacing. Always prefer "for" loops as they're much clearer. They also allow you to declare variables in a smaller scope. I'd go for

for (int i=0, j=0; i<len && j<len; ++j) ...

(add spaces to adhere to conventions), but you're modifying j somewhere in the middle (which is unexpected with a "for" loop). So feel free to stick with "while".

        int substrLen=Math.abs(i-j);

This looks like you lost track of what your variables do. I guess, j>=i always holds, but you should know. You may want to call them start and end.

        int substrLen = Math.abs(i-(j+1));

This makes me suspect. Why do you need a different expression for computing the same thing?

Optimizations

  How can I tweak my logic to improve running time for large strings?

What the problem? You run through the string until you find out that they're too many characters. Then you throw away all information collected so far and start over at the next index. This implies a complexity of \$O(n^2)\$, which is way too slow for \$n=10^6\$.

When you find out that you collected too many distinct characters, you could move the start index until their number gets smaller again. For this, a Set is insufficient, as you have to know, when you get rid of the last occurrence of a given character.

So instead of remembering what characters you ran across, you need to count their occurrences. A Map<Character, Integer> would do. Guava Multiset<Character> would be even better as a multiset is just like a set but allowing duplicates.

But the best solution is an array. As there are just \$2^{16}\$ characters (let's ignore surrogates), you can keep track of them in new int[Character.MAX_VALUE+1]. This gives you a nice speed bonus of maybe factor 10 (it's just a bonus, it would not help you with the quadratic complexity and it's most probably not needed with the new algorithm).

Just a snippet to get you started:

private int distinctChars;
private int[] counts = new int[Character.MAX_VALUE+1];

private void addCharacter(char c) {
    if (counts[c] == 0) {
        distinctChars++;
    }
    counts[c]++;
}

private void removeCharacter(char c) {...}

public int maximumLength(String str, int limit) {
     ...
     for (int i=0, j=0; j<len; ) {
         if (distinctChars <= limit) {
             max = Math.max(max, j-i);
             addCharacter(str[j++]);
         } else {
             removeCharacter(str[i++]);
         }
     }
     ...
}