Python coin flipper with GUI I am doing some simple projects in an attempt to get good at programming, this is my first GUI would love some feedback, and some guidelines.

from tkinter import *
import random

root = Tk()
root.resizable(False, False)

root.title('Coinflipper')

topframe = Frame(root)
topframe.pack()
botframe = Frame(root)
botframe.pack(side=BOTTOM)
midframe = Frame(root)
midframe.pack()

choice = Label(topframe, text="Enter the number of flips: ")
choice.grid(row=1)
ent = Entry(topframe)
ent.grid(row=1, column=2)

clickit = Button(botframe, text="FLIP THE COIN!!!")
clickit.pack()

out = Text(midframe, width=15, height=1)
out2 = Text(midframe, width=15, height=1)
out.grid(row=1, column=1, columnspan=3)
out2.grid(row=2, column=1, columnspan=3)

def flipy(event):
    guess = ent.get()
    heads = []
    tails = []

    if guess == '' or guess == str(guess):
        out.delete(1.0, "end-1c")
        out.insert("end-1c", 'Invalid')

    for flips in range(int(guess)):
        out.delete(1.0, "end-1c")
        out2.delete(1.0, "end-1c")
        random_number = random.randint(1, 2)
        if random_number == 1:
            heads.append("Heads")
        elif random_number == 2:
            tails.append("Tails")

    out.insert("end-1c", len(tails))
    out.insert("end-1c", " -TAILS")
    out2.insert("end-1c", len(heads))
    out2.insert("end-1c", " -HEADS")

clickit.bind("<Button-1>", flipy)

root.mainloop()

You are abusing the heads & tails lists into making simple counters.

heads = []
tails = []

for flips in range(int(guess)):
    random_number = random.randint(1, 2)
    if random_number == 1:
        heads.append("Heads")
    elif random_number == 2:
        tails.append("Tails")

len(tails)
len(heads)

This could be replaced with simply:

heads = 0
tails = 0

for flips in range(int(guess)):
    random_number = random.randint(1, 2)
    if random_number == 1:
        heads += 1
    else:
        tails += 1


Generate prime factorisations of factorial numbers The code was written to generate the factorisation of numbers into primes in ascending order. E.g 3! ='2 * 3', 4!='2^3 * 3'. When I have huge numbers such as 100!, the run time becomes a problem. I want to have a code which will run in less time.
from math import floor,sqrt,factorial
from decimal import Decimal
def prime(x):
   if x==2 or x==3 or x==5:
      return True
   if (x%2==0) or (x%3==0) or (x%5==0) or (x==1):
     return False
   for i in range(6,floor(sqrt(x)),6):
      if x%(i+1)==0:
         return False
      elif i%(i+5)==0:
         return False
   return True
def decomp(n):
   y=factorial(n)
   if y<7:
      pri =list(filter(prime,range(1,y+1)))
   else:
      pri=list(filter(prime,range(1,floor(Decimal(y).sqrt()+1))))
   a={};b=0
   for i in pri:
      while y%i==0:
         y=y//i
         b+=1
         a[i]=b
         b=0
   if len(list(a.keys()))==1:
      if list(a.values())!=[1]:

         c=str(a.keys())+'^'+str(a.values())
      else:
         c=str(list(a.keys()))
   elif len(list(a.keys()))>1:
      if a[list(a.keys())[0]]>1:
         c=str(list(a.keys())[0])+'^'+str(a[list(a.keys())[0]])

      elif a[list(a.keys())[0]]==1:
         c=str(list(a.keys())[0])
      for key in list(a.keys())[1:]:
         if a[key]!=1:
            c+=' * '+str(key)+'^'+str(a[key])

         elif a[key]==1:
            c+=' * '+str(key)

   return c

Given that you do y=factorial(n) inside your decomp(n) function, I'm gonna assume that this is really about factoring factorials. And my review is gonna be:

Don't compute the factorial at all.
Use a better function name.
Document what the function does.
Separate the factorization from the presentation. Return something more useful than a string, like pairs of primes and their exponents. That can then be pretty-formatted if desired, or used for computations.
Test your code, because for example for decomp(10) returns '2 * 3 * 5 * 7', missing the exponents.

Instead of computing the factorial and then factoring that, factor all the numbers from 1 to n and combine their prime factorizations. Or even better... compute, for every prime from 1 to n, its exponent in n!. Let's say we want to find the exponent for 3 in 100!. Every third number from 1 to 100 (i.e., 3, 6, 9, 12, etc) contributes a factor 3, so there are \$\lfloor\frac{100}{3}\rfloor=33\$ numbers that contribute factor 3. Of those, every third number (i.e., 9, 18, 27, etc) contributes another factor 3. Of those, every third number (i.e., 27, 54 and 81) contributes yet another. And 81 even contributes a fourth. So the exponent for 3 in 100! is 33+11+3+1=48.
Here's an O(n) solution doing that. Takes my laptop about 0.33 seconds to factor 1,000,000!. Compare that to just computing math.factorial(10**6), which already takes about 10 seconds.
def factorial_factorization(n):
    """Return the prime factorization of n! as list of (prime, exponent) pairs."""

    # Compute primes from 2 to n, see https://cp-algorithms.com/algebra/prime-sieve-linear.html
    lp = [None] * (n+1)
    primes = []
    for i in range(2, n+1):
        if not lp[i]:
            primes.append(i)
            lp[i] = i
        for p in primes:
            if p > lp[i] or i * p > n:
                break
            lp[i * p] = p

    # Find prime exponents for n!
    result = []
    for p in primes:
        e = 0
        m = n // p
        while m:
            e += m
            m //= p
        result.append((p, e))
    return result

Demo:
>>> factorial_factorization(3)
[(2, 1), (3, 1)]

>>> factorial_factorization(4)
[(2, 3), (3, 1)]

>>> factorial_factorization(100)
[(2, 97), (3, 48), (5, 24), (7, 16), (11, 9), (13, 7), (17, 5), (19, 5), (23, 4), (29, 3), (31, 3), (37, 2), (41, 2), (43, 2), (47, 2), (53, 1), (59, 1), (61, 1), (67, 1), (71, 1), (73, 1), (79, 1), (83, 1), (89, 1), (97, 1)]

>>> factorial_factorization(10**6)[:10]
[(2, 999993), (3, 499993), (5, 249998), (7, 166664), (11, 99998), (13, 83332), (17, 62497), (19, 55553), (23, 45453), (29, 35713)]

If you want a less useful but prettier presentation, that's easy to do then:
>>> n = 42
>>> print(f'{n}! =', ' * '.join(f'{p}^{e}' if e > 1 else f'{p}'
                                for p, e in factorial_factorization(n)))
42! = 2^39 * 3^19 * 5^9 * 7^6 * 11^3 * 13^3 * 17^2 * 19^2 * 23 * 29 * 31 * 37 * 41

Checking that:
>>> s = '2^39 * 3^19 * 5^9 * 7^6 * 11^3 * 13^3 * 17^2 * 19^2 * 23 * 29 * 31 * 37 * 41'
>>> (result := eval(s.replace('^', '**')))
1405006117752879898543142606244511569936384000000000

>>> (expect := math.factorial(42))
1405006117752879898543142606244511569936384000000000

>>> result == expect
True


Listing the playlists on which each song appears I created a way to export my Google Play playlists to a text file and also a way to export all of my songs in Google Play to a text file. Now I have both the song list (in an array format) and playlist data (in object and array format) in their respective text files. Here is a simplified version of the files:

[
    "song 1 name",
    "song 2 name",
    "song 3 name"
]

and

{
    "playlist 1": [
        "song 1",
        "song 3"
    ],
    "playlist 2": [
        "song 1"
    ]
}

When I run them in my script, it works correctly and creates the following output:

{
    "song 1": [
        "playlist 1",
        "playlist 2"
    ],
    "song 2": [],
    "song 3": [
        "playlist 1"
    ]
}

Here's my code:

var songInput = document.getElementById("songInput"),
    playlistInput = document.getElementById("playlistInput"),
    result = document.querySelector("textarea");

var songList,
    playlistData;

function loadData(id, elem) {
    if(elem.files
    && elem.files[0]) {
        let myFile = elem.files[0];
        let reader = new FileReader();

        reader.addEventListener('load', function (e) {
            if(id === "songInput")
                songList = JSON.parse(e.target.result);

            if(id === "playlistInput")
                playlistData = JSON.parse(e.target.result);

            checkBothAdded();
        });

        reader.readAsBinaryString(myFile);
    }
}

function checkBothAdded() {
    if(songList
    && playlistData) {
        getSongPlaylistData();
    }
}

var songData = {};
function getSongPlaylistData() {
    for(let song of songList) {
        let playlistsItsIn = [];

        Object.keys(playlistData).forEach(function(key) {
            if(playlistData[key].includes(song)) {
                playlistsItsIn.push(key);
            }
        });

        songData[song] = playlistsItsIn;
    }

    result.value = JSON.stringify(songData, null, '\t');
}

songInput.addEventListener("change", function() {
    loadData("songInput", this);
});

playlistInput.addEventListener("change", function() {
    loadData("playlistInput", this);
});
Song text file (generated from this answer): <input type="file" id="songInput">

Playlist text file (generated from this answer): <input type="file" id="playlistInput">

<textarea style="width: 500px; height: 200px;"></textarea>

It works on this small set of data I hand-wrote, but it freezes the browser page when I run it on my ~4,000 song list and substantial playlist file.

How can I do this same thing while helping it perform better so it doesn't freeze my browser tab?

I'm theorizing that it would work faster if I keep them as text files and try to parse each of the playlist names if the song is found, but I'm curious if I can speed up/reduce the memory of this object approach somehow.

Use Map for fast lookups

The main performance cost is the search for songs

The best way to get better lookup performance is to use a Map to store the songs. Maps use a hash to lookup items and is very fast once the Map has been created.

You can create a map as follows, it adds an array for the playlist.

const songMap = new Map(songList.map(name => [name,{name,playLists:[]]));

Then you iterate the playlist and the songs it contains, adding the playlist to each song

for (const [playListName, songs] of Object.entries(playlists)) {
    for (const songName of songs) {
        const songItem = songMap.get(songName);
        if (songItem) {
             songItem.playLists.push(playListName);
        }
    }
}

You can then iterate the songMap and create the final object containing songNames with arrays of playlists

 const songPlaylists = {};
 for(const song of songMap.values()) {
       songPlaylists[song.name] = song.playLists;
 }

Then convert to JSON if you need.

Why the processed store?

However I would argue that the whole process is pointless unless you are extracting some higher level information from this process.

Ideally you would store the songs in a Map and each playlist also stored in a Map and would contain a Map of songs. Thus you can lookup playlists a song is in as follows

  function findSongPlaylists(songName) {
      const playlists = [];
      for (const playlist of playlistMap.values()) {
          if (playlist.songs.has(songName)) { playLists.push(playList.name) }
      }
      return playlists;
  }

To further improve the store, you would assign a unique id (a number) to each song and playlist and use the and do all the searches using ids. This will give even better performance and save a huge amount of RAM.

For casual viewing it is the ideal.

BTW the link you provided showing how you extracted the song data has a very long way of getting the data. All that data is store in the page indexed DB and can be extracted and saved to disk with a few lines of JS. (no need to scroll the page). Use Dev tools > application tab and open indexedDB to find the data.

Update Extra

Looking through the link you posted Copy Google Play Music songs all the solutions required rather hacky method (due to song list being a view) of slowly scrolling over the data to acquire it. The question is locked thus I am unable to add a better answer. As this question is related I will add a better way of sourcing the data here.

Paste the following snippet into the Dev-Tools console and hit enter. It will extract all the songs and save them to your download directory as a JSON file (named music_[your id].tracks.json)

The json file contains (data refers to the parsed json file)

data.fields an array of field names in the same order as in the array
data.tracks an array of records (one per song) containing fields in the same order as in  data.fields.

Note I do not use Google Music (Used a friends account so had limited time) and hence I guessed the content. You can add extra fields if you find the array index. To get a single record go to dev-tools > application (tab) > indexedDB > music_[your id] and select a record.

Example record extracted from music_####.tracks For security ids have been masked with # or A...

"########-####-####-####-############":["########-####-####-####-############","Sheep","https://lh4.ggpht.com/AAAAAAA-AAAAAAAAAAAAAA_AAAAAA...","Pink Floyd","Animals","Pink Floyd","sheep","pink floyd","animals","pink floyd","Roger Waters","Rock",null,620059,4,0,0,0,1977,0,null,null,0,0,1372528857298236,1408583771004924,null,"AAAAAA...","AAAAAAAA...",2,"",null,"AAAAAAA...","AAAAAAAA...",128,1372582779985000,"https://lh3.googleusercontent.com/AAAAAA_AAAA...",null,null,[],null,null,null,null,null,null,null,null,null,null,"AAAAAA...",[],null,null,null,null,0,null,null,[["https://lh3.googleusercontent.com/AAAAA...",0,2],["https://lh3.googleusercontent.com/AAAAA...",0,1]],true,6,null,1],

To keep the file size down the data is in an array. Nulls have been converted to 0.

(function () {
    console.log("From https://codereview.stackexchange.com/a/213683/120556"); // DO NOT REMOVE THIS LINE if you publish this snippet.
    console.log("Just a moment locating music DB...");
    const data = {
        dbName:"",
        info: "From Google Play Music App IndexedDB.\nField names in order of song array index.\nTo keep JSON size down null has been replaced with 0.\nDates ar in micro seconds 1/1,000,000 sec (I don't know what date1, date2, date3 represent)\nImages may require additional query properties.\nSource https://codereview.stackexchange.com/a/213683/120556",
        fields: "GUID,song,image1,band,album,albumArt,composer,genre,length,track,tracks,disk,disks,year,plays,date1,date2,bitrate,date3,image2".split(","),
        tracks: [],
    };
    const idxs = [0,1,2,3,4,5,10,11,13,14,15,16,17,18,22,24,25,34,35,36]; // idx of info to extract. There are many more field,
    indexedDB.databases().then(info => {
        const name = data.dbName = info.find(db => db.name.indexOf("music_") === 0).name;
        indexedDB.open(name).onsuccess = e => {
            console.log("Extracting tracks fro DB " + name);
            const t = e.target.result.transaction("tracks", IDBTransaction.READ_ONLY);
            t.oncomplete = () => {
                Object.assign(document.createElement("a"), { download : name + ".tracks.json", href: URL.createObjectURL(new Blob([JSON.stringify(data)]), {type: "text/json" })})
                    .dispatchEvent(new MouseEvent("click", {view: window, bubbles: true, cancelable: true}));
            }
            t.objectStore("tracks").openCursor().onsuccess = e => {
                if (e = e.target.result) {
                    Object.values(JSON.parse(e.value)).forEach(t => data.tracks.push(idxs.map(i=>t[i]===null?0:t[i])));
                    e.continue();
                }
            }
        }
    }).catch(()=>console.log("Sorry can not complete data dump :("));
})()

To extract a record from the JSON

// data is the parsed JSON file
const songName = "A song name";
const songFieldIdx = data.fields.indexOf("song");
const track = data.tracks.find(rec => rec[songFieldIdx] === songName);

To convert track array record to track object

const trackObj = {}
for(const field of data.fields) { trackObj[field] = track.shift() }


While focus on input check if empty I made a small function that is getting looped when an input is getting focus and stopping the loop when the input is losing focus. Now I would like to know if there is a better way to write this code.

 var timer;
  function Change () {
    if( !$('#eventurl').val() ) {

      $("#js-skipbutton").text('overslaan');
      console.log(0);
    }
    else{
      $("#js-skipbutton").text('ophalen');
      console.log(1);
    }
  };
  $('#eventurl').focus(function() {
    timer = setInterval(Change, 200);
  });
  $('#eventurl').focusout(function() {
    clearTimeout(timer);
    Change();
  });

This code is changing the text to "overslaan" on a span when the input is empty and to "ophalen" when the input is containing text.

You could cache $("#eventurl") and $("#js-skipbutton") so jQuery won't search those elements multiple times, that's a little speed up.

var eventulr = $(document.getElementById("eventurl"));
var js_skipbutton = $(document.getElementById("js-skipbutton"))

It's not necessary to check for focus or focusout events in an input element (specially the focusout event, that is used for event bubbling).

Instead, only check for keypress event. This way you won't be checking if the input element is empty every 200 milliseconds whether the user is pressing a key or not

eventurl.on("keypress", Change);

Edit

Since keypress event is not triggered when text is pasted into the input element you can listen for keyup event instead; Which is triggered whenever a key is released (Including Ctrl, Shift, etc.). Or rely on jQuery's change() method which works with input, select, textarea elements.

eventurl.change(Change);


Ruby code forces the answer using a loop and a branch to guess my favorite ice cream flavor I'm writing very simple programs. "Crawl before you walk..." they say.

How could I clean this up? And/or simplify?

While I am very new, technical lanquage is necessary because it helps me learn the lessons I need to learn

puts "What is my favorite ice cream flavor?"

ice_cream_1 = gets.chomp.capitalize

while ice_cream_1 != "Vanilla"
   puts "Wrong answer; what is my favorite ice cream flavor?"

   my_favorite_ice_cream_flavor = gets.chomp.capitalize

   if my_favorite_ice_cream_flavor == "Vanilla"
      break
   end
end

puts "Now that's my jam"

There is no need to write the gets and the string comparison code twice. The following code is equivalent:

puts "What is my favorite ice cream flavor?"
while gets.chomp.capitalize != "Vanilla"
   puts "Wrong answer; what is my favorite ice cream flavor?"
end
puts "Now that's my jam"


SQL Query Builder in PHP I have made a Query Builder, (not the full version of it yet) but I wanna ask if there is someone out there, there can improve this code even more. Will be nice to know so I can code the rest in the better way :)

<?php
class DB {

    private static $instance = NULL;

    public static function sql(){
        self::$instance = new self();
        return self::$instance;
    }

    // These methods set the query clauses
    // Use * as default
    public function select(string $select_options = "*") {

        $query_part = "SELECT ".$select_options;
        $this->select = $query_part;
        return $this;
    }

    // From table in DB
    public function from(string $from_options) {

        $query_part = "FROM ".$from_options;

        $this->from = $query_part;
        return $this;
    }

    // These methods set the query clauses
    public function where(string $where_options) {

        $query_part = "WHERE ".$where_options;
        $this->where = $query_part;
        return $this;
    }

    // GroupBy
    public function groupBy(string $group_options) {

        $query_part = "groupBy ".$group_options;
        $this->groupBy = $query_part;
        return $this;
    }

    public function get() {
        $array = (array)$this;
        $string = implode(" ", $array);

        return $string;
    }

}

And this I how it to call the query builder

// Use class DB
$query = DB::sql()
    ->select()
    ->from("CustomerInfo")
    ->where("customer_id > 300")
    ->get();

// Echo result
echo $query;

Layout for code:
Look at PSR spec: http://www.php-fig.org/psr/psr-2/

for comments:
https://www.phpdoc.org/docs/latest/getting-started/your-first-set-of-documentation.html#writing-a-docblock

Finally is your code testable and possibility to extend without modifying it?

Also "(static) $instance" doesn't feel right to me for some reason, try different name?


Telegram - GetUpdates Process My code fetches telegrams from a server periodically in the background. The server API is documented here. What do you think of the way I do multithreading by handling my _updatesList?

public class AutoUpdate
{
    public bool IsStarted { get; private set; }
    private readonly Timer _timer;

    private readonly List<Update> _updatesList;
    private readonly object _lock = new object();
    private long _lastUpdateId;

    public AutoUpdate()
    {
        _lastUpdateId = 0;
        _updatesList = new List<Update>();
        TimerCallback getUpdates = GetUpdates;
        _timer = new Timer(getUpdates, null, Timeout.Infinite,Timeout.Infinite);
        IsStarted = false;
    }

    public bool Start()
    {
        if (Monitor.TryEnter(_lock, 500))
        {
            try
            {
                _timer.Change(0, 100);
                IsStarted = true;
            }
            finally
            {
                Monitor.Exit(_lock);
            }
            return true;
        }
        return false;
    }

    public bool Stop()
    {
        if (Monitor.TryEnter(_lock, 500))
        {
            try
            {
                _timer.Change(Timeout.Infinite, Timeout.Infinite);
                IsStarted = false;
            }
            finally
            {
                Monitor.Exit(_lock);
            }
            return true;
        }
        return false;
    }

    public bool Terminate()
    {
        var stop = Stop();
        if (!stop)
            return false;
        _timer.Dispose();
        return true;
    }

    public ICollection<Update> GetLastUpdates()
    {
        lock (_lock)
        {
            //Make A Copy: for thread safety
            //Then Pass To ReadOnlyCollection
            return new ReadOnlyCollection<Update>(_updatesList.ToArray());
        }
    }

    public void RemoveFromUpdates(Update update)
    {
        lock (_lock)
        {
            _updatesList.Remove(update);
        }
    }

    private void GetUpdates(object state)
    {
        ReturnedResult<List<Update>> responseObject = null;

        if (Monitor.TryEnter(_lock))
        {
            try
            {
                HttpClient hc = new HttpClient();
                HttpContent requestContent = new ObjectContent<UpdateRequest>(new UpdateRequest(),
                    new JsonMediaTypeFormatter());
                var task =
                    hc.PostAsync(
                        "https://api.telegram.org/bot<Private Token>/getUpdates",
                        requestContent);
                if (task != null)
                {
                    var result = task.Result;
                    var jsonString = result.Content.ReadAsStringAsync().Result;
                    responseObject = JsonConvert.DeserializeObject<ReturnedResult<List<Update>>>(jsonString, new JsonSerializerSettings
                    {
                        ContractResolver = new CustomPropertyNamesContractResolver { Case = IdentifierCase.UnderscoreSeparator}
                    });
                }

                //Add To The List...
                if (responseObject != null && responseObject.Ok)
                {
                    var updateList = responseObject.Result.Where(w => w.UpdateId > _lastUpdateId);

                    //Resharper Detected: Possible multi enumeration of same list in same time
                    var list = updateList as Update[] ?? updateList.ToArray();

                    var maxId = list.Max(m => (long?) m.UpdateId) ?? 0;
                    _lastUpdateId = Math.Max(maxId, _lastUpdateId);

                    _updatesList.AddRange(list);
                }
            }
            finally
            {
                Monitor.Exit(_lock);
            }
        }
    }
}

Disposable

You class manages a Timer, which you dispose of if Terminate is called.  It feels like your class should implement IDisposable and cleanup the timer if it hasn't already been done.

Is Started

Does this need to be exposed as a public property?  You're setting it in both your Start and Stop methods, but you never check its existing state.  Is calling Start twice in a row without calling Stop acceptable?  Perhaps Start should return IsStarted, rather than a value to indicate if it was able to acquire the lock.

Uncertain Usage

As I said in my response to your previous code it is unclear what your use case for this code is.  You are protecting the list, by creating a copy of it in GetLastUpdates, however if this is called by multiple threads each thread could end up with its own copy of the underlying list with the same items in it.  Each thread could then decide to perform some processing on the same item in the list.  Does this matter?  Is it prevented in someway outside of the code you've shown?  Some alternative options would be:

Calling threads simply pop the first item from the list, then perform processing on it.  If the underlying _updateList was a ConcurrentQueue and it was the the only way to fetch items, you could change GetUpdates to enqueue items and probably do away with the Monitor lock altogether.  Using your existing data structure you would have something like this.

public Update GetFirstItem()
{
    Update update = null;

    if (Monitor.TryEnter(_lock))
    {
        try
        {
            update = _updatesList.FirstOrDefault();
            if (null != update)
            {
                _updatesList.Remove(update);
            }
        }
        finally
        {
            Monitor.Exit(_lock);
        }
    }
    return update;
}

Calling threads pass in a selection delegate to select the item from the list they need to remove, then perform processing on it.

public Update GetFirstMatchingUpdate(Func<Update, bool> searchCriteria)
{
    Update update = null;

    if (Monitor.TryEnter(_lock))
    {
        try
        {
            update = _updatesList.Where(searchCriteria).FirstOrDefault();
            if(null != update)
            {
                _updatesList.Remove(update);
            }
        }
        finally
        {
            Monitor.Exit(_lock);
        }
    }
    return update;
}

Called like this:

var update = au.GetFirstMatchingUpdate(x => x.SomeField == "someText" && x.Id > 3);

Calling threads call existing GetLastUpdates, find the update they want to process, remove it from the list, then perform processing only if that removal works (i.e. they were the first to remove it).
Processing is only actually performed by a single thread (with the contention being between the update and the processor), in which case the problem I've described doesn't exist.


"The Trip" challenge from Programming Challenges Here is a problem from the book Programming Challenges by Skiena and Revilla:

  The Trip

  A number of students are members of a club that travels annually to
  exotic locations. Their destinations in the past have included
  Indianapolis, Phoenix, Nashville, Philadelphia, San Jose, and Atlanta.
  This spring they are planning a trip to Eindhoven. The group agrees in
  advance to share expenses equally, but it is not practical to have
  them share every expense as it occurs. So individuals in the group pay
  for particular things, like meals, hotels, taxi rides, plane tickets,
  etc. After the trip, each student's expenses are tallied and money is
  exchanged so that the net cost to each is the same, to within one
  cent. In the past, this money exchange has been tedious and time
  consuming. Your job is to compute, from a list of expenses, the
  minimum amount of money that must change hands in order to equalize
  (within a cent) all the students' costs.

  The Input

  Standard input will contain the information for several trips. The
  information for each trip consists of a line containing a positive
  integer, n, the number of students on the trip, followed by n lines of
  input, each containing the amount, in dollars and cents, spent by a
  student. There are no more than 1000 students and no student spent
  more than $10,000.00. A single line containing 0 follows the
  information for the last trip.

  The Output

  For each trip, output a line stating the total amount of money, in
  dollars and cents, that must be exchanged to equalize the students'
  costs.

  Sample Input

3
10.00
20.00
30.00
4
15.00
15.01
3.00
3.01
0

  Sample Output

$10.00
$11.99

Here is my solution which gives correct answer for all inputs I have tried so far, but there must be some case when it does not work because the judge says so. Can anybody help me to understand why?

int main (int argc, char * argv[]) {
    int64_t a;
    float paid[1000];
    std::vector<float> results;
    while (std::cin >> a && a != 0) {

        int sum = 0;
        for (int i = 0; i < a; i++) {
            std::cin >> paid[i];
            paid[i] *= 100;
            sum += paid[i];
        }
        int result = 0;
        int avg = (std::ceil)((float)sum/a);

        int mod = sum % a;
        int c = 0;
        if (mod > 0) {
            for (int i = 0; i < a; i++) {
                if (c >= a - mod)
                    break;
                if (paid[i] < avg) {
                    paid[i]++;
                    c++;
                }
            }
            for (int i = 0; i < a; i++) {
                if (c >= a - mod)
                    break;
                if (paid[i] > avg) {
                    paid[i] += a - mod - c;
                    break;
                }
            }
        }

        for (int i = 0; i < a; i++) {
            if (paid[i] > avg) {
                result += (paid[i] - avg);
            }
        }

        results.push_back((float)result/100);
    }
    std::cout << std::fixed;
    std::cout << std::setprecision(2);
    for (std::vector<float>::iterator it = results.begin(); it != results.end(); ++it) {
        std::cout << "$" << *it << std::endl;
    }
    return 0;
}

Your algorithm seems to be correct though a bit overcomplicated.
But the code is not correct. consider:

9999.1
9999.1
9999.0
9999.1

$0.05

And the correct answer is $0.07

This is due to lack of precision in float implementation. It does not mean you should use double, it mean you should check your casts and try to avoid using float at money computations at all.

Also, i suggest to check your inputs.


jQuery plugin to detect Konami cheat code sequence How can this be written a bit shorter?

jQuery.konami = function(fn, code) {
    // ↑ ↑ ↓ ↓ ← → ← → B A
    code = code || [38, 38, 40, 40, 37, 39, 37, 39, 66, 65];

    var kkeys = '',
        i = 0;

    $(document).keydown(function(e) {
        var char = String.fromCharCode(e.which).toLowerCase();
        if (char === code[i++]) {
            kkeys += char;

            if (kkeys === code) {
                fn();
                kkeys = '';
                i = 0;
            }
        } else if (e.which === code[kkeys++]) {
            if (kkeys === code.length) {
                fn();
                kkeys = '';
                i = 0;
            }
        } else {
            kkeys = '';
            i = 0;
        }
    });
};

This is almost like code golf; this could probably be shortened.  You just need to keep track of i and when it's past the end of the array, you know all the keys were hit in the correct order.

jQuery.konami = function() {
    function KonamiCode(kFn, kCode) {
        var i = 0;

        $(document).keydown(function(e) {
            var char = typeof kCode === 'string' ? String.fromCharCode(e.which).toLowerCase() : e.which;
            i = char === kCode[i] ? i + 1 : 0;
            if (i === kCode.length) {
                kFn();
                i = 0;
            }
        });
    }
    return function(fn, code) {
        // ↑ ↑ ↓ ↓ ← → ← → B A
        kCode = code || [38, 38, 40, 40, 37, 39, 37, 39, 66, 65];
        new KonamiCode(fn, kCode);
    };
}();


Replacing accented letters with regular letters in a spreadsheet I am using the following VBA code to replace accented letters with regular letters in a spreadsheet. This is necessary because these spreadsheets have to be uploaded to an import tool that does not allow foreign characters.

Function RemoveAccentsFromForeignLetters()

    StartNewTask ("Removing accents from foreign letters")

    Dim AccChars As String
    Dim RegChars As String

    AccChars = "ŠŽšžŸÀÁÂÃÄÅÇÈÉÊËÌÍÎÏÐÑÒÓÔÕÖÙÚÛÜÝàáâãäåçèéêëìíîïðñòóôõöùúûüýÿ"
    RegChars = "SZszYAAAAAACEEEEIIIIDNOOOOOUUUUYaaaaaaceeeeiiiidnooooouuuuyy"
    Set MyRange = ActiveSheet.UsedRange

    Dim A As String * 1
    Dim B As String * 1
    Dim i As Integer

    For i = 1 To Len(AccChars)
        A = Mid(AccChars, i, 1)
        B = Mid(RegChars, i, 1)
        MyRange.Replace What:=A, Replacement:=B, LookAt:=xlPart, MatchCase:=True
        ' TODO: highlight changed cells yellow
    Next

End Function

I googled the code from somewhere and it works, but it is a bit slow. In a spreadsheet with 1.5 million cells (7000 rows, 200 columns), it takes 21 seconds to run.

I wanted to look into ways to optimize it, for example:

Maybe a RegEx would be faster?
Maybe I should pass the entire spreadsheet to a DLL using an array, have the DLL do the replace, then pass it back? One article I read suggested that this is up to 10x faster than using native Excel VBA.
Maybe I should use the same DLL trick as above, but add multi-threading?
Any other ideas?

In addition to @Raystafarian's observations, there are a couple of other issues that I see.

I would personally put your AccChars and RegChars variables into
Consts, because you never change their values.
Your code also requires that AccChars and RegChars are the same
length, and will fail if they aren't.  I'd add an assert that tests
that before you do anything that writes to the Worksheet.
You generally want to avoid using the Integer type unless you
absolutely need to (for example in an API call). VBA stores them as a
Long regardless of how they are declared.
Declaring A and B as fixed-length strings is a bit of premature
optimization that is backfiring. When you pass them to .Replace as
parameters, they are actually being implicitly cast back to
variable length ones.
Minor thing, but I prefer the term "stripped" to "regular" - all
characters are "regular" in their native context.
Using string functions is not ideal when what you really care about
are individual characters as opposed to a sub-string.  VBA allows a
direct assignment of a String to a Byte array, and indexing into
the array is much faster than calling Mid. The performance hit is a
lot higher when you're doing it inside a loop. As a side note, you
should always use the String returning functions that end with '$'
to avoid superfluous casting unless you explicitly require a
Variant type. Mid$ (returns a String) as opposed to Mid
(returns a Variant).
Your call to .Replace method is acting on every single cell in your
Range. This is a huge performance hit, because I'm guessing that
not every cell in the entire Worksheet is going to have an accented
character in it. You should really only be concerned with cells that
do. By performing the replacement on every cell, your performance is scaling directly with the number of cells, not the number of
replacements. So, if only 5% of the cells have accented characters
you are still doing 100% of the work. This is where the regular
expression would be useful, but you can't easily pawn that off on
Excel (except maybe with using .Find, which has issues of its own).
A loop would be better - a loop over an array pulled from the Range
would be best.
Your "TODO: highlight changed cells yellow" is going to be much more
difficult using the .Replace function, because it would require
storing the state of the entire sheet, then doing a cell-by-cell
comparison. It will be a lot easier to track this concurrently while
you are making changes.

With all of that in mind, I'd do something more like this:

Private Sub RemoveAccentsFromForeignLetters()
    Dim Target As Range
    Set Target = ActiveSheet.UsedRange

    StartNewTask ("Removing accents from foreign letters")

    Dim Values() As Variant
    Values = Target.Value

    Debug.Assert Len(AccentedChars) = Len(StrippedChars)

    Dim FindChars() As Byte
    Dim ReplaceChars() As Byte
    FindChars = AccentedChars
    ReplaceChars = StrippedChars

    Dim AccentedTest As RegExp
    Set AccentedTest = New RegExp
    AccentedTest.Pattern = "[" & AccentedChars & "]"

    Dim index As Long
    Dim character As Long

    Dim col As Long
    Dim row As Long
    For row = 1 To UBound(Values, 1)
        For col = 1 To UBound(Values, 2)
            'Ignore strings that don't require character replacements.
            If AccentedTest.Test(Values(row, col)) Then
                Dim buffer() As Byte
                buffer = StrConv(Values(row, col), vbUnicode)
                'Skip every other character - VBA "Unicode" expansion
                'inserts nulls there.
                For character = 0 To UBound(buffer) Step 2
                    For index = 0 To UBound(FindChars)
                        If buffer(character) = FindChars(index) Then
                            buffer(character) = ReplaceChars(index)
                        End If
                    Next index
                Next character
                'Highlight changed cells yellow left as an exercise for
                'the reader.
                Values(row, col) = StrConv(buffer, vbFromUnicode)
            End If
        Next col
    Next row

    ActiveSheet.UsedRange = Values
End Sub

Some quick and dirty benchmarks, all done with 2000 rows and 10 columns.  In the "worse case" benchmarks, all cells have the value "ŸÀÁÂÃÄÅÇÈÉÊËÌÍÎ".  In the "average case" benchmarks, 5% of the cells have the "ŸÀÁÂÃÄÅÇÈÉÊËÌÍÎ" and the rest of them contain "XXXXXXXXXXXXXXX".

Replace method, worst case: 3.51 seconds.
Array method, worst case: 1.15 seconds.
Replace method, average case: .40 seconds (supports disabling ScreenUpdating).
Array method, average case: .08 seconds.


Disallowing repeated characters and space after punctuation I'm trying to "sanitize" a textarea input by disallowing repeated characters like "!!!", "???", etc. add spaces after commas, and I would like to optimize the code because I'm not a pro at this.

jsFiddle

$("#post_input").keypress(function() {
        var obj = this;
        setTimeout(function() {
            var text = obj.value;
            var selStart = obj.selectionStart;
            var newText = text.replace(/,{2,}|\.{4,}|\!{4,}|\¡{4,}|\?{4,}|\¿{4,}/, function(match, index) {
                if (index < selStart) {
                    selStart -= (match.length - 4);  // correct the selection location
                }
                return(match.substr(0,1));
            });
            if (newText != text) {
                obj.value = newText;
                obj.selectionStart = obj.selectionEnd = selStart;
            }
        }, 1);
    });

    $("#post_input").blur(function(){
      this.value = this.value.replace( /,\s*/g, ', ' );
    });

I would like to merge all in only one regex and function.

Basically, I want to:

Prevent repeated characters (all kind of characters) to 3. No "aaaaa" or "!!!!!"
Add space after commas (,) or any kind of punctuation (!) (?) (.)

Prevent repetead characters (all kind of characters) to 3.

If you mean replace 4 or more repeated characters with single character:

str.replace(/(.)\1{3,}/g, '$1');

  Add space after commas (,) or any kind of punctuation (!) (?) (.)

str.replace(/[,.!?:;](?=\S)/g, '$& ');


C function to capitalize first word in a sentence, handles arbitrary spaces The code below capitalized the first letter of every word in a sentence. It handles an arbitrary number of spaces in any part of string. It works. Can it be optimized further?

void capitalize_string(char *s) {
  while(*s) {
    while (*s && *s == ' ')
      s++;
    *s = toupper(*s);
    s++;
    while (*s && *s != ' ')
      s++;
  }
}

Declare the toupper function
The easiest way to provide a prototype is
#include <ctype.h>

Consider matching all whitespace
At present, we don't capitalize a word at the beginning of a line.  We could deal with this by using isspace() instead of testing for equality with ' '.  Also, we might want to consider a word to start when a letter immediately follows punctuation (such as ", / or -).
Bug: if the string ends with a space
Consider the string "  " and walk through.
We loop while (*s && *s == ' ') until s points at the terminating NUL character.  Now we upcase that NUL (which is harmless), and increment s again.  That's not good.
We need
 if (!*s) { break; }

before line 6.
An alternative approach
Instead of changing between two different loops for whitespace and non-whitespace runs, we can write a single loop if we use a variable to remember what we've most recently seen; I'll call it prev.  Start off with it set to a space, so that we'll capitalise the first letter of the string.  The code then looks like this:
#include <ctype.h>

void capitalize_string(char *s)
{
    for (int prev = ' ';  *s;  ++s) {
        int current = (unsigned char)*s;
        if (!isalnum(prev) && isalpha(current)) {
            *s = (char)toupper(current);
        }
        prev = current;
    }
}

Note that the <ctype.h> functions all accept (and return) an int that represents an unsigned char version of a character - on platforms where plain char is signed, passing one directly can be undefined behaviour.
Here's a short test program so we can give command-line arguments to try out:
#include <stdio.h>
int main(int argc, char **argv)
{
    for (int i = 1;  i < argc;  ++i) {
        capitalize_string(argv[i]);
        printf("%s\n", argv[i]);
    }
}

A more sophisticated test allows us to check a suite of examples every time we make a change to the code, and allows us to be explicit about our expectations:
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
int check(const char *input, const char *expected)
{
    char *s = malloc(strlen(input)+1);
    if (!s) {
        perror("malloc");
        return 1;
    }
    strcpy(s, input);

    int err_count = 0;
    capitalize_string(s);
    if (strcmp(s, expected)) {
        fprintf(stderr, "*** Input \"%s\"; expected \"%s\"; got \"%s\"\n",
                input, expected, s);
        ++err_count;
    }

    free(s);
    return err_count;
}

int main()
{
    return check("", "")
        +  check("123", "123")
        +  check("ab", "Ab")
        +  check("AB", "AB")
        +  check("ab cd", "Ab Cd")
        +  /* add more tests here */
        +  check("harley-davidson", "Harley-Davidson")
        ;
}


tuple initialization in a loop I would like to do the following:
I have a list data of 3-tuples that I need to go through, for each tuple base_tuple I want to know the index index_max of the maximum value held in base_tuple and replace base_tuple with a new 3-tuple new_tuple that only holds 0s except for the value at index index_max which is set at a fixed value fixed_value.
For instance, with a fixed_value of 127 and a base_tuple of (12, 55, 27), I expect new_tuple to be (0, 127, 0).

This is the code I use:

fixed_value = 255
# data is the list of tuples
for i, base_tuple in enumerate(data):
    index_max = base_tuple.index(max(base_tuple))
    new_tuple = tuple(fixed_value*(i==index_max) for i in range(3))
    data[i] = new_tuple

It works but it looks more like a hack than proper python... Any idea how to do this in a more pythonic way?

Code organisation

At this moment, the code is there with no context. In order to make it easier to unerstand, reuse and test, you could try to define in a a function on its own.

It could for instance:

take data as a parameter
take fixed_value as a parameter (with a default of 255 ?)

In-place changes

When trying to explain or test the function, things are make slighly harder by the fact that the input is modified. In your case, it may be just as easy to return a new list of values.

Here is a tweet from Raymond Hettinger (Python core developer and trainer):

  Today's super-easy #python student question.

  Q. What are the best practices for modifying a list while looping over it?

  A. Don't. Seriously, just make a new list and avoid hard-to-read code with hard-to-find bugs.

At this stage, we have something like:

new_data = []
for i, base_tuple in enumerate(data):
    index_max = base_tuple.index(max(base_tuple))
    new_tuple = tuple(fixed_value*(i==index_max) for i in range(3))
    new_data.append(new_tuple)
return new_data

But the call to enumerate is not required anymore and we can write:

new_data = []
for base_tuple in data:
    index_max = base_tuple.index(max(base_tuple))
    new_tuple = tuple(fixed_value*(i==index_max) for i in range(3))
    new_data.append(new_tuple)
return new_data

More functions

We could extract out the tuple computation in a function on its own.
We would have something like:

def get_new_tuple(base_tuple, fixed_value):
    index_max = base_tuple.index(max(base_tuple))
    return tuple(fixed_value*(i==index_max) for i in range(3))

def get_new_val(data, fixed_value = 255):
    # data is the list of tuples
    new_data = []
    for base_tuple in data:
        new_data.append(get_new_tuple(base_tuple, fixed_value))
    return new_data

(Documentation and function names should be improved).

Now, this is a good occasion to use list comprehension in get_new_val.

At this stage, renaming the function, the parameter and adding a simple test, we have:

def get_new_tuple(base_tuple, fixed_value):
    index_max = base_tuple.index(max(base_tuple))
    return tuple(fixed_value*(i==index_max) for i in range(3))

def get_new_tuples(tuples, fixed_value = 255):
    return [get_new_tuple(tup, fixed_value) for tup in tuples]

def test_get_new_tuples():
    # This could/should be written with a proper unit-test framework
    data = [(12, 55, 27), (260, 55, 27), (12, 55, 255)]
    output = get_new_tuples(data)
    assert output == [(0, 255, 0), (255, 0, 0), (0, 0, 255)]

Improving get_new_tuple

There is the magic number 3. It would make sense to be more generic and accept tuples of any length.

For instance:

def get_new_tuple(base_tuple, fixed_value):
    index_max = base_tuple.index(max(base_tuple))
    return tuple(fixed_value*(i==index_max) for i in range(len(base_tuple)))

And we can test it with more unit tests:

def test_get_new_tuple():
    tup = (12, 55, 27)
    output = get_new_tuple(tup, 255)
    assert output == (0, 255, 0)
    tup = (12, 55, 27, 42)
    output = get_new_tuple(tup, 255)
    assert output == (0, 255, 0, 0)

Also, the pattern range(len(foo)) usually suggests we could do things in a better way. For instance, we could use enumerate:

return tuple(fixed_value*(i==index_max) for i, _ in enumerate(base_tuple))

Edge case

Another question we could ask ourselves is about the behavior of the function where the max value appears more than once. It probably deserves to be documented and properly tested. Based on what we want, we may have to change the implementation.

Another edge case that could be tested/documented is how the empty tuple is handled.


ADO.NET transaction handling Please let me know your feedback around handling ADO.NET connection and transaction, especially when it comes the catch block. Any way I could improve?

public static bool FileBulkLoad(string filePath, string tableName, string fieldSeparator, int numberOfSourceColumns)
{
    using (var connection = new SqlConnection(GetConnectionString()))
    {
        connection.Open();
        using (var transaction = connection.BeginTransaction(IsolationLevel.ReadCommitted))
        {
            try
            {
                foreach (var fileData in Helpers.StreamFile(filePath, tableName, fieldSeparator, numberOfSourceColumns))
                {
                    // BulkInsert uses SqlBulkCopy to insert data.
                    Helpers.BulkInsert(fileData.Data, tableName, connection, transaction);
                }

                transaction.Commit();
                return true;
            }
            catch (Exception)
            {
                transaction.Rollback();
                return false;
            }
        }
    }
}

FileBulkLoad isn't an ideal name for a method - it's a noun, and nouns should be used for naming types - method names should be verbs, because they do something.

The fact that it's static is also worrying - less so than that static Helpers, but worrying nonetheless: you're coupling your application logic with a specific data access strategy, which means you have no way of writing a unit test for your application logic without actually hitting the database and, in this case, the file system.

Consider this alternative:

public interface IFilePersistenceService
{
    bool LoadFromDelimitedFile(string path, string separator, int columns);
}

Now you can implement this interface with a class that will use Helpers.BulkInsert with ADO.NET, or one that uses a UnitOfWork and Entity Framework, or NHibernate, or whatever: the point is that bulk insert is a mere implementation detail that the rest of your code doesn't need to care about - that's why having it in the method's name doesn't really bring anything to the table.

Your client code would be written against this IFilePersistenceService abstraction, and then you could inject a fake implementation of that interface into your application logic, and write tests that don't hit the file system or a database.

I said Helpers was worrisome; the name doesn't convey anything but "stuff that serves some purpose, somewhere" - after a while it becomes a dumping ground for any code that doesn't quite belong elsewhere, or for anything that you need to access from anywhere - and that's how spaghettification begins.

Consider killing that (static?) class, with fire.

The implementation itself isn't half bad: you are disposing of disposables, using using blocks - it doesn't get any better than that IMO.

The problem is at a higher level: you're returning true on success, false on failure - and once the client code knows the operation failed, it doesn't know anything about the failure, that was all swallowed by the called method.

I'd much rather return void and have code that either works or blows up.

The Boolean return value is more hurtful than anything, it's the client code that knows how to handle an exception (e.g. display a message to the user about a permission issue, or email tech support with exception details, whatever): it's the client code that should wrap this method call with a try/catch block. Or the method should log the exception details and then rethrow.

I would expect a "load" method to fetch data from a data source, and return that data to the application. Perhaps this would be more appropriate:

public interface IFilePersistenceService
{
    void ImportFile(string path, string separator, int columns);
}

Right? You're importing data into a database, from a file.


Screen timeout/lock script This is a script I made while having trouble with xautolock and dimming of the screen right before locking it. I would like some tips for making it a bit more robust and to know of any disadvantages of it. I don't know how it works with multiple screens but I suspect it does not.

#!/bin/bash

# Lock screen in:
minutes=15
seconds=0

# How many seconds it takes to dim
dim_secs=5

idle_secs=$(( $minutes * 60 + $seconds )) # Screen timeout in seconds
idle_ms=$(( $idle_secs * 1000 ))

timeout=$idle_secs

while true; do
    sleep $timeout

    if pgrep xflock4 > /dev/null; then
        timeout=$idle_secs
        continue
    fi

    # After sleeping, check user idle time
    if [ $(xprintidle) -ge $idle_ms ]; then
        WINDOW=$(echo $(xwininfo -id $(xdotool getactivewindow) -stats | \
                        egrep '(Width|Height):' | \
                        awk '{print $NF}') | \
                sed -e 's/ /x/')
        SCREEN=$(xdpyinfo | grep -m1 dimensions | awk '{print $2}')

        # If greater than timeout check if something's not in fullscreen and active
        if [ "$WINDOW" != "$SCREEN" ]; then
            interrupted=false # If dimming's been interrupted

            screen=$(xrandr -q | grep " connected" | awk '{print $1;}') # Connected screen
            brightness=$(xrandr --verbose | grep -i brightness | cut -f2 -d ' ') # Current brightness

            start_time=$(date "+%s.%N") # Current time
            while true; do
                # Current brightness(0 to $brigtness), decided by time since dimming started
                br=$(echo "$brightness-$brightness*(($(date "+%s.%N")-$start_time)/$dim_secs)" | bc -l)
                if [ $(echo "$br >= 0" | bc) -eq 1  ]; then
                    # Set the current brightness and sleep for a short while
                    xrandr --output $screen --brightness $br
                    sleep 0.01
                else
                    # If brightness is less than 0 then it means that the
                    # time since starting dimming is greater than $dim_secs
                    break
                fi

                # If the user gave an input that's less than $idle_ms
                # Will always be less than $idle_ms if it was interrupted during dimming
                if [ $(xprintidle) -lt $idle_ms ]; then
                    interrupted=true
                    break
                fi
            done
            if $interrupted; then
                # If it was interrupted during dimming, reset brightness
                xrandr --output $screen --brightness $brightness
            else
                xset dpms force off # Turn screen off
                xrandr --output $screen --brightness $brightness # Reset brightness

                xflock4 # Lock screen
            fi
        fi
    fi

    # Timeout before checking idle time again
    timeout=$(echo "$idle_secs - $(xprintidle) / 1000 " | bc -l)
done

You don't need to prefix variables with $ inside $(( ... )),
this will work just as well:

idle_secs=$(( minutes * 60 + seconds ))  # Screen timeout in seconds
idle_ms=$(( idle_secs * 1000 ))

In many places you pipe to a grep and then pipe to awk.
Note that awk can perform many of the functions of grep,
and using one process (awk) instead of two (grep + awk) would be more optimal.

Here are some examples. Instead of:

... | egrep '(Width|Height):' | awk '{print $NF}'

This is equivalent:

... | awk '/(Width|Height):/ {print $NF}'

Instead of:

... | grep -m1 dimensions | awk '{print $2}'

This is equivalent:

... | awk '/dimensions/ {print $2; exit}'

And so on, similarly at other places too, you get the idea.

Another common pattern I see at many places:

echo "$br >= 0" | bc -l

That is, using echo to pass a string to a command.
A better way is using here-strings, like this:

bc -l <<< "$br >= 0"

This long line appears at multiple places, sometimes with minor variations:

# If it was interrupted during dimming, reset brightness
xrandr --output $screen --brightness $brightness

It would be better to move this to a function,
for example reset_brightness,
to reduce duplication of logic.


Efficiency and length of functions when perfomring similar tasks to classes and IDs I'm not proficient at jQuery, however can tinker and get basic functionality working as I would like. The below code is performing a number of functions to change states of some elements in my site, and it is all working correctly.

However, I'm sure it is not the most efficient way of achieving this (fairly simple) functionality, so I'm hoping somebody can help me to make this much leaner and more efficient?

I have commented the code to explain what each line should be achieving, essentially adding and removing classes to a a number of elements. As you can see, a lot of the lines are repeated to accommodate the different states outlined in the comments, but I'm sure something perhaps like .siblings could be used to reduce the amount of code.

Any help greatly appreciated!

JS

$(document).ready(function () {
    $('#menu-button').click(function(){
        // MENU BUTTON STATES
        // If the menu button is active, then remove all classes to return to initial state
        if ($(this).hasClass('active')) {
            $(this).removeClass();
        // When in initial state, add active class
        } else {
            $(this).addClass('active');
        }

        // OVERLAY STATES
        // If in menu state, clicking menu button removes all #overlay classes to return to initial state
        if ($('#overlay').hasClass('active menu')) {
            $('#overlay').removeClass();
        // If in services state, clicking menu button removes all #overlay classes and add classes to return to menu state
        } else if ($('#overlay').hasClass('active services')) {
            $('#overlay').removeClass().addClass('active menu');
        // If in about state, clicking menu button removes all #overlay classes and add classes to return to menu state
        } else if ($('#overlay').hasClass('active about')) {
            $('#overlay').removeClass().addClass('active menu');
        // If in contact state, clicking menu button removes all #overlay classes and add classes to return to menu state
        } else if ($('#overlay').hasClass('active contact')) {
            $('#overlay').removeClass().addClass('active menu');
        // When in initial state, add classes to change to menu state
        } else {
            $('#overlay').addClass('active menu');
        }
        return false;
    });

    $('.services a').click(function(){
        // When clicking this menu item, remove all #overlay classes and add classes to return to services state
        $('#overlay').removeClass().addClass('active services');
        // Also, return menu button to initial state
        $('#menu-button').removeClass();
        return false;
    });

    $('.about a').click(function(){
        // Same as above but for about state
        $('#overlay').removeClass().addClass('active about');
        $('#menu-button').removeClass();
        return false;
    });

    $('.contact a').click(function(){
        // Same as above but for contact state
        $('#overlay').removeClass().addClass('active contact');
        $('#menu-button').removeClass();
        return false;
    });

    $('.logo').click(function(){
        // When clicking .logo, return everythign to initial state
        $('#overlay').removeClass();
        $('#menu-button').removeClass();
        return false;
    });

});

Use toggleClass instead of:

if ($(this).hasClass('active')) {
    $(this).removeClass();
// When in initial state, add active class
} else {
    $(this).addClass('active');
}

You can map the event handlers for multiple classes:

['about', 'contact', ...].map(cls => {$(`.${cls} a`).click(...)}


Created my first wordpress plugin template page, need comments I managed to create my first wordpress plugin template page collecting various code snippets to make the page function.

Everything works fine, but I need hear comments from PHP & wordpress experts to see if it's well-coded.

Here's the code.

<?php
/*
Plugin Name: name
Plugin URI: http://www.example.com
Description: desc
Version: 1
Author: someone
Author URI: http://www.example
*/
?>
<?php
function my_first_plugin_init(){
    register_setting( 'my_first_plugin_grp', 'my_first_plugin' );
}
add_action( 'admin_init', 'my_first_plugin_init' );

function my_first_plugin_add_page() {
    add_menu_page('plugin title', 'plugin title', 'manage_options', 'my_first_plugin', 'my_first_plugin_do_page' );
}
add_action( 'admin_menu', 'my_first_plugin_add_page' );

function my_first_plugin_scripts() {
    wp_enqueue_script('media-upload');
    wp_enqueue_script('thickbox');
    wp_enqueue_script('my-upload', plugin_dir_url( __FILE__ ).'/upload-func.js');
}
add_action('admin_print_scripts', 'my_first_plugin_scripts');

function my_first_plugin_styles() {
    wp_enqueue_style('ads-stls', plugin_dir_url( __FILE__ ).'/ads-options.css');
    wp_enqueue_style('thickbox');
}
add_action('admin_print_styles', 'my_first_plugin_styles');

function my_first_plugin_do_page() {
    global $select_options, $radio_options;
    if ( ! isset( $_REQUEST['settings-updated'] ) )
    $_REQUEST['settings-updated'] = false;
?>

<?php echo $plgnme; ?>

    <?php if ( false !== $_REQUEST['settings-updated'] ) : ?>

Options saved.

    <?php endif; ?>
    <form method="post" action="options.php">
    <?php settings_fields( 'my_first_plugin_grp' ); ?>
    <?php $options = get_option( 'my_first_plugin' ); ?>

Sample text input:
<input id="my_first_plugin[lpgrdneurl]" class="regular-text" type="text" name="my_first_plugin[lpgrdneurl]" value="<?php esc_attr_e( $options[$lpgrdneurl] ); ?>" placeholder="http://" />

    <?php submit_button(); ?>
    </form>

<?php } ?>

First of all, your plugin is not translation ready, see l18n. In the plugin header, you should add

* Text Domain: your-textdomain
* Domain Path: /languages

Create the language files in wp-content/plugins/your-plugin/languages with PoEdit. And make all relevant strings translatable with:

add_menu_page( __('Plugin title','your-textdomain'), ...
// and
<?php _e('Options saved','your-textdomain'); ?>

There's a big issue: you're printing your styles and scripts all over the admin area. It's essential that you always target the correct page.

add_action( 'admin_menu', 'my_first_plugin_add_page' );
function my_first_plugin_add_page()
{
    $hook = add_menu_page(
        'plugin title',
        'plugin title',
        'manage_options',
        'my_first_plugin',
        'my_first_plugin_do_page'
    );
    add_action( "admin_print_scripts-$hook", 'my_first_plugin_scripts' );
}

You don't need to close and open PHP tags if you're not swapping between PHP and HTML:

*/
?>
<?php
function my_first_plugin_init(){

Get rid of the very last ?> too.
See Why do some scripts omit the closing php tag ?>.

It's easier to have a general picture if you declare first the hooks and then the callbacks.

add_action('first_hook', 'prefix_first_callback');
add_action('second_hook', 'prefix_second_callback');
add_filter('third_hook', 'prefix_third_callback');
function prefix_first_callback() {}
function prefix_second_callback() {}
function prefix_third_callback() {}

It's important to prefix everything, function names, meta data and $_POSTed data. But for function names this quickly turns into hard to read code. Encapsulating everything in a class avoids this. And also allows us to start-up our plugin in a safe point: plugins_loaded, see Best way to initiate a class in a WP plugin?.

The following is a clean version of Plugin Class Demo, by toscho◆. Please, go to the source for the full documented class.

add_action(
    'plugins_loaded',
    array ( PREFIX_My_Plugin::get_instance(), 'plugin_setup' )
);

class PREFIX_My_Plugin
{
    protected static $instance = NULL;
    public $plugin_url = '';
    public $plugin_path = '';

    public static function get_instance()
    {
        NULL === self::$instance and self::$instance = new self;
        return self::$instance;
    }

    public function plugin_setup()
    {
        $this->plugin_url    = plugins_url( '/', __FILE__ );
        $this->plugin_path   = plugin_dir_path( __FILE__ );
        $this->load_language( 'plugin_unique_name' );
        // Your stuff: register actions and filters
        add_action( 'admin_init', array( $this, '_admin_init' ) );
    }

    public function _admin_init()
    {
        // your_stuff();
    }

    public function __construct() {}

    public function load_language( $domain )
    {
        load_plugin_textdomain(
            $domain,
            FALSE,
            $this->plugin_path . 'languages'
        );
    }
}


Node.js Is this a great way to show pages in a minimal way as possible? I'm trying to make a Node.js server as minimal as possible to show a landing page, but i'm not sure this is the greatest and fastest way to do it.

I'm using dirty to store my html data.

My app.js file

var http = require('http')
, db = require('dirty')('pages.db')
, url = require('url')
, fs = require('fs');

http.createServer(function (request, response) {
    var pathname = url.parse(request.url).pathname;
    var html = db.get(pathname);

    //Check if the requsted file is CSS or JS
    if (/\.(css)$/.test(pathname) || /\.(js)$/.test(pathname)){

        fs.createReadStream(__dirname + pathname, {
            'bufferSize': 4 * 1024
        }).pipe(response)

    } else if (!!html && pathname !== '/admin' ) {
        //Pages from our Dirty DB
        response.writeHead(200, {'Content-Type': 'text/html'});
        response.end(html);
    } else if (pathname === '/admin') {
        //Display Admin page
        response.writeHead(200, {'Content-Type': 'text/html'});
        response.end('Admin!');
    } else {
        //Show 404 Page
        response.writeHead(404, {'Content-Type': 'text/html'});
        response.end(db.get('404'));
    }
 }).listen(80);

console.log('Server running at http://192.168.56.101:80/');

Good question,
made me learn some more about dirty.

Some observations:

1 var block is good, having the commas not at the end of each line looks odd
I wonder if it is faster to get the extension of the requested resource and compare to js and css instead of executing 2 regexes
Your 3 if blocks contain the same statement: response.writeHead(200, {'Content-Type': 'text/html'}); you should extract that statement into 1 place.
'80' should be in a var which you then use for your last console.log statement
Similarly, hard coding the ip address in your log statement does not make sense, you should get that info from node.
Always calculating 4*1024 and creating an options object seems like a lot work, that ought to get done only once
*

I would counter propose something like this:

var http = require('http'),
    db = require('dirty')('pages.db'),
    url = require('url'),
    fs = require('fs'),
    port = 80,
    streamOptions = { 'bufferSize': 4 * 1024 },
    htmlHeader = {'Content-Type': 'text/html'};

http.createServer(function (request, response)
{
  var pathname = url.parse(request.url).pathname;
  var html = db.get(pathname);

  //First check if the requsted file is css or js
  if (/\.(css)$/.test(pathname) || /\.(js)$/.test(pathname))
  {
    fs.createReadStream(__dirname + pathname,streamOptions).pipe(response);
  }
  else
  {
    response.writeHead(200, htmlHeader );
    if (!!html && pathname !== '/admin' )
    { //Pages from our Dirty DB
      response.end(html);
    }
    else if (pathname === '/admin')
    { //Display Admin page
      response.end('Admin!');
    }
    else
    { //Not found..
      response.end(db.get('404'));
    }
  }
 }).listen(port);

console.log('Server running on port' , port );

If you absolutely want to show the IP address, then you should check out this excellent question.

Finally, any hacker can access your admin pages with /./admin tricks unless either dirty or url is smarter than I give it credit for.


Eliminating files that are completely contained within other files I have a block of code that is really bugging me but I can't figure out how to clean it up. I have a list of objects which are all related. In fact some of them are complete supersets of one or more others.

To identify the 'Dupes' which are completely contained within other files, I am comparing every file against every other. Once a file is flagged as a dupe it doesn't need to be included in the comparisons anymore.

The function isbetter determines when a file is a superset.

for a in self.files:
    if not a.isDupe:
        for b in self.files:
            if  a != b and not b.isDupe:
                if a.isbetter(b):
                    b.isDupe = a.name

I'm sure there is a very pythonic way to simplify this.

It looks like you want to filter out the dupes, so let's apply that to self.files first.

noDupeFiles = self.files # no dupes at the start, so we initialize it to self.files
for a in noDupeFiles:
    for b in noDupeFiles:
        if a != b and a.isbetter(b):
            b.isDupe = a.name
            # got to remove b once we make it a dupe file
            noDupeFiles.remove(b)

This reduces the number of levels from 6 to 4 (since we removed the not x.isDupe checks). Next, we combine for a in noDupeFiles and for b in noDupeFiles with the help of itertools.permutations.

noDupeFiles = self.files
for a, b in itertools.permutations(noDupeFiles, 2):
    if a != b and a.isbetter(b):
        b.isDupe = a.name
        noDupeFiles.remove(b)

This reduces from 4 to 3 levels while keeping the original logic.

EDIT:

@marcp pointed out that removing elements from the list apparently threw an exception, so here's the new version:

for a, b in itertools.permutations(self.files, 2):
    if not (a.isDupe or b.isDupe) and a.isbetter(b):
        b.isDupe = a.name


ConvertAll Methods Implementation for Multidimensional Array in C# - follow-up This is a follow-up question for ConvertAll Methods Implementation for Multidimensional Array in C#. Thanks to aepot's answer and Olivier's answer. In order to match the usage of the build-in API Array.ConvertAll, the input /output array types of the implemented methods below are similar to [], [,], [,,] and etc. style.
The experimental implementation
The experimental implementation is as below.
public static TOutput[,] ConvertAll<TInput, TOutput>(TInput[,] array, Converter<TInput, TOutput> converter)
    where TInput : unmanaged
    where TOutput : unmanaged
{
    return (TOutput[,])ConvertArray(array, converter);
}

public static TOutput[,,] ConvertAll<TInput, TOutput>(TInput[,,] array, Converter<TInput, TOutput> converter)
    where TInput : unmanaged
    where TOutput : unmanaged
{
    return (TOutput[,,])ConvertArray(array, converter);
}

public static TOutput[,,,] ConvertAll<TInput, TOutput>(TInput[,,,] array, Converter<TInput, TOutput> converter)
    where TInput : unmanaged
    where TOutput : unmanaged
{
    return (TOutput[,,,])ConvertArray(array, converter);
}

public static TOutput[,,,,] ConvertAll<TInput, TOutput>(TInput[,,,,] array, Converter<TInput, TOutput> converter)
    where TInput : unmanaged
    where TOutput : unmanaged
{
    return (TOutput[,,,,])ConvertArray(array, converter);
}

public static TOutput[,,,,,] ConvertAll<TInput, TOutput>(TInput[,,,,,] array, Converter<TInput, TOutput> converter)
    where TInput : unmanaged
    where TOutput : unmanaged
{
    return (TOutput[,,,,,])ConvertArray(array, converter);
}

public static TOutput[,,,,,,] ConvertAll<TInput, TOutput>(TInput[,,,,,,] array, Converter<TInput, TOutput> converter)
    where TInput : unmanaged
    where TOutput : unmanaged
{
    return (TOutput[,,,,,,])ConvertArray(array, converter);
}

public static TOutput[,,,,,,,] ConvertAll<TInput, TOutput>(TInput[,,,,,,,] array, Converter<TInput, TOutput> converter)
    where TInput : unmanaged
    where TOutput : unmanaged
{
    return (TOutput[,,,,,,,])ConvertArray(array, converter);
}

public static TOutput[,,,,,,,,] ConvertAll<TInput, TOutput>(TInput[,,,,,,,,] array, Converter<TInput, TOutput> converter)
    where TInput : unmanaged
    where TOutput : unmanaged
{
    return (TOutput[,,,,,,,,])ConvertArray(array, converter);
}

public static TOutput[,,,,,,,,,] ConvertAll<TInput, TOutput>(TInput[,,,,,,,,,] array, Converter<TInput, TOutput> converter)
    where TInput : unmanaged
    where TOutput : unmanaged
{
    return (TOutput[,,,,,,,,,])ConvertArray(array, converter);
}

static Array ConvertArray<T, TResult>(Array array, Converter<T, TResult> converter)
    where T : unmanaged
    where TResult : unmanaged
{
    if (array is null)
    {
        throw new ArgumentNullException(nameof(array));
    }

    if (converter is null)
    {
        throw new ArgumentNullException(nameof(converter));
    }

    long[] dimensions = new long[array.Rank];
    for (int i = 0; i < array.Rank; i++)
        dimensions[i] = array.GetLongLength(i);

    Array result = Array.CreateInstance(typeof(TResult), dimensions);
    TResult[] tmp = new TResult[1];
    int offset = 0;
    int itemSize = System.Runtime.InteropServices.Marshal.SizeOf(typeof(TResult));
    foreach (T item in array)
    {
        tmp[0] = converter(item);
        Buffer.BlockCopy(tmp, 0, result, offset * itemSize, itemSize);
        offset++;
    }
    return result;
}

All suggestions are welcome.
The summary information:

Which question it is a follow-up to?
ConvertAll Methods Implementation for Multidimensional Array in C#

What changes has been made in the code since last question?
Trying to perform higher dimensional overloadings and exception handling based on aepot's answer.

Why a new review is being asked for?
If there is any possible improvement or any other potential defect, please let me know.

When I see that many recurrences like this then it is a good sign for me that this is a perfect place to introduce meta-programming via T4.
Step #1
Separate the overloads from the core logic:
using System;

namespace Sample.Core
{
    internal static class Converter
    {
        public static Array ConvertArray<T, TResult>(Array array, Converter<T, TResult> converter)
            where T : unmanaged
            where TResult : unmanaged
        {
            if (array is null)
            {
                throw new ArgumentNullException(nameof(array));
            }

            if (converter is null)
            {
                throw new ArgumentNullException(nameof(converter));
            }

            long[] dimensions = new long[array.Rank];
            for (int i = 0; i < array.Rank; i++)
                dimensions[i] = array.GetLongLength(i);

            Array result = Array.CreateInstance(typeof(TResult), dimensions);
            TResult[] tmp = new TResult[1];
            int offset = 0;
            int itemSize = System.Runtime.InteropServices.Marshal.SizeOf(typeof(TResult));
            foreach (T item in array)
            {
                tmp[0] = converter(item);
                Buffer.BlockCopy(tmp, 0, result, offset * itemSize, itemSize);
                offset++;
            }
            return result;
        }
    }
}

Step #2
Introduce a *.tt file with the following content:
<#@ template language="C#" #>
<#@ assembly name="System.Core" #>
<#@ import namespace="System.Linq" #>
<#@ import namespace="System.Text" #>
<#@ import namespace="System.Collections.Generic" #>
using System;
using static Sample.Core.Converter;

namespace Sample.Overloads
{
    public static class ArrayConverter
    {
<#
for(int i = 1; i < 10; i++)
{
var dimensions = new String(',', i);
#>
        public static TOutput[<#=dimensions#>] ConvertAll<TInput, TOutput>(TInput[<#=dimensions#>] array, Converter<TInput, TOutput> converter)
            where TInput : unmanaged
            where TOutput : unmanaged
        {
            return (TOutput[<#=dimensions#>])ConvertArray(array, converter);
        }
<#
}
#>
    }
}

Step #3
Execute the tt file and examine the generated code:
using System;
using static Sample.Core.Converter;

namespace Sample.Overloads
{
    public static class ArrayConverter
    {
        public static TOutput[,] ConvertAll<TInput, TOutput>(TInput[,] array, Converter<TInput, TOutput> converter)
            where TInput : unmanaged
            where TOutput : unmanaged
        {
            return (TOutput[,])ConvertArray(array, converter);
        }
        public static TOutput[,,] ConvertAll<TInput, TOutput>(TInput[,,] array, Converter<TInput, TOutput> converter)
            where TInput : unmanaged
            where TOutput : unmanaged
        {
            return (TOutput[,,])ConvertArray(array, converter);
        }
        public static TOutput[,,,] ConvertAll<TInput, TOutput>(TInput[,,,] array, Converter<TInput, TOutput> converter)
            where TInput : unmanaged
            where TOutput : unmanaged
        {
            return (TOutput[,,,])ConvertArray(array, converter);
        }
        public static TOutput[,,,,] ConvertAll<TInput, TOutput>(TInput[,,,,] array, Converter<TInput, TOutput> converter)
            where TInput : unmanaged
            where TOutput : unmanaged
        {
            return (TOutput[,,,,])ConvertArray(array, converter);
        }
        public static TOutput[,,,,,] ConvertAll<TInput, TOutput>(TInput[,,,,,] array, Converter<TInput, TOutput> converter)
            where TInput : unmanaged
            where TOutput : unmanaged
        {
            return (TOutput[,,,,,])ConvertArray(array, converter);
        }
        public static TOutput[,,,,,,] ConvertAll<TInput, TOutput>(TInput[,,,,,,] array, Converter<TInput, TOutput> converter)
            where TInput : unmanaged
            where TOutput : unmanaged
        {
            return (TOutput[,,,,,,])ConvertArray(array, converter);
        }
        public static TOutput[,,,,,,,] ConvertAll<TInput, TOutput>(TInput[,,,,,,,] array, Converter<TInput, TOutput> converter)
            where TInput : unmanaged
            where TOutput : unmanaged
        {
            return (TOutput[,,,,,,,])ConvertArray(array, converter);
        }
        public static TOutput[,,,,,,,,] ConvertAll<TInput, TOutput>(TInput[,,,,,,,,] array, Converter<TInput, TOutput> converter)
            where TInput : unmanaged
            where TOutput : unmanaged
        {
            return (TOutput[,,,,,,,,])ConvertArray(array, converter);
        }
        public static TOutput[,,,,,,,,,] ConvertAll<TInput, TOutput>(TInput[,,,,,,,,,] array, Converter<TInput, TOutput> converter)
            where TInput : unmanaged
            where TOutput : unmanaged
        {
            return (TOutput[,,,,,,,,,])ConvertArray(array, converter);
        }
    }
}
```


How might I make this code more DRY? I have the following Ruby code in an RSpec file:

  describe "order" do
    before do
      LIST_LENGTH ||= 10
      @skills = FactoryGirl.create_list(:skill, LIST_LENGTH)
      @developer = FactoryGirl.create(:user)
      @requests = FactoryGirl.create_list(:request, LIST_LENGTH)
      LIST_LENGTH.times { |i| @requests[i].skills = @skills[0..i] }
    end

    describe "#order_by_interestingness_for" do
      it "orders by interestingness" do
        @developer.add_interested_skills(@skills)
        Request.all.shuffle.map {
          |r| r.interestingness_for(@developer)
        }.should_not be_in_order

        Request.order_by_interestingness_for(@developer).map {
          |r| r.interestingness_for(@developer)
        }.should be_in_order
      end
    end

    describe "#order_by_qualifiedness_for" do
      it "orders by interestingness" do
        @developer.add_proficient_skills(@skills)
        Request.all.shuffle.map {
          |r| r.qualifiedness_for(@developer)
        }.should_not be_in_order

        Request.order(:interestingness, :developer => @developer).map {
          |r| r.qualifiedness_for(@developer)
        }.should be_in_order
      end
    end
  end

Obviously, this is not very DRY. I don't really know how to make it more DRY, though, since the the unique parts are so deeply ingrained in the code. I'm guessing I have a deeper design problem. I'm not really sure, though. Any ideas?

Some suggestions:

There's no need for the negative assertion. You're testing Ruby more than you're testing your own code and in the end, we don't actually care. It only matters that our attempt to order the results behaves as expected. Assuming they're un-ordered otherwise (or at least order isn't guaranteed).
Favor RSpec's 'let' over instance variables and constants in your tests.
RSpec's 'should' syntax will be deprecated in the next release. Favor using the new 'expect' syntax instead.
Use string literals when you can.
Use a shared example to DRY up your tests a bit.
Modify the underlying implementation and the public API in your code for better symmetry between ordering by 'interestingness' and 'qualifiedness'. I suspect there's some code duplication in there given the current API that could also be eliminated.

Do all that, and you could end up with a test that looks something like this:

shared_examples_for 'a ordered request' do |order_type, dev|
  it "orders by #{order_type}" do
    result = Request.call(:order, { order_type, :developer => dev })
    result.map! { |r| r.send("#{order_type}_for", dev) }
    expect(result).to be_in_order
  end
end

describe Request do
  let(:list_length) { 10 }
  let(:skills) { FactoryGirl.create_list(:skill, list_length) }
  let(:developer) { FactoryGirl.create(:user) }
  let(:requests) { FactoryGirl.create_list(:request, list_length) }

  before { list_length.times { |i| requests[i].skills = skills[0..i] } }

  describe '#order' do
    context 'when ordering by interestingness'
      before { developer.add_interested_skills(skills) }
      it_behaves_like 'a ordered request', 'interestingness', developer
    do

    context 'when ordering by qualifiedness' do
      before { developer.add_proficient_skills(skills) }
      it_behaves_like 'a ordered request', 'qualifiedness', developer
    end
  end
end

I'd also recommend checking out this site which is a pretty good, but still growing style guide for writing behavior tests in RSpec.


Building query for search engine I'm writing a basic search engine for my website.

It works but I'm really unhappy with the code. I know it can't be improved but I'm lacking some experiences here.
In my params from my search I get:

{"utf8"=>"✓", "authenticity_token"=>"aNY2ZVpeyZsLnIVegQPpPz/22VYx2ug+tbfu0Nzngac=", "skills"=>"3,8", "country"=>"USA", "state"=>"TX", "city"=>"Paris", "commit"=>"Search", "controller"=>"jobs", "action"=>"index"}

Here is my controller:

class JobsController < ApplicationController
  before_action :authenticate_user!, :except => [:index]

  def index
    search_jobs
  end

  def search_jobs
    search_filters = search_params
    if search_filters.present?
      skills = search_filters[:skills]
      city, state, country = search_filters[:city], search_filters[:state], search_filters[:country]
      search_jobs = Job.select(:id).joins(:skills)
      search_jobs = search_jobs.where(build_where(skills)) if skills.present?
      search_jobs = search_jobs.group("jobs.id")
      search_jobs = search_jobs.group("jobs.city, jobs.state") if city.present? || state.present?
      search_jobs = search_jobs.group("jobs.created_at")
      search_jobs = search_jobs.order("city <> '#{city}'") if city.present?
      search_jobs = search_jobs.order("state <> '#{state}'") if state.present?
      search_jobs = search_jobs.order("country <> '#{country}'") if country.present?
      search_jobs = search_jobs.order("count(*) desc")
      search_jobs = search_jobs.order('jobs.created_at DESC')
      jobs_id = search_jobs.map(&:id)
    end
    jobs = Job.includes(:company, :skills)
    jobs = jobs.where(id: jobs_id) if jobs_id.present?
    jobs = jobs.most_recent
    @jobs = jobs
  end

  def build_where(skills_filters)
    skills = skills_filters.split(", ")
    query = "'skills'.'id' = #{skills.first}"
    skills.shift
    skills.each do |skill|
      query << " OR 'skills'.'id' = #{skill}"
    end
    return query
  end

  private

  def search_params
    params.permit(:skills, :country, :city, :state)
  end

end

Few, things:

I'm not happy with the build_where method, but I can't think of a way to build a where dynamically based on the skills the users are looking for.
The search_jobs seems the be a bad idea. When I run this in the developer console it looks like each line is executing a SQL query which is an issue performance wise. 10 queries  to get a list of IDs seems to be terrible
Then I'm doing 3 more queries afterward: to get all the jobs with all the information needed.

So, I think my code is not really good, but I'm not sure how I can improve it. My goal overall is

There are some bugs related to parameter handling:

Splitting by ", ": In #build_where, you split the skill parameter by ", ".  However, you example has just "skills"=>"3,8" — with no space after the comma.  Splitting will therefore fail.
SQL injection vulnerability: Parts of the value of the skills parameter end up in the SQL query's WHERE clause, but you never validated, escaped, or parameterized the query.  You use the city, state, and country parameters with similar carelessness, interpolating values directly into an ORDER BY clause.  If a user tries to specify St. John's as the city, the query will break due to the apostrophe being treated as a quotation mark.  A malicious user can exploit that to do nasty things to your database.

I also suspect that

Job.select(:id).joins(:skills)

is the wrong thing to do.  That produces an INNER JOIN.  Any job that has no special required skills will therefore not appear in the join result.  I'm guessing that a LEFT OUTER JOIN would be more appropriate.

As for the skills filter itself, the WHERE clause you want in SQL is

WHERE "skills"."id" IN (3, 8)

(Writing 'skills'.'id' results in much brokenness.  To quote identifiers in PostgreSQL, use double-quotes.  Single quotes are used to quote literal strings, not identifiers.)

I would therefore write it as

def restrict_skills(search_jobs, skills_filters)
  # Check for non-empty string consisting only of digits, commas, and spaces
  if skills_filters.match(/\A[0-9, ]+\Z/)
     search_jobs.where(%Q{"skills"."id" IN (#{skills_filters})})
  end
end

Usually, I recommend parameterizing queries.  Escaping is my second choice.  Validating is normally my third choice.  However, here I've used validation instead of parameterizing and escaping because validation is easy: just check that the skills parameter consists of just digits, commas, and spaces.  It also happens that the SQL IN clause expects comma-delimited integers here.

I've also changed the helper method to manipulate the query-building object rather than to return a fragment of SQL.


Find hamming distance between two Strings of equal length in Java I have a private class that I want to be able to find the shortest Hamming Distance between two Strings of equal length in Java. The private class holds a char[] and contains a method to compare against other char arrays. It returns true if there is only a hamming distance of one.

The average length of the Strings used is 9 characters. They range from 1 to 24 characters.

Is there a way to make the isDistanceOne(char[]) method go any faster?

private class WordArray {

  char[] word;

  /**
   * Creates a new WordArray object.
   *
   * @param word The word to add to the WordArray.
   */
  private WordArray(String word) {
     this.word = word.toCharArray();
  }

  /**
   * Returns whether the argument is within a Hamming Distance of one from
   * the char[] contained in the WordArray.
   *
   * Both char[]s should be of the same length.
   *
   * @param otherWord The word to compare with this.word.
   * @return boolean.
   */
  private boolean isDistanceOne(char[] otherWord) {

     int count = 0;

     for(int i = 0; i < otherWord.length; i++) {
        if (this.word[i] != otherWord[i])
           count++;
     }

     return (count == 1);
  }
}

Given the limited context, and no information about where the hotspot is in the code, it's difficult to give concrete advice. Here are some musings for your consideration:

For ease of reading, it's preferable to have whitespace after control flow keywords and before the (.

It is suggested to always include curly braces, even when they're not required by the compiler.

Use final where possible to reduce cognitive load on the readers of your code.

word should be private.

There's no apparent reason to use char[] instead of just keeping a pointer to the original String. They're costing you time and space to make, to no benefit.

You can short-circuit out of your for loop if the count ever becomes greater than one. Unless a significant fraction of your inputs have a distance of one, you should see some performance gain here.

Using a boolean instead of an int might make a very small difference in execution time, but that would need to be tested. It also makes the code harder to read.

private class WordArray {

    private final String word;

    private WordArray(final String word) {
        this.word = word;
    }

    private boolean isDistanceOne(final char[] otherWord) {
        assert word.length() == otherWord.length;

        int distance = 0;

        for (int i = 0; i < otherWord.length; i++) {
            if (this.word.charAt(i) == otherWord[i]) {
                continue;
            }

            if (distance > 0) {
                return false;
            }

            distance++;
        }

        return distance == 1;
    }
}


nTier architecture for an MVC application I am endeavoring to build my first non-trivial web application and so I'd like to confirm a couple of design decisions.

I have three tiers as follows:

UI (my MVC project)
Business Logic (separate project)
Data Access Logic (a simple .dbml that is part of project #2)

Is this a reasonable approach to separating concerns? Should my DAL consist of more than just my Linq .dbml file?

To play this out, lets say I have an Orders table and a Shipments table in my database.  Furthermore lets say that I want to pass a list of orders and shipments through to a page in the UI.  Here's how I'm envisioning that would work.

OrdersBLL.cs

Public IEneumerable<Order> GetAllOrders()
{
DataContext db = new DataContext();
 return db.Orders;

}

ShipmentsBLL.cs

Public IEneumerable<Shipment> GetAllShipments()
{
DataContext db = new DataContext();
return db.Shipments;
}

ViewModels.cs

public class ShowAll
{
    public IQueryable<Order> order { get; set; }
    public IQueryable<Shipment> shipment { get; set; }
}

Then when I create the view in the MVC app to display all orders and shipments I would simply pass in a ShowAll object from the ViewModel.

Over all I think this approach separates concerns reasonably well.  However, I'd be interested in learning if others have a better approach.  One concern I have is that my approach requires instantiating Order and Shipment objects in the View Model.  However, these come from classes that are defined in my data layer.  This strikes me as a bad idea given that I read somewhere that only the BLL should talk to the DAL.  Is this correct?

An accepted design pattern for separation of concerns is the Repository design pattern.  There is a pretty good description of using this design pattern in MVC here. Your example is close to this pattern in your BLL layer, although your simple example does not have any business logic and just encapsulates the DAL.  If you need to access multiple Repositories for your view it is recommended to use a Service Layer that coordinates transactions between multiple Repositories.  There is a good description of this pattern here.

Your View Model should be totally separated from the BLL and not query it directly. I assume you are querying the results in the View since you are returning an IQueryable object.  The controller for the MVC should take any query parameters from the View, pass them to the Model (Repository or Service Layer) and get back an object graph that represents the View Model, which in turn is passed to the View.  The View Model usually does not clone the objects that are manipulated in the Repository.  For example you would not need to pass things like foreign keys used for navigation in the DAL to the View Model.  Only pass to the View Model what it needs to get its job done, which is to allow the View to render a user interface.  This usually requires a lot of data mapping from one object to another. Although it is a hassle upfront you will appreciate it going forward because you will have a code base that is not fragile to changes in either your model or view.  A good tool to help with the data mapping is AutoMapper.
For a great example on how to create a robust modern website using MVC take a look at Microsoft's Silk Project.