Typed NSUserDefaults I was looking around for a Swift wrapper around NSUserDefaults and found some very nice projects (see e.g. SwiftyUserDefaults). Unfortunately, it all boils down to stringy keys galore...

Eventually, I tried something myself and came up with the code below. I'm tempted to use it in production, but do not have much experience with user defaults and would be grateful for any comments regarding the safety of the solution below:

We start by declaring a pair of protocols that we can use to tag the types we wish to support:

public protocol UserDefaultsObject: AnyObject {}
public protocol UserDefaultsValue {
    var userDefault: UserDefaultsObject { get }
    init(UserDefaultsObject)
}

Tag all the classes supported by NSUserDefaults:

import Foundation

extension NSNumber: UserDefaultsObject {}
extension NSString: UserDefaultsObject {}
// etc...

We declare a struct because that allows us to decide on mutability simply but instantiating a UserDefault value as a let or a var:

public struct UserDefault<T: UserDefaultsValue> {

    public let key: String
    public var value: T? {
        get { return store }
        set {
            store = newValue
            NSUserDefaults.standardUserDefaults().setObject(store?.userDefault, forKey: key)
        }
    }

    public init(key: String, defaultValue: T?) {
        self.key = key
        if let object: AnyObject = NSUserDefaults.standardUserDefaults().objectForKey(key) {
            let userDefaultsObject = object as! UserDefaultsObject // this is the crucial line
            store = T(userDefaultsObject)
        } else {
            store = defaultValue
        }
    }

    private var store: T?
}

Now we can extend as many types as we wish. Let's start with Int:

extension Int: UserDefaultsValue {
    public var userDefault: UserDefaultsObject {
        return NSNumber(integer: self)
    }
    public init(_ object: UserDefaultsObject) {
        self = (object as! NSNumber).integerValue
    }
}

Which can be used like so:

var life = UserDefault(key: "Life %", defaultValue: 100) {
    didSet {
        // observe
    }
}

life.value = 50

We can add support for String:

extension String: UserDefaultsValue {
    public var userDefault: UserDefaultsObject {
        return NSString(string: self)
    }
    public init(_ object: UserDefaultsObject) {
        self = object as! String
    }
}

And use it like so (note that we can start with a nil default):

var answer = UserDefault<String>(key: "answer to life the universe and everything", defaultValue: nil)

answer.value ?? "42"

answer.value = "42"

answer.value //--> 42

And so on, we can similarly extend any type whatsoever, as long as we can think of a way of converting it to and from UserDefaultObject...

I'm primarily concerned with safety at this point (will this launch any missiles, etc.), but would certainly welcome any ideas about possible improvements as well as discussion about the general approach of wrapping individual user default values rather than NSUserDefaults class or its standard instance.

Our constructors would be better as failable initializers.

Nothing prevents us from passing a wrapped NSNumber to the String initializer, or a wrapped String to the Int initializer.  In your code, this would simply crash.  It would be better if our initializers were allowed to fail.

First, we need to change the protocol to define a failable initializer rather than a non-failable initializer:

public protocol UserDefaultsValue {
    var userDefault: UserDefaultsObject { get }
    init?(UserDefaultsObject)
}

Now, write the initializers as failable:

extension Int: UserDefaultsValue {
    public var userDefault: UserDefaultsObject {
        return NSNumber(integer: self)
    }
    public init?(_ object: UserDefaultsObject) {
        if let number = object as? NSNumber {
            self = number.integerValue
        } else {
            return nil
        }
    }
}

We can clean up our UserDefault constructor a bit as well.

if let object: AnyObject = NSUserDefaults.standardUserDefaults().objectForKey(key) {
    let userDefaultsObject = object as! UserDefaultsObject // this is the crucial line
    store = T(userDefaultsObject)
}

Like our initializers which should be failable, this runs the risk of crashing if object turns out to be some type which isn't a UserDefaultsObject.  We can clean this up slightly though.  This can look cleaner and be safer:

if let object = NSUserDefaults.standardUserDefaults().objectForKey(key) as? UserDefaultsObject {
    store = T(object)
}

The conditional will fail if:

There isn't already an object for this key.
Whatever NSUserDefaults returns for this key cannot be interpreted as a UserDefaultsObject.

There is one major concern I have, however, and I don't know of the right solution right now.

Consider the following situation, I save an NSNumber object to the key "abckey".  Now, I do this:

let s = UserDefault(key: "abckey", defaultValue: "Hello World")

In your version of the code, this will happen will crash because this line:

let userDefaultsObject = object as! UserDefaultObject

Will have no problem converting an NSNumber to a UserDefaultObject (assuming you fixed it with extension NSNumber: UserDefaultsObject {}), but the following line will be problematic:

store = T(userDefaultsObject)

Because the type of T will be a String (we passed a String for defaultValue argument).

Our extension String: UserDefaultsValue will have to include several constructors which take every sort of possible thing that can be stored in NSUserDefaults.  And some of those simply won't make sense.


Processing text files for data extraction and analysis I started learning Haskell to see if I can use it at my job. A lot of my work is processing text files for data extraction and analysis.

For my first test, I added a counter at the end of each line from a .csv text file (currently I don't care about the format management).

My current code in Haskell is:

import qualified Data.ByteString.Lazy.Char8 as L

addRecordId :: String -> String -> Int -> String
addRecordId "" _ _ = ""
addRecordId rec sep cnt = rec ++ sep ++ show cnt

addIncrementalId :: String -> [String] -> [String]
addIncrementalId _ []       = []
addIncrementalId sep ls = addId ls 1
  where
    addId []     _   = []
    addId (l:ls) cnt = addRecordId l sep cnt : addId ls (cnt + 1)

identifyFile :: FilePath -> String -> IO [String]
identifyFile path sep = do
  inpStr <- L.readFile path
  return (addIncrementalId sep (lines (L.unpack inpStr)))

printLnIdentifiedFile :: IO [String] -> IO ()
printLnIdentifiedFile ls = do
  lines <- ls
  putStr (unlines lines)

main = printLnIdentifiedFile (identifyFile "myfile.csv" ";")

This code processes a file of 1GB (4,845,000 records) in 90 seconds.

This C code below does the same job in 10 seconds:

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

int main() {
  FILE *f = fopen("myfile.csv", "r");

  size_t bytes_read;
  size_t current_buffer_size = 400;
  char *buffer = calloc(current_buffer_size, 1);

  long cnt = 1;
  while ((bytes_read = getline(&buffer, &current_buffer_size, f)) > 0) {
    if (feof(f)) break;

    buffer[ bytes_read - 2 ] = 0;
    printf("%s;%ld\n", buffer, cnt++);
  }

  fclose(f);
  return 0;
}

And the Java code below does the job in 30 seconds:

package test.perf.numadr;

import java.io.BufferedReader;
import java.io.FileReader;
import java.io.IOException;

public class NumAdr {

    static public void main(String[] args) {
        BufferedReader br = null;

        try {

            String sCurrentLine;

            br = new BufferedReader(new FileReader("myfile.csv"));

            int cnt = 1;
            String lineWithId;
            while ((sCurrentLine = br.readLine()) != null) {
                lineWithId = sCurrentLine + ";" + cnt;
                cnt++;
                System.out.println(lineWithId);
            }

        } catch (IOException e) {
            e.printStackTrace();
        } finally {
            try {
                if (br != null)br.close();
            } catch (IOException ex) {
                ex.printStackTrace();
            }
        }

    }
}

For each test, I print the result to stdout and redirect to a result file.

My Haskell code is 3 times slower than the Java code and 9 times than the C code.
As I'm a beginner in Haskell, I think my Haskell code is not the best.

How can I improve my program?

I think your program needs to be more Haskell-style (and shorter). Here is my rewrite:

import qualified Data.ByteString.Lazy.Char8 as L

processContents :: L.ByteString -> L.ByteString
processContents contents = L.unlines out
    where out = zipWith f [1..] (L.lines contents)
          sep = L.pack ";"
          f n l = l `L.append` sep `L.append` L.pack (show n)

main = do
    contents <- L.readFile "myfile.csv"
    L.putStr (processContents contents)

Need better speed? It is trivial to convert this code into parallel one.

import qualified Data.ByteString.Lazy.Char8 as L
import Control.Parallel.Strategies
import GHC.Conc(numCapabilities)

processContents :: L.ByteString -> L.ByteString
processContents contents = L.unlines (out `using` parListChunk chunks rdeepseq)
    where out = zipWith f [1..] (L.lines contents)
          sep = L.pack ";"
          f n l = l `L.append` sep `L.append` L.pack (show n)
          chunks = 1 + (length out `div` numCapabilities)

main = do
    contents <- L.readFile "myfile.csv"
    L.putStr (processContents contents)

To compile it use the following command:

ghc -O2 -threaded -with-rtsopts=-N program.hs


Finding sum in an array I have this program finding 2 numbers in the array (unsorted) so that its sum equal to the target sum. Here is my program:
vector<int> findNumbers(vector<int>& nums, int target)
    {
        vector<int> result;
        for(int i = 0; i < nums.size() - 1; i++)
            for(int j = i + 1; j < nums.size(); j++)
                if(nums[i] + nums[j] == target)
                {
                    result.push_back(i);
                    result.push_back(j);
                    return result;
                }
        return result;
    }

But in the last line, the return result doesn't seem to do anything, because when the numbers are found it is already return before it, but if I remove it, it will cause error. Does the last line slow my program down?

Question

Does the last line slow my program down?

This depends on what you intend to return when there are no pairs that sum to target.  If you leave the last line out, your program would invoke undefined behavior when control flow reaches the end of the function body, and the compiler will rightfully warn you about that.
Integer overflow
There is an edge case that your code fails to handle — integer overflow.  When the sum of nums[i] and nums[j] exceeds the range of int, undefined behavior is invoked.
Immediate improvements
Here are some improvements to your code:

nums should taken by const reference, since it is not modified inside the function.

A vector should be indexed with a std::size_t (or its size_type member type), because nums.size() might exceed the range of int.

When nums is empty, nums.size() - 1 will produce SIZE_MAX, which is typically \$2^{32} - 1\$ or \$2^{64} - 1\$.  This didn't cause a bug in this particular case, but it is definitely a logical mistake that should be fixed by, e.g., special-casing nums.size() < 2.

You don't need to construct an empty result at the beginning — construct it on the fly by using return {nums[i], nums[j]} for example.

Here's the result: (overflow check is omitted for simplicity)
#include <cstddef>
#include <vector>

std::vector<std::size_t>
find_numbers(const std::vector<int>& nums, int target) {
    if (nums.size() < 2) {
        return {};
    }

    for (std::size_t i = 0; i < nums.size() - 1; ++i) {
        for (std::size_t j = i + 1; j < nums.size(); ++j) {
            if (nums[i] + nums[j] == target) {
                return {i, j};
            }
        }
    }

    return {};
}

Generalization
The function can be made more flexible by taking an iterator pair and a functor:
#include <iterator>
#include <utility>

// returns a pair of iterators [it_a, it_b]
// such that it_a <= it_b && pred(*it_a, *it_b)
// or [last, last] if such a pair is not found
template <typename ForwardIterator, typename BinaryPredicate>
auto find_pair(
    ForwardIterator first,
    ForwardIterator last,
    BinaryPredicate pred
) -> std::pair<ForwardIterator, ForwardIterator> {
    if (first == last) {
        return {last, last};
    }

    for (auto next = std::next(first); next != last; ++first, ++next) {
        for (auto it = next; it != last; ++it) {
            if (pred(*first, *it)) {
                return {first, it};
            }
        }
    }

    return {last, last};
}

Optimization
The original algorithm runs in \$O(n^2)\$ time, which can be improved to \$O(n \log n)\$ for large inputs:

First, maintain a vector of indexes sorted by the numbers.

For each number, binary search for target - number.


Finding combinations of 3 numbers What this code tries to do is find possible combinations of three numbers.  For example, let's say I have the following numbers:

1, 2, 3, 4, 5, 6, 7

Some possible combinations of three numbers would be:

1,2,3
1,2,4
2,3,4
2,3,5
2,3,6

I do get the desired outcome but my code looks ugly.  Are there any remarks or alternate ways to do this?

<?php

    $numbers = [2, 8, 16, 30, 44, 48];
    $number  = $numbers;
    $slice   = [];
    $threes  = [];
    $x       = 1;
    $stake   = 10;
    $odds    = 325;

    echo implode(" | ", $numbers)."
";

    foreach ($numbers as $key) {

        if (count($number) == 6) {
            // $slice = array_slice($number, 0,3);

            $threes[] = $key;
            $threes[] .= next($number);
            $threes[] .= next($number);

            $x++;

            array_pop($threes);
            $threes[] .= next($number);

            $x++;

            array_pop($threes);
            $threes[] .= next($number);

            $x++;

            array_pop($threes);
            $threes[] .= next($number);

            $x++;

            //--------------------------------------------------------------//

            array_pop($threes);
            array_pop($threes);
            $threes[] .= prev($number);
            array_pop($threes);
            $threes[] .= prev($number);
            array_pop($threes);
            $threes[] .= prev($number);
            $threes[] .= next($number);

            $x++;

            array_pop($threes);
            $threes[] .= next($number);

            $x++;

            array_pop($threes);
            $threes[] .= next($number);

            $x++;

            array_pop($threes);
            array_pop($threes);
            $threes[] = prev($number);
            array_pop($threes);
            $threes[] = prev($number);
            $threes[] = next($number);

            $x++;

            array_pop($threes);
            $threes[] = next($number);

            $x++;

            array_pop($threes);
            array_pop($threes);
            $threes[] = prev($number);
            $threes[] = next($number);

            $x++;

        } elseif (count($number) == 5) {
            $threes[] = $key;
            $threes[] .= next($number);
            $threes[] .= next($number);

            $x++;

            array_pop($threes);
            $threes[] .= next($number);

            $x++;

            array_pop($threes);
            $threes[] .= next($number);

            $x++;

            array_pop($threes);
            array_pop($threes);
            $threes[] .= prev($number);
            array_pop($threes);
            $threes[] .= prev($number);
            $threes[] .= next($number);

            $x++;

            array_pop($threes);
            $threes[] .= next($number);

            $x++;

            array_pop($threes);
            array_pop($threes);
            $threes[] .= prev($number);
            $threes[] .= next($number);

            $x++;
        } elseif (count($number) == 4) {
            $threes[] = $key;
            $threes[] .= next($number);
            $threes[] .= next($number);

            $x++;

            array_pop($threes);
            $threes[] .= next($number);

            $x++;

            array_pop($threes);
            array_pop($threes);
            $threes[] .= prev($number);
            $threes[] .= next($number);

            $x++;
        } elseif (count($number) == 3) {
            $threes[] = $key;
            $threes[] .= next($number);
            $threes[] .= next($number);

        }

        array_shift($number);
        $threes = [];
    }

    echo "
---------------------------------------------------------------------------
";
    echo "Total combinations of 3's: ".$x."
";

    ?>

Your current code
After a long investigation of the problem, I ended up solving more than what you are asking for. I totally misunderstood what your code were actually doing. I found out that your $numbers array is not interesting at all.
The output of your code, the variable $x is the Binomial coefficient for 6 choose 3. (Which is quite funny because in your question you asked about 7 numbers, but in your code you are using 6).
Your code is doing a whole lot of array_pop, next and prev which is just ugly. You need to spend more time thinking about what the algorithm is behind all of this.
Additionally, I don't know what you are intending to do with these lines:
$threes[] .= next($number);

Why use .= ? Last time I checked, .= is for string concatenation. What does that have to do with anything? And why do that when you add a new index to the array? That code does exactly the same as $threes[] = next($number);
You seem to add a whole lot of stuff to the $threes array, but you never end up using it.
Additionally, your $stake, $odds and even $slice seem totally irrelevant here.

Shortening your code
When removing everything related to $threes, and simplifying your multiple $x++ to one $x += ...; your code can be dramatically shortened:
<?php

    $numbers = [2, 8, 16, 30, 44, 48];
    $number  = $numbers;
    $x       = 1;
    echo implode(" | ", $numbers)."
";

    foreach ($numbers as $key) {

        if (count($number) == 6) {
            $x += 10;
        } elseif (count($number) == 5) {
            $x += 6;
        } elseif (count($number) == 4) {
            $x += 3;
        } elseif (count($number) == 3) {
        }
        array_shift($number);
    }

    echo "Total combinations of 3's: ".$x."
";

?>

Then, when removing some more fluff and using an integer variable instead of that $numbers array.
<?php

    $number = 4;
    $x       = 1;
    for ($i = $number; $i >= 0; $i--) {
        if ($i == 6) {
            $x += 10;
        } elseif ($i == 5) {
            $x += 6;
        } elseif ($i == 4) {
            $x += 3;
        }
    }

    echo "Total combinations of 3's: ".$x."
";

?>

So, your code is not very extensible in it's current form. It can only support 3's and a maximum $number of 6... That's not optimal.

A better approach
This is how I calculate the Binomial coefficient in Java - which is in use in my Minesweeper Flags Extreme project, translated into PHP
function nCr($n, $r) {
    if (($r > $n) || ($r < 0)) {
        return 0;
    }
    if (($r == 0) || ($r == $n)) {
        return 1;
    }

    $value = 1;

    for ($i = 0; $i < $r; $i++) {
        $value = $value * ($n - $i) / ($r - $i);
    }

    return $value;
}

echo nCr(8, 4); // Prints 70
echo nCr(6, 3); // Your code example, prints 20

It is very tempting to write $value *= ..., but avoid doing that as the results will not be the same in languages which separates integer division and floating division (not that PHP is one of those languages, but I would still recommend avoiding it).


Count every word occurrence from file How good is this algorithm? Personally I think it's over-complicated and needs some improvement. Should I be using iterators here (I think it's "harder" this way) or indexing? Should I stick with a vector or try other containers? Is a vector (array) of pointers generally better than one of objects?

#include <iostream>
#include <fstream>
#include <string>
#include <vector>
struct StringOccurrence //stores word and number of occurrences
{
    std::string m_str;
    unsigned int m_count;
    StringOccurrence(const char* str, unsigned int count) : m_str(str), m_count(count) {};
};

int main()
{
    std::string path;
    std::cout << "enter file name: " << std::endl;
    std::cin >> path;
    std::ifstream in(path);
    if (!in) //check if file path is valid
    {
        std::cerr << "failed to load file!" << std::endl;
        return -1;
    }

    std::vector<std::string>vec;
    std::string lineBuff;
    while (std::getline(in, lineBuff)) // write multiline text to vector of strings
    {
        vec.push_back(lineBuff);
    }

    std::vector<StringOccurrence*> strOc;
    std::string stringBuff;

    for (auto it = vec.begin(); it < vec.end(); it++) //itterate through each line
    {
        for (auto it2 = it->begin(); it2 < it->end();it2++) //itterate through each letter
        {
            if (*it2 != ' ') //keep adding letters to buffer until space (word)
            {
                stringBuff += *it2;
            }
            if (*it2 == ' ' || it2 + 1 == it->end()) //(need fix?)
            {
                if (*(it2 - 1) == ' ') //check for reccurring spaces so they are not counted as words
                {
                    continue;
                }
                for (auto it3 = strOc.begin(); it3 < strOc.end(); it3++)
                {
                    if (stringBuff == (*it3)->m_str) //if word was found increase count
                    {
                        (*it3)->m_count++;
                        goto end; //skip next step (need fix?)
                    }
                }
                strOc.push_back(new StringOccurrence(stringBuff.c_str(), 1)); //if word was not found add it
            end:
                stringBuff.clear(); //empty buffer for next word
            }
        }
    }

    std::ofstream out("test2.out"); //write to file
    for (auto it = strOc.begin();it < strOc.end();it++)
    {
        out << (*it)->m_str << ' ' << (*it)->m_count << std::endl;
    }

    return 0;
}

Excessive Memory Use

The first thing you do is read every line into memory:

while (std::getline(in, lineBuff)) // write multiline text to vector of strings
{
    vec.push_back(lineBuff);
}

Why? Do you need to have every line in memory? No. You're just trying to count the words. What if the file was 100TB worth of "all work and no play makes Jack a dull boy"?

Poor Memory Use

std::vector<StringOccurrence*> strOc;

That should yell antipattern to you. You're not holding pointers to existing objects, this is strOc actually owning that memory. You use new to add to it. I don't see any deletes. So you're leaking memory. Doesn't matter so much in this case, but this is exactly what RAII is for. Prefer:

std::vector<std::unique_ptr<StringOccurrence>> strOc;

Though, regardless, don't use a vector to begin with because...

Data Container Choice

You are holding a vector of StringOccurrences. That means that on each new word, you have to walk through the whole vector doing string comparisons. That is O(N). Everything about this is bad. To start with:

goto end; //skip next step (need fix?)

Yes, this needs a fix. Never use goto. The way to search for things is to use the <algorithm> library, specifically std::find_if:

auto it = std::find_if(strOc.begin(), strOc.end(), [&](StringOccurence* so){
    return so->m_str == stringBuff;
});
if (it != strOc.end()) {
    // success case
}
else {
    // new word case
}

That'll make your code easier to understand by far and drop the goto. But it won't handle the O(N) problem. For that, we just need to use an entirely new data structure. We need to map a word to a number, and we don't care about word ordering. Thus, std::unordered_map. This will have O(1) lookup. And we don't even need to do all that work ourselves!

std::unordered_map<std::string, int> strOc;
// for each word
++strOc[stringBuff]; // this will insert new elements as necessary

Cool.

Read the words directly

You are walking through a line character by character. That is error-prone at best, hard to follow at worst. But there's already support for this in C++. Use a std::stringstream. You could have just done:

while (std::getline(in, lineBuff)) {
    std::string word;
    std::istringstream iss(lineBuff);
    while (iss >> word) {
        ...
    }
}

Then again, operator>> is defined on any istream, not just istringstream. So we can use that on the ifstream directly:

std::string word;
while (in >> word) {
    ...
}

Use the right loops

Don't use auto with iterators - just use a range-based for-expression. It'll save on the typing and add to clarity.

Improved solution

std::unordered_map<std::string, int> wordCounts;
std::string word;
while (in >> word) {
    ++wordCounts[word];
}

std::ofstream out("test2.out");
for (auto const& wc : wordCounts) {
    out << wc.first << ' ' << wc.second << '\n';
}


Managing Contact Collections I have a WPF/MVVM form that contains sections for managing Recipient Contacts, CC Contacts and BCC Contacts.

Each of the three sections has buttons/ICommands for 'Add', 'Clear', 'Previous' and 'Next' that move through the collection of objects.

I'm trying to work out how I can remove the repeated code.

I wire up the commands in my constructor:

        this.AddressBookRecipientCommand = new GenericCommand(AddressBookRecipientPressed);
        this.ClearRecipientCommand = new GenericCommand(ClearRecipientPressed, CanClearRecipient);
        this.NextRecipientCommand = new GenericCommand(NextRecipientPressed, CanPressNextRecipient);
        this.PrevRecipientCommand = new GenericCommand(PrevRecipientPressed, CanPressPrevRecipient);
        this.AddressBookCCCommand = new GenericCommand(AddressBookCCPressed);
        this.ClearCCCommand = new GenericCommand(ClearCCPressed, CanClearCC);
        this.NextCCCommand = new GenericCommand(NextCCPressed, CanPressNextCC);
        this.PrevCCCommand = new GenericCommand(PrevCCPressed, CanPressPrevCC);
        this.AddressBookBCCCommand = new GenericCommand(AddressBookBCCPressed);
        this.ClearBCCCommand = new GenericCommand(ClearBCCPressed, CanClearBCC);
        this.NextBCCCommand = new GenericCommand(NextBCCPressed, CanPressNextBCC);
        this.PrevBCCCommand = new GenericCommand(PrevBCCPressed, CanPressPrevBCC);

Then I have the various command code:

    public ICommand AddressBookRecipientCommand { get; set; }
    public void AddressBookRecipientPressed()
    {
        this.Recipient = this.Details.Recipients.ReplaceAndReturn(this.Recipient, this.Outlook.GetContact());
    }

    public ICommand PrevRecipientCommand { get; set; }
    public bool CanPressPrevRecipient()
    {
        return Details.Recipients.IndexOf(Recipient) != 0;
    }
    public void PrevRecipientPressed()
    {
        int i = Details.Recipients.IndexOf(Recipient);
        Recipient = Details.Recipients[i - 1];
    }

    public ICommand NextRecipientCommand { get; set; }
    public bool CanPressNextRecipient()
    {
        return !Recipient.IsNullOrEmpty() && Details.Recipients.IndexOf(Recipient) != MaxRecipients;
    }
    public void NextRecipientPressed()
    {
        if (Details.Recipients.IndexOf(Recipient) + 1 == Details.Recipients.Count)
        {
            Recipient = new Contact();
            Details.Recipients.Add(Recipient);
        }
        else
        {
            int i = Details.Recipients.IndexOf(Recipient);
            Recipient = Details.Recipients[i + 1];
        }
    }

    public ICommand ClearRecipientCommand { get; set; }
    public bool CanClearRecipient()
    {
        return !Recipient.IsNullOrEmpty();
    }
    public void ClearRecipientPressed()
    {
        int i = this.Details.Recipients.IndexOf(this.Recipient);
        if (i + 1 == this.Details.Recipients.Count)
            this.Recipient.Clear();
        else
        {
            this.Details.Recipients.Remove(this.Recipient);
            this.Recipient = this.Details.Recipients[i];
        }
    }

This code is repeated for CC and BCC.

If I understand correctly what you're doing, I think what you need is a UserControl that encapsulates some AddressBookCommand, ClearCommand, NextCommand and PreviousCommand commands.

Your main view would have 3 instances of that control, one for each field ("To", "CC", "BCC"), and each would have its own data context, some IList<Contact> I'd guess.

Also you're not showing your entire class so it's hard to tell, but since you mention MVVM I believe a lot of this code belongs in a ViewModel class, not in the View's code-behind; the XAML markup uses DataBinding markup extensions to bind visual element properties to properties in the ViewModel - the Window's DataContext property is set to the instance of the ViewModel class.

The reason I mention this, is because ideally in MVVM the View should only be caring for presentation concerns, which clearly Details.Recipients.Add(Recipient); isn't.


Map object types using custom converter I'm trying to design a mapper that will convert one object type to another shown in the diagram below:

The object has structure as follows (it's not a JSON):

{
"type": "DownloadAppComponent",
"name": "Download App",
"contentId": "download-app",
"properties": {
    "iosUrl": "http://apple.com",
    "androidUrl": "http: //google.com",
    "promoText": "Download our app",
    "hidden": false
}

My first solution was to have mapper for each type but this required a lot of code duplication to map common attributes (i.e. name, type, contentId).

public DownloadAppComponent map(CmsDocument cmsDocument) {
    DownloadAppComponent downloadAppComponent = new DownloadAppComponent();

    downloadAppComponent.setType(cmsDocument.getType()); // <-- this will be duplicated in each mapper
    downloadAppComponent.setName(cmsDocument.getName()); // <-- this will be duplicated in each mapper
    downloadAppComponent.setContentId(cmsDocument.getText(CONTENT_ID_PATH)); // <-- this will be duplicated in each mapper
    downloadAppComponent.setIosURL(cmsDocument.getText(IOS_URL_PATH));
    downloadAppComponent.setAndroidURL(cmsDocument.getText(ANDROID_URL_PATH));
    downloadAppComponent.setHidden(Boolean.parseBoolean(cmsDocument.getText(HIDE_PATH)));
    downloadAppComponent.setPromoText(cmsDocument.getText(DOWNLOAD_PROMO_TEXT_PATH));

    return downloadAppComponent;
}

I've been trying to refactor that code and I come up with generic BaseDocumentMapper:

public BaseDocument map(CmsDocument cmsDocument) {
    BaseDocument document = documentsMapperFactory.getMapper(cmsDocument.getType()).map(cmsDocument);
    document.setType(cmsDocument.getType());
    document.setName(cmsDocument.getName());
    document.setContentId(cmsDocument.getText(CONTENT_ID_PATH));
    return document;
}

The documentsMapperFactory returns specific mapper that will map only type related attributes and return that object instance.

However, there are still more levels in inheritance and I just have one type value which I can resolve to concrete mapper. So I'll have to repeat Component's specific fields mapping in each component mapper.

I was thinking that since I know the hierarchy I could create some mappers that will map the documents from Up to Bottom, i.e. create first DownloadAppComponent and then extend it with Component and then with BaseDocument specific properties. However, I have not found any good solution except using abstract classes and inheritance in mappers as well.

Could someone advise me if this is good approach or if there are any issues or other better solutions for my case?

The first thing that surprise me in your code is that your map method are non-static. You would need an instance of your component to create a different instance mapping a document.

I think the method to get Components from a document should be static:

public static DownloadAppComponent of(CmsDocument cmsDocument) {
    return new DownloadAppComponent().map();
}

Now, your map methods can initialize only the attributes specific of the Class and rely on the superclass to initialize the more generics attributes:

// Map method of DownloadAppComponent class
protected DownloadAppComponent map(CmsDocument cmsDocument) {
    super.map(cmsDocument);
    setIosURL(cmsDocument.getText(IOS_URL_PATH));
    setAndroidURL(cmsDocument.getText(ANDROID_URL_PATH));
    setHidden(Boolean.parseBoolean(cmsDocument.getText(HIDE_PATH)));
    setPromoText(cmsDocument.getText(DOWNLOAD_PROMO_TEXT_PATH));
    return this;
}

// Map method of Component class
protected Component map(CmsDocument cmsDocument) {
    super.map(cmsDocument);
    setHidden(Boolean.parseBoolean(cmsDocument.getText(HIDE_PATH)));
    return this;
}

// Map method of BaseDocument class
protected BaseDocument map(CmsDocument cmsDocument) {
    setType(cmsDocument.getType());
    setName(cmsDocument.getName());
    setContentId(cmsDocument.getText(CONTENT_ID_PATH));
    return this;
}

EDIT:

If you want the map functionality in mapper classes, your mapper classes could implement a parallel hierarchy to the Component classes and still use inheritance to only initialize the attribute specific to each mapper:

public class DownloadAppComponentMapper extends ComponentMapper {

    public DownloadAppComponent of(CmsDocument cmsDocument) {
        return map(new DownloadAppComponent(), cmsDocument);
    }

    protected DownloadAppComponent map(DownloadAppComponent downloadAppComponent, CmsDocument cmsDocument) {
        super.map(downloadAppComponent, cmsDocument);
        downloadAppComponent.setIosURL(cmsDocument.getText(IOS_URL_PATH));
        downloadAppComponent.setAndroidURL(cmsDocument.getText(ANDROID_URL_PATH));
        downloadAppComponent.setHidden(Boolean.parseBoolean(cmsDocument.getText(HIDE_PATH)));
        downloadAppComponent.setPromoText(cmsDocument.getText(DOWNLOAD_PROMO_TEXT_PATH));
        return downloadAppComponent;
    }
}

public class ComponentMapper extends BaseDocumentMapper {

    public Component of(CmsDocument cmsDocument) {
        return map(new Component(), cmsDocument);
    }
    protected ComponentMapper map(Component component, CmsDocument cmsDocument) {
        super.map(component, cmsDocument);
        component.setHidden(Boolean.parseBoolean(cmsDocument.getText(HIDE_PATH)));
        return component;
    }
}

public class BaseDocumentMapper {

    public BaseDocument of(CmsDocument cmsDocument) {
        return map(new BaseDocument(), cmsDocument);
    }
    protected BaseDocument map(BaseDocument baseDocument, CmsDocument cmsDocument) {
        baseDocument.setType(cmsDocument.getType());
        baseDocument.setName(cmsDocument.getName());
        baseDocument.setContentId(cmsDocument.getText(CONTENT_ID_PATH));
        return baseDocument;
    }
}


Getting rid of certain HTML tags This code simply returns a small section of HTML code and then gets rid of all tags except for break tags.

It seems inefficient because you cannot search and replace with a beautiful soup object as you can with a Python string, so I was forced to switch it back and forth from a beautiful soup object to a string several times so I could use string functions and beautiful soup functions.  It seems that there must be a simpler way to do this without switching back and forth between soup objects and strings.

def whiteScrape(webaddress):
     url = (webaddress)
     ourUrl = opener.open(url).read()

     soup = BeautifulSoup(ourUrl)
     soup = soup.find("div", { "class" : "addy" })
     #below code will delete tags except /br
     soup = str(soup)
     soup = soup.replace('
' , '^')
     soup = BeautifulSoup(soup)
     soup = (soup.get_text())
     soup = str(soup)
     soup=soup.replace('^' , '
')

     return soup

The original HTML is something like:

 blah blah
 blahblah
 blah

The output should be:

blah blah
 blahblah
 blah

By PEP 8, whiteScrape() should be renamed to white_scrape().

Variable names should be purposeful.  Therefore, it's a bad idea to continually redefine soup.  If someone were to ask you to explain what the variable soup contained, you would have a hard time explaining.

It seems like what you want to do is to stringify the children of div(s) in question.  One way to do that would be:

def white_scrape(url):
    page = opener.open(url).read()
    soup = BeautifulSoup(page)
    addr = soup.find('div', { 'class': 'addy' })
    return ''.join(str(child) for child in addr.children)

Note that this is not exactly equivalent to your original code.  For example, if an address contains an element, such as 123 1st St., this solution would preserve the  tag and its contents, whereas your original code would discard the tag but keep its contents and strip the tag.


Read from SQL database table, store in list of objects IList<WebSite> wsl = new List<WebSite>();
 login1 l = new login1();

 string q = "select Id, DomainUrl, IsApproved, Date from website where UserId = @UserId";

 using (MySqlConnection con = new MySqlConnection(WebConfigurationManager.ConnectionStrings["MySqlConnectionString"].ToString()))
 {
      using (MySqlCommand cmd = new MySqlCommand(q, con))
      {
           cmd.Parameters.Add("@UserId", MySqlDbType.Int32).Value = userId;
           con.Open();

           var reader = cmd.ExecuteReader();
           while (reader.Read())
           {
                var ws = new WebSite();
                ws.Id = reader.GetInt32("Id");
                ws.DomainUrl = reader.GetString("DomainUrl");
                ws.IsApproved = reader.GetBoolean("IsApproved");
                ws.User.Id = reader.GetInt32("UserId");
                ws.Date = reader.GetDateTime("Date");
                wsl.Add(ws);
           }
           reader.Close();
           return wsl;
      }
 }

I want to make this code as elegant as possible.

For this particular project I cannot use NHibernate as I have been instructed.

This code is very tightly coupled with the specific database engine. That makes it hard to change if you decide to go with different database engine. And it's not possible to unit test it at all.

Instead of using the specific MySql classes for connections, etc., change it to use the base interfaces, and then pass the connections and the queries from outside.

Also, I generally avoid using "return" inside a code block like "using" and if/switch, if it does not make the code too clunky. That way the code path is much easier to follow.

Bellow is an example of this particular method as part of some class, which can be reused in many different situations, incl web pages, services, etc. and with different databases, w/o changing the implementation of this class itself.

private IDatabaseProvider _dbProvider; //initialized in ctor, etc.
private IConnectionStringProvider _connectionStringProvider;
private IWebSiteMapper _mapper; //the db schema may change, or you may need to map from other source

public IList<WebSite> GetWebSitesForUser(int userId)
{
    var wsl = new List<WebSite>();
    var con = _dbProvider.GetNewConnection(); //returns IDbConnection
    var cmd = _dbProvider.GetWebSiteListQueryCommand(con, userId); //IDbCommand, and addParam goes there

    conn.Open(); // no need of try/catch or using, as if this fails, no resources are leaked
    using (var reader = cmd.ExecuteReader(CommandBehavior.CloseConnection)) //close con on reader.Close
    {
         while (reader.Read())
         {
             wsl.Add(_mapper.CreateFromReader(reader));
         }
    }

    return wsl;
}


Android RecyclerView Adapter: Passing normal clicks and menu clicks over the same activity I am unsure about my approach here. As you can see, I want to send normal clicks and 2 different menu item clicks back to the underlying activity over an interface. Now I wonder if it is bad practice to send it over the same interface. I couldn't find an answer online. Also, any suggestions about what to change here?

public class ImageAdapter extends RecyclerView.Adapter<ImageAdapter.ImageViewHolder> {
private Context mContext;
private List<Upload> mUploads;
private OnItemClickListener mListener;

public ImageAdapter(Context context, List<Upload> uploads) {
    mContext = context;
    mUploads = uploads;
}

@Override
public ImageViewHolder onCreateViewHolder(ViewGroup parent, int viewType) {
    View v = LayoutInflater.from(mContext).inflate(R.layout.image_item, parent, false);
    return new ImageViewHolder(v);
}

@Override
public void onBindViewHolder(ImageViewHolder holder, int position) {
    Upload uploadCurrent = mUploads.get(position);
    holder.textViewName.setText(uploadCurrent.getName());
    Picasso.with(mContext)
            .load(uploadCurrent.getImageUrl())
            .placeholder(R.mipmap.ic_launcher)
            .fit()
            .centerCrop()
            .into(holder.imageView);
}

@Override
public int getItemCount() {
    return mUploads.size();
}

public class ImageViewHolder extends RecyclerView.ViewHolder implements View.OnClickListener,
        View.OnCreateContextMenuListener, MenuItem.OnMenuItemClickListener {
    public TextView textViewName;
    public ImageView imageView;

    public ImageViewHolder(View itemView) {
        super(itemView);

        textViewName = itemView.findViewById(R.id.text_view_name);
        imageView = itemView.findViewById(R.id.image_view_upload);

        itemView.setOnClickListener(this);
        itemView.setOnCreateContextMenuListener(this);
    }

    @Override
    public void onClick(View v) {
        if (mListener != null) {
            int position = getAdapterPosition();
            if (position != RecyclerView.NO_POSITION) {
                mListener.onItemClick(position);
            }
        }
    }

    @Override
    public void onCreateContextMenu(ContextMenu menu, View v, ContextMenu.ContextMenuInfo menuInfo) {
        menu.setHeaderTitle("Select Action");
        MenuItem doWhatever = menu.add(Menu.NONE, 1, 1, "Do whatever");
        MenuItem delete = menu.add(Menu.NONE, 2, 2, "Delete");

        doWhatever.setOnMenuItemClickListener(this);
        delete.setOnMenuItemClickListener(this);
    }

    @Override
    public boolean onMenuItemClick(MenuItem item) {
        if (mListener != null) {
            int position = getAdapterPosition();
            if (position != RecyclerView.NO_POSITION) {

                switch (item.getItemId()) {
                    case 1:
                        mListener.onWhatEverClick(position);
                        return true;
                    case 2:
                        mListener.onDeleteClick(position);
                        return true;
                }
            }
        }
        return false;
    }
}

public interface OnItemClickListener {
    void onItemClick(int position);

    void onWhatEverClick(int position);

    void onDeleteClick(int position);
}

public void setOnItemClickListener(OnItemClickListener listener) {
    mListener = listener;
}
}

It's quite hard to read that code. I think some part is missing as I can't figure out what's going on.

However when you look at SOLID principles, you'll find that the public interface OnItemClickListener you created probably violates the I — The Interface Segregation Principle (ISP), exactly as TextWatcher does.

For more on this, see also Donn Felker's article I is for the Interface Segregation Principle.


Simple JavaScript (sub) class [properties, getters, methods] The more I consider object oriented JavaScript the more I am confused. There are so many different ways and concepts and I simply do not know any longer what fits best for my purposes.

I like the constructor pattern but I have the feeling that I've mixed different concepts:

Subclass.prototype = Object.create(Superclass.prototype);

function Subclass(name, value) {
    Superclass.call(this);

    this._name = name;
    this._value = value;

    this._init();
}

Object.defineProperties(Subclass.prototype, {
    'name': {
        get: function() { return this._name; }
    },
    'value': {
        get: function() { return this._value; },
        set: function(value) { this._value = value; }
    }
})

Subclass.prototype._init = function() {
    // some initialization
};

Subclass.prototype.compute = function() {
    var name = this.getName();
    var value = this.getName();
    // do something with name and value...
};

Is there a cleaner way?
Should one use getters and setters for "internal" access (like I would do it in other languages)?

Edit #1

Define properties of prototype (Object.defineProperties(Subclass, {} to Object.defineProperties(Subclass.prototype, {})

These are the general rules I follow:

If using actual private variables in my class, create get/set properties for those I want accessible to the outside world:

function Point(x, y) {
    var _x = x || 0,
        _y = y || 0,
        _foo = "bar";

    Object.defineProperty(this, "x", {
        enumerable: true,
        get: function() {
            return _x;
        },
        set: function(value) {
            _x = value;
        }
    });

    Object.defineProperty(this, "y", {
        enumerable: true,
        get: function() {
            return _y;
        },
        set: function(value) {
            _y = value;
        }
    });
}

var p = new Point(2, 10);
p.x; // is 2
p.y; // is 10;
p.foo; // is undefined
p._foo; // is undefined

If private variables aren't all that beneficial, or the property is meant to be public, I'll just declare that as part of the prototype:

function Point(x, y) {
    this.x = x || 0;
    this.y = y || 0;
}

Point.prototype = {
    x: 0,
    y: 0,

    constructor: Point
};

Or if I want something to be read only, I'll use a combo of private variables and public getters (Example: Viewport on GitHub):

function Foo(bar) {
    var _bar = bar || "baz";

    Object.defineProperty("bar", {
        enumerable: true,
        get: function() { return _bar; }
    });
}

var foo = new Foo("foo");
foo.bar = "abc";
alert(foo.bar); // alerts "foo"

I tend to gravitate towards making everything public, and part of the prototype. As a personal convention properties and methods prefixed with an underscore are not meant to be called publically. Some people force encapsulation by using private variables, but this always feels like I'm trying to fit a square peg into a round hole. Unless you jump through hoops, everything in JavaScript is public. Embrace it. It makes unit testing your JavaScript code easier as well.

As an added benefit, most browsers have optimized the Constructor Function + Prototype code by generated classes behind the scenes for your JavaScript "classes", making property access in JavaScript almost as efficient as native code. Browsers haven't done much to optimize Constructor Functions that use private variables --- which is not to say that kind of code won't ever be optimized. It just isn't optimized very much right now.

Edit #1:

From OP's comment below:

  The point I've got at is that adding a property to each object might result in "larger" objects. If the property is defined in the object's prototype (same for all) it is only defined once.

This is not necessarily the case. Consider this code:

Using Object.defineProperty

function Point(x, y) {
    this._x = x;
    this._y = y;
}

Object.defineProperty(Point.prototype, "x", {
    get: function() {
        return this._x;
    },
    set: function(value) {
        this._x = value;
    }
});

Object.defineProperty(Point.prototype, "y", {
    get: function() {
        return this._y;
    },
    set: function(value) {
        this._y = value;
    }
});

var p = new Point(3, 0);

There is absolutely no difference between p.x = 5; and p._x = 5;. The x and y properties created using Object.defineProperty are superfluous, and only serve to add weight to your JavaScript code. There is no logic behind the getting and setting of those values.

On the contrary, consider a case where you do want some special logic around setting a value. In the code below, the element property is a reference to a DOM node. When setting the DOM node we also want to set properties for the document and window to which the DOM node belongs:

function Foo(element) {
    this.element = element;
}

Foo.prototype._element = null;
Foo.prototype.document = null;
Foo.prototype.window = null;

Object.defineProperty(Foo.prototype, "element", {
    get: function() {
        return this._element;
    },
    set: function(value) {
        this._element = value;
        this.document = value.ownerDocument;
        this.window = this.document.defaultView;
    }
};

Now we perform special logic in the set function for element. Defining the element property via Object.defineProperty is now beneficial.

You might find Inheritance and the prototype chain a worthwhile read, as well as Introduction to Object-Oriented JavaScript.

Edit #2:

Defining a property using Object.defineProperty is useful for creating delegate properties. I'm going to build on the Foo class from my previous edit.

The element belongs to a document and a window. In the previous example, there is nothing preventing you from running this code:

var foo = new Foo(document.body);
foo.document = someOtherDocumentObjectFromAnIFRAME;

This breaks the relationship between element and document because we assume foo.document is the document object that contains foo.element. Instead, we want to create delegate properties for document and window so we can maintain that relationship:

function Foo(element) {
    this.element = element;
}

Foo.prototype.element = null;

Object.defineProperty(Foo.prototype, "document", {
    get: function() {
        return this.element.ownerDocument;
    }
});

Object.defineProperty(Foo.prototype, "window", {
    get: function() {
        return this.element.ownerDocument.defaultView;
    }
});

Notice in this variation that the element property is now defined as part of the prototype since it has no special logic around getting or setting its value.

Now the document and window properties are read-only, maintaining the relationship that document is the Document object that contains element, and window is the Window object that contains both document and element.


Lock-free MultiConsumer/MultiProducer queue I've written a simple lock-free multi-consumer/-producer FIFO queue.
It's supposed to work like this:

At any time many consumers may read from it
At any time multiple producers can write to it, added elements will
be made available later
After write modification is finished, we call makePushedElementsAvailable(), so that consumers can read the newly added elements

It's meant to be used for several worker threads to communicate with the GUI simultaneously. I wouldn't necessarily need multi-consumers, single would suffice, but I figured it would be relatively easy to have multi-consumers.

#pragma once
#include <list>
#include <atomic>

template<typename T, int Size>
class MCMP
{
public:
    MCMP();
    bool tryPushLater(const T &element);
    bool tryPop(T &element);
    void makePushedElementsAvailable();
    bool wasEmpty();
    int size();
protected:
private:
    T elements[Size];
    std::atomic<int> iHead, iTail, iWrite;
};

template<typename T, int Size>
int MCMP<T, Size>::size()
{
    return (iTail.load() - iHead.load());
}

template<typename T, int Size>
bool MCMP<T, Size>::wasEmpty()
{
    return (iHead.load() == iTail.load());
}

template<typename T, int Size>
MCMP<T, Size>::MCMP() : iHead(0), iTail(0), iWrite(0)
{
}

template<typename T, int Size>
void MCMP<T, Size>::makePushedElementsAvailable()
{
    iTail.store(iWrite.load());
}

template<typename T, int Size>
bool MCMP<T, Size>::tryPop(T &element)
{
    int newIndex;
    int index;
    do {
        index = iHead.load();
        if (index == iTail.load())
            return false;
        newIndex = index + 1;
    } while (!iHead.compare_exchange_weak(index, newIndex));

    index = index % Size;
    element = elements[index];
    return true;
}

template<typename T, int Size>
bool MCMP<T, Size>::tryPushLater(const T &element)
{
    int newIndex;
    int index;
    do {
        index = iWrite.load();
        if (index - iHead.load() >= Size)
            return false;
        newIndex = index + 1;
    } while (!iWrite.compare_exchange_weak(index, newIndex));

    index = index % Size;
    elements[index] = element;
    return true;
}

So far this seems to work fine. I'd like to have it checked and validated by some others though, please. Is there a simpler way than making the elements available after all updating has finished?

Hello and welcome to Code Review.

Code comments

Consider writing what you expect each function to do at its most important steps. Short concise comments are preferred (I am guilty of not doing this, too!). This would help reviewers that look at your code and yourself when debugging.

Class names

Your queue's name is MCMP, which I assume stands for multi-consumer/multi-producer. It is an unclear class name. Consider changing it to something more descriptive such as: lock_free_queue.

template <typename T, int Size>
class lock_free_queue
{
    /* declarations */
}

Redundancy in iHead, iTail, iWrite

One of those variables is redundant, your queue should support pushing from one end and popping from the other concurrently, so you only require iHead and iTail. The interaction between these three variables is confusing. Can you explain how they are supposed to work together?

They don't do what you think they do

size() and wasEmpty() are obviously supposed to return the size of the queue and if it is empty, respectively. However, you are not doing an atomic operation to evaluate their expressions. Therefore, nothing prevents another thread from modifying one of either atomic variable after it is loaded. For example:

template<typename T, int Size>
bool MCMP<T, Size>::wasEmpty()
{
    return ( iHead.load() == iTail.load() );
}

What happens if a value is loaded from iHead.load(), but then another thread modifies iHead a bunch of times before iTail loads? It will be compared with an outdated value of iHead and could make your queue say that it is not empty, but it is actually empty in reality.

Compare/exchange loops

You seem to misunderstand how compare/exchange loops work. Every time the compare/exchange operation fails, the first argument of the compare/exchange function (expected) gets updated with the newest value of A.

So in your case, there is no need to load from A every single time in the loop. Your compare/exchange loops thus become:

int newIndex;
int index = iHead.load(); // the initial load from the atomic variable
do
{
    if ( index == iTail.load() )
        return false;
    newIndex = index + 1;
} // 'index' gets updated when the loop fails
while ( !iHead.compare_exchange_weak( index, newIndex ) );

The ABA problem

tryPop( T& ) and tryPushLater( const T& )

Because concurrent code can execute with undetermined timings, your do{} while{} loops suffer from the ABA problem; newIndex gets updated in the loop when the condition fails, but it is possible for one thread to stall and for another to come in and change the value of index from A to B then back to A. In that case, newIndex would not be updated, as the condition would succeed when the original (stalled) thread resumes.

How could it fail if iHead is always incremented?

Thread A can be paused for an indeterminate amount of time. While A is paused, a number of threads concurrently push/pop elements causing your index to overflow and reach the value that A has; A resumes and finds that the comparison is equal and doesn't update newIndex. You have the ABA problem.

While this scenario is very unlikely, it is still possible. Therefore, the code is not error-free. Your queue could not be used as part of a long running application because that issue might eventually arise causing the application's state to become corrupted.

If you require further explanation of this issue, tell me so in a comment.

The ABA problem - Extended

For an atomic compare/exchange operation:

Let T1, T2 be threads.
Let A be the atomic variable.
Let E be the expected value of A.
Let N be the new value of A on success.

In any compare/exchange based algorithm that loads the value of A and performs another operation based on that value, such as:

Updating N if the compare/exchange operation fails (A.load() != E) with the result of a calculation dependent on the most up-to-date value of A.
Dereferencing the value loaded from A if it is a pointer.
Performing a lookup based on the value loaded from A.
etc.

The ABA problem occurs when thread execution timings cause T1 to stall before the compare/exchange operation (that is dependent on the value loaded from A) is evaluated.

While T1 is stalled, T2 changes the value of A to some other value and then back to the original value of A before T1 resumes. Once T1 continues, the condition of the compare/exchange operation will thus evaluate to true when it should in fact be false.

In the case of the first example of the previous bullet list, N would be stored in A while N has an invalid value/state; had the compare/exchange operation failed (as it should have) N would have been updated according to the new value of A before the next compare/exchange evaluation.

Other bugs

When you pop an element from your queue, you don't actually remove the element, you just return a reference to it.
The values get written to the array whether makePushedDataAvailable() is called or not, so other threads will see those values. Is this expected?

Conclusion

I've highlighted the most obvious bugs in your program and a few design issues. The ABA problem being the most serious one.


Filter by a distinguishing field when making publications using meteor In my publications with meteor.js, I find myself repeating the same style of code and would like a cleaner approach. For example:

I have a users collection that has the distinguishing field school, an identifier that tells me by which school, I should filter what information on the server so that I don't send the wrong school the wrong information. Currently, I

Meteor.publish('schoolDirectory', function (schoolId) {
        return Meteor.users.find({'profile.school': schoolId}});
});

Where I take in the schoolId on the client, and filter appropriately. I am not sure of the security implications there and have tried switching to a style where I use the authenticated users object to determine the school on the server before publishing any more data like so:

Meteor.publish('schoolDirectory', function () {
             var currentUser = Meteor.users.findOne({_id: this.userId});
             var schoolId = Schools.findOne({_id: currentUser.profile.school});
            return Meteor.users.find({'profile.school': schoolId}});
    });

This has the issue of not being able to publish data for anonymous users, anyone have any suggestions on how to improve the code base to use this distinguishing field globally ?

I can't see how you can filter field B (school) that relies on field A (user) when there's no information about field A.

What is the purpose of showing schools to anonymous users?

If we take the scenario of displaying information to anonymous users without any related functionality (read-only stuff), then perhaps you should have another publication which only publishes 'public' properties of the school entities.


Find all combinations of a number sequence which is first increasing then decreasing Given an integer N(Natural Number), A program/Algorithm to find the
  remainder of arrangements that can be obtained by rearranging the
  numbers 1, 2, ...., N.

  Input Format: One line containing the integer N

  Output Format: An integer m, giving the remainder of the number of
  arrangements that could be obtained from 1, 2, ...., N is divide
  by Mod

  Constraints:
  Mod = 10^9+7
  N ≤ 10^9

  Example 1 Input
  3  Output
  2  Explanation: Consider the first three natural numbers 1, 2, 3. These can be arranged in the following ways: 2, 3, 1 and 1,
  3, 2. In both of these arrangements, the numbers increase to a certain
  point and then decrease. There are two such arrangements: 2, 3, 1 and
  1, 3, 2.

  Example 2

  Input
  4  Output
  6  Explanation: The six arrangements are (1, 2, 4, 3), (1,3,4,2), (1,4,3,2), (2,3,4,1), (2,4,3,1), (3,4,2,1).

#include<stdio.h>
#include<stdlib.h>
#define m 1000000007

unsigned long long int power(unsigned long long int x, unsigned long long int n){

    unsigned long long int res = 1;
    while(n > 0){
        if(n & 1){
            res = res * x;
            res = res%m;
        }
        x = x * x;
        x= x%m;
        n >>= 1;
    }
    return res;

}

int main(){

    unsigned long long int n,res=0,temp=1,i;
    scanf("%llu", &n);
    if(n==1 || n==0){
        printf("0\n");
        return 0;
    }
    temp = power(2, n-1);
    temp--;
    temp--;
    printf("%llu\n", temp);

    return 0;
}

Can Anyone Solve This with Better Time Complexity?

Bug

Input : 500000003
Output: 18446744073709551615

The problem is that the result of your power() call could be 0 or 1.  Then when you subtract 2, you get a negative number.  Given the input constaint of \$n ≤ 10^9\$, 500000003 is the only bad input, but in general this happens when n is any multiple of 500000003.  To correct this, you could change this:

temp--;
temp--;

to:

temp = (temp + m - 2) % m;

This ensures that temp will remain positive.  Time complexity wise, your program is great at \$O(\log n)\$.


"Heads or Tails?" guessing game in Python I'm pretty new to coding and I want to get an opinion on my coding. I am learning Python and I made a really simple heads and tails guessing game.

import random
import time
import easygui
import sys
while True:
    rand = random.choice(["Heads", "Tails"])
    firstguess = raw_input("Guess Heads or Tails: ")
    if firstguess == rand:
        print "Wow you win"
    else:
        print "That is wrong you suck so bad lol."
    time.sleep(2)
    answer = easygui.buttonbox("Play again?", choices=["Yes","No"])
    if answer == "Yes":
        easygui.msgbox("Ok")
    else:
        break

easygui.msgbox("Ok, see you later!")
sys.exit(0)

Why are you mixing raw_input() with easygui?  That's a suboptimal user experience.

sys.exit(0) at the end is superfluous, but harmless.


Finding an entry in a Hashmap I've been playing around with functional programming for a while now and just started learning Clojure. So the problem I'm trying to solve is the following:

I have a tree-like hashmap of tasks where each task can include a number of tasks and each of those can include other tasks. The toy one I'm using for starters looks like this:

(def master-list
  {:name "Master List"
   :details "One List to Rule them all"
   :list
   [{:name "Writing"
     :details "Writing things is fun"
     :list
     [{:name "Write A Best-Seller"
       :details "Making money is also fun"
       :list
       [{:name "Come up with the plot"
         :details "What do I want to talk about?"}]}
      {:name "Write for the New York Times"
       :details "People need to hear this"}]}
    {:name "Reading"
     :details "I have a long list of books to read"
     :list
     [{:name "The Catcher in the Rye"
       :details "Who is Holden Caulfield and Why should I care?"}]}
    {:name "Profit"
     :details "All Done"}]})

Where :list contain the subtask list of a given task. Ok, cool, so far so good.

But in order to work with a given task I need to be able to get to it within that hashmap. And because that hashmap sits within an atom and to keep every representation of the tasks consistent I can't just copy it into a new map. What I figured is that I'd keep a vector of keys that can be used to access each item individually with the function get-in.

For example to refer to the task named "Come up with the plot" I can just store the vector [:list 0 :list 0 :list 0]. Since

(get-in master-list [:list 0 :list 0 :list 0])

Will return me the task I need. Cool.

So now I can access a task within the hashmap and if that changes the changes will be reflected everywhere. Now I need to have a way to find that vector given any of my tasks. Something that will always pass the test (called find-in in the code below):

(= list-i-have (get-in master-list (helper/find-in master-list list-i-have)))
;;Should always be true

After a while, I came up with the code:

(defn- inner-func [index item]
  (cond
   (= item '()) nil
   (seq? item) (conj item index)
   (== item true) (conj '() index)
   true nil))

(defn- is-in [task subtask]
  (cond
   (= task subtask) true
   (:list task)  (flatten (keep-indexed inner-func (keep #(is-in % subtask) (:list task))))
   true  nil))

(defn find-in [task subtask]
  (let [result (is-in task subtask)]
    (cond
     (== result true)  []
     (= result '())   nil
     true (interleave (cycle [:list]) result))))

That seems to solve my problem which brings me to my (many) questions:

What do you think about the design choices?
Have I missed any other ways to represent the list and the selected part of the list? Can you find any major flaws with my choice?
What about the code itself? Is it functional? As I was writing it, I felt like I kept falling back to imperative-like solutions.
Any other problems with it I might have missed?

I think your choice of using a nested map/vector structure is a good one, and I think it is pretty idiomatic to Clojure. It can be tricky to work with nested structures like this, but it's certainly doable.

Your find-in function (and the private functions it depend on) is an imperative kind of solution -- you're iterating through the maps/lists and building up a "key vector" from scratch. I'm not sure if there is a strictly non-imperative way to navigate tree structures like this, but there is at least a much simpler way. You should have a look at clojure.zip. It provides zippers, which are data structures that represent a location in a tree-like data structure, and they keep track of the path that it took to get there.

With zippers, the approach to representing the "location of" (or "path to") an item is a bit different. To use your example:

(get-in master-list [:list 0 :list 0 :list 0])

This returns the node {:name "Come up with the plot" :details "What do I want to talk about?"}. In zipper terminology, this is "down down down" -- if you visualize the tree in your mind, with the master list as the root, you go down and end up at the first item in the list ("Writing"), then down again to the first item of that list ("Write A Best-Seller"), and then down a third time to the first item of that list, which is "Come up with the plot." Here's how you would code that:

(require '[clojure.zip :as z])

; (def master-list
;   ...)

(def zipper-list (z/zipper map? :list #(assoc %1 :list %2) master-list))

(-> zipper-list z/down z/down z/down z/node)

Zippers can be confusing at first, so I'll explain how this one works. The function clojure.zip/zipper takes 4 arguments:

A function that, given a node in the tree, returns true if that node can have children, even if it currently doesn't. I used map? because each node we're working with is a map that can potentially have a :list field, containing children.
A function that, given a branch node, returns a seq of its children. In this case, we can use :list as a function to get a given node's children (in Clojure, a keyword can be used as a function to look itself up in a map).
A function that takes two arguments -- (1) an existing node and (2) a seq of children -- and returns a new branch node that has the supplied children. In our case, the function would be #(assoc %1 :list %2).
The actual structure that we're navigating; in this case, it's master-list.

Now you can pass the zipper through any of clojure.zip's handy methods for navigating zippers. Once you've reached the node you want, you can use the clojure.zip/node method to return the actual data in the node.

I'm not sure what your use case is, but it sounds like you might want to have a way of representing an "address of" or "path to" a certain node. Working with zippers, you could actually represent a node's location as a list of the zipper functions necessary to reach it, something like this:

(def come-up-with-a-plot [z/down z/down z/down])

And then if you wanted to, you could write a helper function that inserts the zipper and the directions into a form just like we used above, sandwiched between -> and z/node:

(defn get-node [zipper directions]
  (eval `(-> ~zipper ~@directions z/node)))

(get-node zipper-list [z/down z/down z/down])

As an alternative, you could use a nested map structure that has only maps, no vectors, and use the names of each node as their keys:

(def master-list
   {:details "One List to Rule them all"
    "Writing"
      {:details "Writing things is fun"
      "Write A Best-Seller"
        {:details "Making money is also fun"
         "Come up with the plot"
           {:details "What do I want to talk about?"}}
      "Write for the New York Times"
      {:details "People need to hear this"}}
    "Reading"
      {:details "I have a long list of books to read"
       "The Catcher in the Rye"
         {:details "Who is Holden Caulfield and Why should I care?"}}
    "Profit"
      {:details "All Done"}})

This way, you would access each node using the exact names of each node in the tree in string form, like this:

(get-in master-list ["Writing" "Write A Best-Seller" "Come up with the plot"])

The obvious downside of this method is that it's vulnerable to accidental variation in the string keys. You might accidentally type, say, "Come up with a plot," and get nil as a result because the map doesn't have that key. Another downside is that maps aren't ordered or sorted, so any items that you add in (say by using assoc) are not stored in any particular order; although, you could remedy this by using ordered lists.

If you want my 2 cents, I think zippers are the best way to go for something like this. :)


Poker Hands in Python Project Euler problem 54 asks:

The file, poker.txt, contains one-thousand random hands dealt to two players. Each line of the file contains ten cards (separated by a single space): the first five are Player 1's cards and the last five are Player 2's cards. You can assume that all hands are valid (no invalid characters or repeated cards), each player's hand is in no specific order, and in each hand there is a clear winner.
How many hands does Player 1 win?

I wrote this program rather hastily, but I also tried to make use of Python features. Here are my questions:

Are the functions clear and does eval_hand return too much (or too little) info?
Would this benefit from rewriting with OOP?
Is the code easy to understand, and how can the code be shortened or clarified? Performance is not a concern.

value_dict = {'T': 10, 'J': 11, 'Q': 12, 'K': 13, 'A': 14}

def eval_hand(hand):
    # Return ranking: high card = 0, ... royal flush = 9
    # Also return high card(s) of rank

    values = sorted([c[0] for c in hand])
    suits = [c[1] for c in hand]
    straight = (values == range(min(values), max(values)+1))
    flush = all(s == suits[0] for s in suits)

    # Should not occur (too rare)
    if straight and flush:
        if values[0] == 10:
            return 9, None
        else: return 8, max(values)

    pairs = []
    pair_present = False
    three_of_a_kind = False
    three_value = None
    for v in set(values):
        if values.count(v) == 4:
            return 7, v
        if values.count(v) == 3:
            three_of_a_kind = True
            three_value = v
        if values.count(v) == 2:
            pair_present = True
            pairs.append(v)

    if three_of_a_kind and pair_present: return 6, (three_value, pairs[0])
    if flush: return 5, None
    if straight: return 4, max(values)
    if three_of_a_kind: return 3, three_value
    if len(pairs) == 2: return 2, pairs
    if len(pairs) == 1: return 1, pairs[0]
    return 0, max(values)

def tiebreaker(hand1, hand2, hand1_info, hand2_info):
    # Return True if player 1 wins
    #print(hand1, hand1_info, hand2, hand2_info)
    assert(type(hand1_info) != list) # Shortcut, no identical Two Pairs
    assert(type(hand1_info) == int) # Flushes (None type) can't be compared
    if hand1_info != hand2_info:
        return (hand1_info > hand2_info)

    values1 = sorted((c[0] for c in hand1), reverse=True)
    values2 = sorted((c[0] for c in hand2), reverse=True)
    print(values1, values2, values1 > values2)
    return (values1 > values2)

player1_wins = 0
ranks1 = [0]*10
ranks2 = [0]*10
with open("p054_poker.txt") as f:
    for line in f:
        s = line.split(' ')
        line_pairs = []
        for card in s:
            try:
                value = int(card[0])
            except:
                value = value_dict[card[0]]

            line_pairs.append((value, card[1]))

        hand1 = line_pairs[:5]
        hand2 = line_pairs[5:]
        hand1_rank, hand1_info = eval_hand(hand1)
        hand2_rank, hand2_info = eval_hand(hand2)

        ranks1[hand1_rank] += 1
        ranks2[hand2_rank] += 1

        if hand1_rank > hand2_rank:
            player1_wins += 1

        elif hand1_rank == hand2_rank and tiebreaker(hand1, hand2, hand1_info, hand2_info):
            player1_wins += 1

#print(eval_hand([(2,'D'), (2,'D'), (1,'H'), (4,'D'), (2,'D')]))
print(ranks1)
print(ranks2)
print(player1_wins)

First impressions - this code is very clear.  I like the variable names - that makes lines like if straight and flush read very naturally.

I don't think there's a need to treat a Royal Flush separately - this is the same rank as Straight Flush, just with the largest possible high-card.  The "should not occur" comment is irrelevant - no matter how rare, we must correctly score Straight Flush (and we would expect a thorough test set to include some).

Instead of needing a separate tiebreaker function to evaluate the kickers, we could return a tuple from eval_hand, with the kickers following the rank+value.  We're already doing something like that for Full House; it's easy to extend that throughout.  I'd collect values in descending order:

values = sorted([c[0] for c in hand], reverse=True)
straight = (values == range(max(values), min(values)-1, -1)) # but see below

Then we can return as much as is necessary for the rank:

if straight and flush:
    return 8, values[0]

if three_of_a_kind and pair_present: return 6, three_value, pairs[0], values
if flush: return 5, values
if straight: return 4, values[0]
if three_of_a_kind: return 3, three_value, values
if len(pairs) == 2: return 2, pairs, values
if len(pairs) == 1: return 1, pairs[0], values

Counting pairs/trips/quads can be simplified, given that we have the values in sorted order.  We can use itertools.groupby() to get the count of identical values, rather than needing to search with values.count():

trips = []
pairs = []
for v, group in itertools.groupby(values):
    count = sum(1 for _ in group)
    if count == 4:
        return 7, v, values
    if count == 3:
        trips.append(v)
    elif count == 2:
        pairs.append(v)

Some additional test cases indicated a few problems:

The test for a straight doesn't work when I compare a list against a range (Python 3.6.5).  I needed to materialise it as a list first:

straight = values == list(range(values[0], values[-1]-1, -1))

We don't test for an ace-low straight:

straight = (values == list(range(values[0], values[0]-5, -1))
            or values == [14, 5, 4, 3, 2])

Flushes should be comparable - highest differing card still tie-breaks between flushes of different suits.

And some simplifications:

Straights and flushes can both be returned before checking for pairs/trips/quads
We don't need boolean flags to say that three_value or pairs is populated - we can directly test those variables are truthy or not.
For ranks 0, 1, 2, the rank is exactly the number of pairs found.
Instead of try/except, we can populate value_dict with the digits 2 to 9.
We're not required to find anything other than the number of Player 1 wins, so remove the code that was useful while debugging.

Applying the above, I get the following modified code:

import itertools

value_dict = {'T': 10, 'J': 11, 'Q': 12, 'K': 13, 'A': 14}
value_dict.update((str(x), x) for x in range(2,10))

def eval_hand(hand):
    # Return ranking followed by tie-break information.
    # 8. Straight Flush
    # 7. Four of a Kind
    # 6. Full House
    # 5. Flush
    # 4. Straight
    # 3. Three of a Kind
    # 2. Two pair
    # 1. One pair
    # 0. High card

    values = sorted([c[0] for c in hand], reverse=True)
    suits = [c[1] for c in hand]
    straight = (values == list(range(values[0], values[0]-5, -1))
                or values == [14, 5, 4, 3, 2])
    flush = all(s == suits[0] for s in suits)

    if straight and flush: return 8, values[1]
    if flush: return 5, values
    if straight: return 4, values[1]

    trips = []
    pairs = []
    for v, group in itertools.groupby(values):
        count = sum(1 for _ in group)
        if count == 4: return 7, v, values
        elif count == 3: trips.append(v)
        elif count == 2: pairs.append(v)

    if trips: return (6 if pairs else 3), trips, pairs, values
    return len(pairs), pairs, values

player1_wins = 0
with open("p054_poker.txt") as f:
    for line in f:
        cards = [(value_dict[x[0]], x[1]) for x in line.split(' ')]
        player1_wins += eval_hand(cards[:5]) > eval_hand(cards[5:])

print(player1_wins)


Datepickers for departure and return dates I have to two datepickers. I want achieve the following functionality: the return date cannot be selected before the selected departure date and the departure date cannot be after the selected return date. Mainly I am interested in correctness of my angular architecture.

I have moved datepicker into my custom directive which I plan to reuse many times for different screen sizes.

Both datepickers reuse the same DatepickerContoller and I have one parent controller FormDatepickerController which shares information from both outbound and return datepickers.

Could you please review my code and let me know if my design is good enough?

index.html

            <date-picker date="fd.departDate" min-date="fd.departDate"></date-picker>

            <date-picker date="fd.returnDate" min-date="fd.departDate"></date-picker>

datepicker.html template

    <input type="text" class="form-control"
           datepicker-popup
           min-date="minDate"
           ng-model="date"
           is-open="isOpened"
           ng-required="true"
           close-text="Close" />

app.js

var app = angular.module("fridayFly", ['ui.bootstrap']);

datepicker-controller.js

var app = angular.module("fridayFly");

(function(){

    function Controller($scope) {
        $scope.open = function($event) {
            $event.preventDefault();
            $event.stopPropagation();

            $scope.isOpened = true;
        };

        $scope.today = function() {
            $scope.dt = new Date();
        };
        $scope.today();

        $scope.clear = function () {
            $scope.dt = null;
        };

        $scope.toggleMin = function() {
            $scope.minDate = $scope.minDate ? null : new Date();
        };
        $scope.toggleMin();

        $scope.dateOptions = {
            formatYear: 'yyyy',
            startingDay: 1
        };

        $scope.formats = ['dd-MMMM-yyyy', 'yyyy/MM/dd', 'dd.MM.yyyy', 'shortDate'];
        $scope.format = $scope.formats[0];

    }

    app.controller('DatePickerController', Controller);

})();

(function(){

    function directive(){
        return {
            scope :{
                date:'=date',
                minDate:'=minDate',
                isOpened:'=isOpened'
            },
            restrict: 'E',
            templateUrl: 'templates/datepicker/datepicker.html',
            controller: 'DatePickerController'
        };
    };

    app.directive('datePicker', directive);

})();

form-datepicker-contoller.js

var app = angular.module("fridayFly");

app.controller('FormDatepickerController', ['$scope', function($scope) {

    $scope.toggleDepartDate = function() {
        $scope.departDate = $scope.departDate ? null : new Date();
    };
    $scope.toggleDepartDate();

    $scope.toggleReturnDate = function() {
        $scope.returnDate = $scope.returnDate ? null : new Date();
    };
    $scope.toggleReturnDate();

}]);

Excess controllers?
This code appears to be similar to several examples of UI Bootstrap datepickers - e.g. this plunker. Initially the layout looked fine but then I realize that there are three total instances of DatePickerController - one for each <date-picker> plus the wrapping <div ng-controller="DatePickerController as dp">. My presumption is that you should be able to implement this without that wrapping DatePickerController but maybe there is something I am missing.
Descriptive function/class names
I noticed that the functions for the controller and the directive are named Controller and directive, respectively. Most widely-accepted style guides call for class names in UpperCamelCase while method/function names should be in lowerCamelCase (e.g. the Google Style Guide). As your application grows it would be advisable to make the function names more descriptive - e.g. datePickerController and datePickerDirective.
Functions only called once
Do the methods toggleDepartDate() and toggleReturnDate() in FormDatepickerController get called anywhere other than the controller initializer (i.e. immediately after they are declared)? If not, then the functionality within them could be moved outside the functions.
IIFEs could be combined
There are two IIFEs in datepicker-controller.js. Those could be combined into a single IIFE because there isn't any reason to have them separated (at least code-wise).


Handling web services in an iOS app I'm building an iOS app that uses web services extensively. I've built classes that handle the requests. However since there are a lot of web service request. I need to find out how to detect which web service to run, based on the action - registration, login, image upload, etc.

This is just a small snippet of a very large method I came up with:

- (BOOL) wsExecution:(int) wsID wsParameters: (NSDictionary*) parameters
{

    switch (wsID)
    {
        case WEB_SERVICE_ID
        {

            ClassWS* ws = [[ClassWS alloc] initWithURL:WEB_SERVICE_ID Url:WEB_SERVICE_URL];

            break;
        }
        case AAWEB_SERVICE_ID
        {

            ClassWS* ws = [[ClassWS alloc] initWithURL:AAWEB_SERVICE_ID Url:AAWEB_SERVICE_URL];

            break;
        }

The web service IDs are defined in a file called constants. I then map a URL to an ID in the method above. There are another 30 switch cases in this method. The Web Service IDs are just ints that have no relevance to the actual web service. It was just a way to check which service, based on a switch statement.

It makes it hard to read and I think it can be done better. Is there a way to do with and remove the notion of a web service ID completely?

To be a little more clear on where I am going with this:

Here is another example of the switch case (I have counted 200 cases):

switch (wsID)
    {
        case RUN_ORDER_WS_ID:
        {
            NSDictionary* postData = [NSDictionary dictionaryWithObject:[parameters objectForKey:@"OrderResponse"] forKey:@"OrderResponse"];

            NSMutableDictionary* newParams = [NSMutableDictionary dictionaryWithDictionary:parameters];

            OrderWS* ws = [[OrderWS alloc] initWithURL:RUN_ORDER_WS_ID Url:RUN_ORDER_WS_ID_WS_URL];
            self.placeClassifiedResponseWS = ws;
            ws.delegate = self;
            [newParams removeObjectForKey:@"OrderResponse"];
            [ws fetch:newParams PostDataValuesAndKeys:postData];
            [ws release];

            break;
        }
        case GET_LIST_OF_USERS_ID:
        {
            NSDictionary* postData = [NSDictionary dictionaryWithObject:[parameters objectForKey:@"userList"] forKey:@"userList"];

            NSMutableDictionary* newParams = [NSMutableDictionary dictionaryWithDictionary:parameters];

            UsersWS* ws = [[UsersWS alloc] initWithURL:GET_LIST_OF_USERS_ID Url:GET_LIST_OF_USERS_URL];
            self.userListWS = ws;
            ws.delegate = self;
            [newParams removeObjectForKey:@"userList"];
            [ws fetch:newParams PostDataValuesAndKeys:postData];
            [ws release];

            break;
        }
        case GET_USER_REVIEW_FROM_ITEMS_WS_ID:
        {
            NSString* itemId = [parameters objectForKey:@"itemId"];
            NSMutableDictionary* newParams = [NSMutableDictionary dictionaryWithDictionary:parameters];
            NSString* newURL = [NSString stringWithFormat:GET_USER_REVIEW_FROM_ITEMS_WS_URL, itemId];

            DataStoreWS = [[DataStoreWS alloc] initForComponents: [NSArray arrayWithObjects:REVIEW_COMPONENT, nil] WebserviceID: wsID Url:newURL];
            self.reviewOfUserWS = ws;
            ws.delegate = self;
            [newParams removeObjectForKey:@"itemId"];
            [ws fetch:newParams];
            [ws release];

            break;
        }

Here's a quick and dirty example to illustrate the benefit of the dictionary setup:

- (NSDictionary *)webServiceIDsAndURLs{
    NSDictionary *sillyExampleDictionary = @{ @1 : [NSURL URLWithString:@"http://www.noreagle.com"],
                                              @2 : [NSURL URLWithString:@"http://www.stackOverflow.com"],
                                              @3 : [NSURL URLWithString:@"http://www.etc.com"]};
    return sillyExampleDictionary;
}

...

- (BOOL) webServicesExecution:(NSNumber *) webServiceID webServiceParameters: (NSDictionary*) parameters{
    NSURL *URL = [[self webServiceIDsAndURLs] objectForKey:webServiceID];
    ClassWebService *webService = [[ClassWebService alloc] initWithID:webServiceID URL:URL];

    // see, no lengty switch, yay!

    return YES; // Not sure what you are planning here...
}


Project Euler #3 - largest prime factor I was going through the Project Euler problem #3 and made a program to solve it. The problem is as follows:

  The prime factors of 13195 are 5, 7, 13 and 29. What is the largest
  prime factor of the number 600851475143 ?

My code is as follows:

#include<iostream>
#include<vector>
using std::cout;
using std::endl;
using std::vector;
typedef long long int llint;

vector<llint> computeFactors(llint);
vector<llint> computePrimeFactors(vector<llint>&);
llint getLargestPrimeFactor(const llint);
bool isPrime(const llint);

int main() {
   llint num = 600851475143;
   llint prime;
   cout<<"The number to factorize is : "<<num<<endl;
   //call the function getLargestPrimeFactor to get the factor
   prime = getLargestPrimeFactor(num);
   cout<<"The largest prime factor is : "<<prime<<endl;
   return 0;
}

//////////////////////////////////////////////////////////////
////////// GET THE LARGEST PRIME FACTOR BY MAGIC /////////////
//////////////////////////////////////////////////////////////

llint getLargestPrimeFactor(const llint n) {
   vector<llint> list, primeList;

   list = computeFactors(n);
   //if n itself is prime then return n;
   if(list.size()==2) {
      return *(primeList.end());
   } else {
      primeList = computePrimeFactors(list);
   }
   for(vector<llint>::iterator i=primeList.begin(); i<primeList.end(); i++) {
      cout<<*i<<endl;
   }
   vector<llint>::iterator i = primeList.end() - 1;
   return *i;
}

//////////////////////////////////////////////////////////////
/////////// GET THE FACTORS  /////////////////////////////////
//////////////////////////////////////////////////////////////

vector<llint> computeFactors(llint n) {
   vector<llint> list;
   //iterate through every number in the range
   for(llint i=1; i<=n; i++) {
      //find out whether any of the number divides n
      if(n%i==0) {
         //if it does push it into the vector
         list.push_back(i);
         n=n/i;
      }
   }
   return list;
}

//////////////////////////////////////////////////////////////
/////////// GET THE PRIME FACTORS ////////////////////////////
//////////////////////////////////////////////////////////////

vector<llint> computePrimeFactors(vector<llint>& factorList) {
   vector<llint> prime;
   //iterate through every number in the vector
   for(vector<llint>:: iterator i=factorList.begin();
                               i<factorList.end();
                               i++)
   {
      //check whether each of the element is prime
      if(isPrime(*i)) {
         prime.push_back(*i);
      }
   }
   return prime;
}

//////////////////////////////////////////////////////////////
/////////// WHETHER A PRIME NUMBER ///////////////////////////
//////////////////////////////////////////////////////////////

bool isPrime(const llint n) {
   int count = 0;
   for(llint i=1; i<=n; i++) {
      if(n%i==0) {
         count++;
      }
   }
   if(count == 2) {
      return true;
   } else {
      return false;
   }
}

My doubts start from here:

I called a single function from main() that gives me the result.
Now that function calls other functions from itself rather than in main().
Those sub-functions call other functions to achieve the task.

So now while designing the program, I just had to think about a single function that gave me the result. Now I decompose the function into different parts.

So when I call the getLargestPrimeFactor(), others are implicitly called.

But what I could have done is that I could have called each function explicitly in main().

Is there a difference in between the two approaches ? Does it really matter ? In what cases thinking like this can be harmful ? I have studied a bit about cohesion and coupling. I just want to get the habits right from the beginning.

Algorithm

I think you've gotten distracted by focusing on the wrong aspect of the problem.

The challenge asks for the largest prime factor of n = 600851475143.  There are three approaches you might consider in tackling this challenge:

Construct the list of all relevant prime numbers, then take the largest one that is a factor of n.

How high do we need consider?  The largest candidate would be \$\frac{n}{3}\$.

What algorithm could we use?  A good method to build a list of many prime numbers is the Sieve of Eratosthenes.  That would require \$\frac{n}{3}\$ bits of memory, or 23 GiB. Even if you had a machine with that much memory, just initializing that much memory to zero would be too slow to be reasonable as a Project Euler solution.

Any other algorithm for building a list of primes would only be worse.

You didn't choose Door #1.  Good for you!
Test the largest candidate factors first.

Where would you start?  The largest candidate would be \$\left\lceil\sqrt{n}\right\rceil\$.  You can test odd candidates in descending order, stopping as soon as you find an answer that is prime.

How do you guarantee that the answer is prime?  You have to check for primality.  We've already ruled out the Sieve of Eratosthenes above, so you would have to use trial division or a fancier primality-testing algorithm.

How long would we expect this to take?  \$\left\lceil\sqrt{n}\right\rceil\$ is 775147.  We would have 387573 possible odd factors to test.  There's no telling where we'll find numbers that are actually factors of \$n\$.  Once we find a factor, we still have to verify whether it's prime.

Food for thought: what would you do if \$n\$ were even?

You didn't choose Door #2, even though some other users have advocated it.  Good for you!
Test the smallest candidate factors first.

Suppose that I asked you to find the largest prime factor of 10241.  How would you go about it?  Would you start at \$\left\lceil\sqrt{10241}\ \right\rceil\$, trying 103, 101, 99, 97, 95, …?

Chances are, you would prefer to do it the following way instead, and you could do it with just pencil and paper.  (It's also easy to implement as one function.)

10241 / 2 … no.
10241 / 3 … no.
10241 / 5 … no.
10241 / 7 = 1463
1463 / 7 = 209
209 / 7 … no.
209 / 9 … no.
209 / 11 = 19
19 / 11 … no.
19 / 13 … no.
19 / 15 … no.
19 / 17 = no.
19 / 19 = 1
done!

As you can see, the big win comes when you find that 10241 is divisible by 7 — you immediately reduce the search space by a factor of 7.  In just a few iterations, you've reduced the problem size from 10241 down to 209 — a much more manageable number.

Bonus question: in the example above, do we still need to test 19 for primality?  Why or why not?

As it turns out, strategy 3 also works well for Project Euler Problem 3.  In general, strategy 3 is a better bet than strategy 2, since prime numbers tend to be more densely clustered closer to 0.

You've chosen an algorithm that is similar to strategy 3, which is good.  However, it isn't the same solution.  Your computeFactors() function very closely mimics the algorithm illustrated for strategy 3, but with a subtle and crucial difference.  Can you spot it?


Java code to delete comments from an input string I wrote this code to delete comments from an input string (such as a Java file converted to string) and it works great. For simple curiosity, I was wondering if there was a more elegant way of presenting the function that I wrote.

I'm looking for some advice on presenting the code, and how to condense the code.

Here's the function:

public static String deleteComments(String myString) {
    String newString = "";

    if (myString.contains("\"")) {
        if (myString.indexOf("\"") != 0) {

            String[] stringParts = myString.split("\"");

            for (int i = 0; i < stringParts.length; i++) {

                if ((i & 1) == 0) {
                    Pattern commentaryPattern = Pattern.compile("(/\\*((.|\n)*?)\\*/)|//.*");

                    Matcher m = commentaryPattern.matcher(stringParts[i]);

                    newString += m.replaceAll("");
                } else {
                    newString += "\"" + stringParts[i] + "\"";
                }
            }
        }
    } else {
        Pattern commentaryPattern = Pattern.compile("(/\\*((.|\n)*?)\\*/)|//.*");

        Matcher m = commentaryPattern.matcher(myString);

        newString += m.replaceAll("");
    }

    return newString;
}

Note: the regex pattern comes from another Stack Overflow link. It does most of the work but also deletes comments inside a string
(eg: String myString = "bob said, \"Hi //To his friendly neighbor\".";) which, according to the java doc, is not a comment.

Advice 1

newString += m.replaceAll("");

This is inefficient. Use StringBuilder for literally building the strings. Your approach works, but if you += \$n\$ characters one at a time, the time complexity is \$\Theta(n^2)\$. With StringBuilder it would be \$\Theta(n)\$.

Alternative implementation

The following is by no means a suggested implementation, but rather a starting point:

import java.util.Scanner;
import java.util.regex.Matcher;
import java.util.regex.Pattern;

public class Main {

    public static String deleteComments2(String str) {
        StringBuilder sb = new StringBuilder(str.length());
        boolean inText = false;
        char previousCharacter = '\u0000';

        for (int i = 0; i < str.length(); ++i) {
            char currentCharacter = str.charAt(i);

            if (inText) {
                if (currentCharacter == '\"' && previousCharacter != '\\') {
                    inText = false;
                }
            } else {
                if (currentCharacter == '\"') {
                    if (previousCharacter == '\\') {
                        throw new RuntimeException("bad syntax");
                    }

                    inText = true;
                }

                if (currentCharacter == '/' && previousCharacter == '/') {
                    sb.deleteCharAt(sb.length() - 1);
                    break;
                }
            }

            sb.append(currentCharacter);
            previousCharacter = currentCharacter;
        }

        return sb.toString();
    }

    public static void main(String[] args) {
        Scanner scanner = new Scanner(System.in);

        while (true) {
            String str = scanner.nextLine();

            if (str.equals("quit")) {
                break;
            }

            System.out.println("1> " + deleteComments(str));
            System.out.println("2> " + deleteComments2(str));
            System.out.println("---");
        }
    }

    public static String deleteComments(String myString) {
        String newString = "";

        if (myString.contains("\"")) {
            if (myString.indexOf("\"") != 0) {

                String[] stringParts = myString.split("\"");

                for (int i = 0; i < stringParts.length; i++) {

                    if ((i & 1) == 0) {
                        Pattern commentaryPattern = Pattern.compile("(/\\*((.|\n)*?)\\*/)|//.*");

                        Matcher m = commentaryPattern.matcher(stringParts[i]);

                        newString += m.replaceAll("");
                    } else {
                        newString += "\"" + stringParts[i] + "\"";
                    }
                }
            }
        } else {
            Pattern commentaryPattern = Pattern.compile("(/\\*((.|\n)*?)\\*/)|//.*");

            Matcher m = commentaryPattern.matcher(myString);

            newString += m.replaceAll("");
        }

        return newString;
    }
}