Run two computations in parallel, and return the results together I am reading the "Learning Concurrent Programming in Scala" and there are exercises in the end of each chapter.
One of exercises is

  Implement a parallel method which takes two computation blocks a and
  b, and starts each of them in a new thread. The method must return a
  tuple with the result values of both the computations. It should have
  the following signature: def parallel[A, B](a: => A, b: => B):(A, B)

I have implemented it in this way:

def parallel[A, B](a: => A, b: => B):(A, B) = {
  var aResult:Option[A] = Option.empty
  var bResult:Option[B] = Option.empty
  val t1 = thread{aResult = Some(a)}
  val t2 = thread{bResult = Some(b)}
  t1.join()
  t2.join()
  (aResult.get, bResult.get)
}

where thread is

def thread(block: =>Unit):Thread = {
  val t = new Thread{
    override def run(): Unit = block
  }
  t.start()
  t
}

My question is if this implementation is free of race condition? I have seen that other people had a similar implementations but declared result holder variables aResult and bResult to be volatile. I am not sure that this is necessary, and also I was not able to design a test which would break the correctness of my implementation.

From my point of view the implementation is free of race condition because the parallel method does not access any shared variables. All its state is in its local variables so is not shared. Thus I think that adding @volatile to aResult and bResult is not needed in this case.

From my point of view the implementation is free of race condition because the parallel method does not access any shared variables. All its state is in its local variables so is not shared.

That is not true. The variable aResult is shared between the thread that called parallel and the thread t1. So there needs to be some synchronization between these two threads, otherwise you run into undefined behaviour.

This low-level JDK memory model gets tricky, so I would try to avoid it as far as possible, i.e. use higher-level abstractions and prefer immutable state.

I suppose the following (still rather low-level) tools (which would solve these issues) are not available for this exercise:

Scala Futures
Java Callables (which return a result instead of having to publish it somewhere)
java.util.concurrent.AtomicReference (which you can use as a thread-safe holder if you have to use Runnable instead of Callable)

As for the specific question: According to the Java Memory Model FAQ

  All actions in a thread happen before any other thread successfully returns from a join() on that thread.

Meaning that after your master thread has joined the first worker thread, it will read the updated value of aResult without that field needing to be volatile.

So your code looks correct to me.

But I had to look this up. Really try to avoid mutable state when multiple threads are concerned.


Socket connect realization: gethostbyname or getnameinfo for now I'm using:

int connect(const String& address, int port) {
    struct sockaddr_in servAddr;
    struct hostent* host;            /* Structure containing host information */
    /* open socket */
    if((handle = socket(PF_INET, SOCK_STREAM, IPPROTO_TCP)) < 0)
        return ERROR;
    //TODO: gethostbyname is obsolete.
    if((host = (struct hostent*) gethostbyname(address)) == 0)
        return ERROR;
    memset(&servAddr, 0, sizeof(servAddr));
    servAddr.sin_family      = AF_INET;
    servAddr.sin_addr.s_addr = inet_addr(inet_ntoa(*(struct in_addr*)(host -> h_addr_list[0])));
    servAddr.sin_port        = htons(port);
    if(::connect(handle, (struct sockaddr*) &servAddr, sizeof(servAddr)) < 0)
        return ERROR;
    return OK;
    }

this procedure but everytime I compile it I'm getting:

socket.cpp:(.text+0x374): warning: gethostbyname is obsolescent, use getnameinfo() instead.

getname info is still being confusing stuff for me. here is my try to implement it:

struct sockaddr_in servAddr;
struct hostent *host;        /* Structure containing host information */

/* open socket */
if ((handle = socket(PF_INET, SOCK_STREAM, IPPROTO_TCP)) < 0)
    return ERROR;

memset(&servAddr, 0, sizeof(servAddr));
servAddr.sin_family      = AF_INET;
servAddr.sin_addr.s_addr = inet_addr(address.ptr());
servAddr.sin_port        = htons(port);

char servInfo[NI_MAXSERV];
if ( ( host = (hostent*) getnameinfo(
                 (struct sockaddr *) &servAddr
                 ,sizeof (struct sockaddr)
                 ,address.ptr(), address.size()
                 ,servInfo, NI_MAXSERV
                 ,NI_NUMERICHOST | NI_NUMERICSERV )  ) == 0)
    return ERROR;

if (::connect(handle, (struct sockaddr *) &servAddr, sizeof(servAddr)) < 0)
    return ERROR;

-- yes this doesn't work :(

Maybe I should use getaddrinfo instead?

getaddrinfo(hostname, NULL, &hints, &res) - can I use it alike gethostbyname? but where is host actually here? hints?

Working recode based on answer:

int Socket::connect(const String& address, int port) {
    struct sockaddr_in servAddr;
    /* open socket */
    if((handle = socket(PF_INET, SOCK_STREAM, IPPROTO_TCP)) < 0)
        return ERROR;
    //gethostbyname by getaddrinfo replacement
    addrinfo hints = {sizeof(addrinfo)};
    hints.ai_flags = AI_ALL;
    hints.ai_family = PF_INET;
    hints.ai_protocol = 4; //IPPROTO_IPV4
    addrinfo* pResult = NULL;
    int errcode = getaddrinfo(address, NULL, &hints, &pResult);
    if(errcode != 0)
        return ERROR;
    memset(&servAddr, 0, sizeof(servAddr));
    servAddr.sin_family      = AF_INET;
    servAddr.sin_addr.s_addr = *((uint32_t*) & (((sockaddr_in*)pResult->ai_addr)->sin_addr));
    servAddr.sin_port        = htons(port);
    if(::connect(handle, (struct sockaddr*) &servAddr, sizeof(servAddr)) < 0)
        return ERROR;
    return OK;
    }

This works for me:

int connect2(const CStringA& address, int port) {
  struct sockaddr_in servAddr;
  /* open socket */
  SOCKET handle;
  if((handle = socket(PF_INET, SOCK_STREAM, IPPROTO_TCP)) < 0)
    return ERROR;
  //gethostbyname by getaddrinfo replacement
  ADDRINFO hints;
  ZeroMemory(&hints, sizeof(hints));
  hints.ai_flags = AI_ALL;
  hints.ai_family = PF_INET;
  hints.ai_protocol = IPPROTO_IPV4;
  ADDRINFO* pResult = NULL;
  int errcode = getaddrinfo((LPCSTR)address, NULL, &hints, &pResult);
  if(errcode != 0)
    return ERROR;
  memset(&servAddr, 0, sizeof(servAddr));
  servAddr.sin_family      = AF_INET;
  servAddr.sin_addr.S_un.S_addr = *((ULONG*)&(((sockaddr_in*)pResult->ai_addr)->sin_addr));
  servAddr.sin_port        = htons(port);
  if(::connect(handle, (struct sockaddr*) &servAddr, sizeof(servAddr)) < 0)
    return ERROR;
  return OK;
}


Update join table using list of checkboxes in Rails I have Gig and Singer Active Record models (standard--no customization just yet) with a many-to-many relationship through a generic join table which holds nothing but the respective ids of Gig and Singer.  My form sends a given gig id and all the singers who will be attending, denoted with check boxes.  I need to have the ability to check or uncheck singers.  The following code works, but it does so by removing all the singers from a gig and re-adding them.  This feels hacky... is there a better way?  (I think this is all the code necessary but let me know if you need me to add anything)

class GigSingersController < ApplicationController

    def create
        gig = Gig.find(params[:gig_id])
        singer_ids = params[:singer_ids] # [1, 4, 5,]
        gig.singers = []
        singer_ids.each do |id|
            singer = Singer.find(id)
            gig.singers << singer
        end
        redirect_to gigs_path
    end
end

EDIT:

As requested in the comments, here are the schema and relevant models, although as I said, they are completely generic.  Perhaps I didn't do a good job of making my question clear:  Is the best way to create these relationships when using a checkbox to remove all existing ones and recreate them from the boxes currently checked, thereby removing any that the user unchecked on an edit?

ActiveRecord::Schema.define(version: 2019_07_19_195106) do

  create_table "gig_singers", force: :cascade do |t|
    t.integer "gig_id"
    t.integer "singer_id"
    t.datetime "created_at", null: false
    t.datetime "updated_at", null: false
  end

  create_table "gigs", force: :cascade do |t|
    t.string "name"
    t.text "notes"
    t.datetime "datetime"
    t.datetime "created_at", null: false
    t.datetime "updated_at", null: false
  end

  create_table "singers", force: :cascade do |t|
    t.string "name"
    t.datetime "created_at", null: false
    t.datetime "updated_at", null: false
    t.boolean "active"
  end

class Gig < ApplicationRecord
    has_many :gig_singers
    has_many :singers, through: :gig_singers
end

class GigSinger < ApplicationRecord
    belongs_to :gig
    belongs_to :singer
end

class Singer < ApplicationRecord
    has_many :gig_singers
    has_many :gigs, through: :gig_singers

end

Actually, Active Record can work with joined collection (including has_many :through) from the box.

So, you just need to pass new collection instead of old, and AR will delete excessive and add new records.

In your code, need to change create method body to:

gig = Gig.find(params[:gig_id])
singer_ids = params[:singer_ids] # [1, 4, 5,]
gig.singers = Singer.find(singer_ids)
redirect_to gigs_path


Script for copying files to the appropriate directory In the book Real World Haskell, the author writes in the first line of every file that uses, for example the following: file: ch05/PrettyJSON. The chapter slash and the name of a module.

I wanted to create a script which reads the first line and copies the files to the appropriate directory. In the above example we should copy the file to the directory ch05. I wanted to write this in Haskell in order to use it for "real world" applications.

But before starting writing to Haskell I wrote it in Python:

import os
import shutil

current_dir = os.getcwd()
current_files=os.listdir(current_dir)
haskell_files=filter(lambda x:x.find(".hs")!=-1,current_files)

for x in haskell_files:
 with open(x,"r") as f:
     header=f.readline()
    try:
        chIndex=header.index("ch")
    except ValueError as e:
        continue

    number=header[chIndex:chIndex+4]
 try:
    os.mkdir(number)
    shutil.copy2(x,number)
except OSError as e:
    shutil.copy2(x,number)

And then I tried to write the same code in Haskell:

f x = dropWhile (\x-> x/='c') x
g x = takeWhile (\x-> x=='c' || x=='h' || isDigit x) x
z= do
   y<-getCurrentDirectory
   e<-getDirectoryContents y
   let b=findhs e
   mapM w b
findhs filepaths = filter (isSuffixOf ".hs") filepaths
w filename= do
           y<-readFile filename
           let a=(g.f) y
           if "ch" `isPrefixOf` a
           then do
                 createDirectoryIfMissing False a
                 g<-getCurrentDirectory
                 writeFile (g ++"/"++ a ++ "/" ++ filename) y
            else
                return ()
main=z

Haskell code should be more elegant and more compact but it is not. Can you give me suggestions on making the above program more Haskellish?

Notes:

don't use single letter variable names for top level bindings
leave spaces around syntax like = and <-
use meaningful variable names for system values (such as file contents)
inline single uses of tiny functions, such as your findhs e
give your core algorithm, w, a meaningful name
don't use long names for local parameters like filepaths. fs is fine.
use pointfree style for arguments in the final position
use when instead of if .. then ... else return ()
use the filepath library for nice file path construction
your only use of f and g is to compose them. so do that and name the result.
use section syntax for lambdas as function arguments to HOFs
indent consistently
use mapM_ when the result you don't care about.

Resulting in:

import Data.Char
import Data.List
import Control.Monad
import System.Directory
import System.FilePath

clean = takeWhile (\x -> x == 'c' || x == 'h' || isDigit x)
      . dropWhile (/='c')

process path = do
   y <- readFile path

   let a = clean y

   when ("ch" `isPrefixOf` a) $ do
         createDirectoryIfMissing False a
         g <- getCurrentDirectory
         writeFile (g </> a </> path) y

main = do
    y <- getCurrentDirectory
    e <- getDirectoryContents y
    mapM_ process $ filter (isSuffixOf ".hs") e


Simple PyQt5 counting GUI I just began learning how to use signals and slots in PyQt5, and so I made a cute (pun intended) little program to display just one button. When the button is pressed a thread is spawned which will count from 0 to 99 and display the current count onto the button. However, when the button is counting, the program will not allow the user to spawn another thread.

I am about to actually use my knowledge for a much heavier task, and I wanted to know if there was an easier way to do what I did. Or, perhaps my way is not very efficient? Thanks in advance!

import threading
import sys
import time

from PyQt5.QtWidgets import *
from PyQt5.QtCore import *

class Application(QMainWindow):
    counter = pyqtSignal(str)
    counting = False

    def __init__(self):
        super(Application, self).__init__()

        self.button = QPushButton()
        self.button.setText('99')
        self.button.clicked.connect(self.startCounting)
        self.counter.connect(self.button.setText)

        self.layout = QVBoxLayout()
        self.layout.addWidget(self.button)
        self.frame = QFrame()
        self.frame.setLayout(self.layout)
        self.setCentralWidget(self.frame)

    def startCounting(self):
        if not self.counting:
            self.counting = True
            thread = threading.Thread(target=self.something)
            thread.start()

    def something(self):
        for x in range(100):
            self.counter.emit(str(x))
            time.sleep(.01)
        self.counting = False

if __name__ == '__main__':
    app = QApplication(sys.argv)
    window = Application()
    window.show()
    sys.exit(app.exec_())

Looks good to me!

super minor nits:

        thread = threading.Thread(target=self.something)
        thread.start()

only uses the thread variable once, so you might as well do:

        threading.Thread(target=self.something).start()

Also, the only thing you use from threading is Thread so you might as well change your import to:

from threading import Thread

and then it can be just:

Thread(target=self.something).start()

...but again, these are super minor things!  It looks good to me; I may have to look at pyQT again :)


Sed script that removes specific lines in `seq` output I have a file to process with sed but I am not quite familiar with the commands with capital letters used for multi-line patterns. I use seq to test the script and I have converted it to the problem described below. The sed script and expected output are also attached. I believe the script can be written in a much better way but I am not sure how to do it.

Problem description:

  Filter the output of seq $m, where $m is a given integer. removing the \$n^{\rm th}\$ line if either \$n-2\$, \$n-1\$, \$n\$, or \$n+1\$ contains the digit \$7\$.

Sed script (together with the seq pipe, note this is GNU sed):

seq "$m"|sed ':c N;N;N;:a N;s/.*\n.*7.*\n.*\n.*\n//g;tc;P;s/^[^\n]*\n//g;ba;'

I believe setting two labels (a and c) is not necessary.

Edit: There seems to be a nicer alternative, based on the answer to Delete 5 Lines Before and 6 Liens After Pattern Match Using sed, as follows,

seq "$m"|sed 'N;/7/!{P;D};:b N;s/\n/&/3;Tb;d'

This avoids writing out a lot of N; and \n's explicitly. Still I believe it can be improved.

Sample output for m=100

1
2
3
4
5
10
11
12
13
14
15
20
21
22
23
24
25
30
31
32
33
34
35
40
41
42
43
44
45
50
51
52
53
54
55
60
61
62
63
64
65
82
83
84
85
90
91
92
93
94
95
100

Related: Extension of Game of Sevens Challenge at CodeGolf

N;/7/!{P;D}

Nice job there, this very concisely (and clearly) allows you to capture both lines \$n\$ and \$n+1\$.

:b N;s/\n/&/3;Tb;d

Now the loop that follows it is mostly redundant. You are essentially appending lines of input until you are left with \$4\$ lines in total. You already have \$2\$ in your pattern space and need \$2\$ more, i.e. lines \$n-1\$ and \$n-2\$, which can simply be expressed in N;N followed by a d.


Break text into letters (regular Unicode graphemes+language specific digraphs) In English, "Sh" is two letters. In other languages it's considered a single letter. I'm trying to calculate the length of a string in a Unicode aware way, with this in mind. I'm imagining a function like

def count_letters(my_string, lang="en")

But here's how I would do that for just one language, using a library that has had no new versions since 2015, uniseg:

from uniseg.graphemecluster import grapheme_clusters

def albanian_digraphs(s, breakables):
    digraphs = ["Dh", "Gj", "Ll", "Nj", "Rr", "Sh", "Th", "Xh", "Zh"]
    digraphs += [d.lower() for d in digraphs]
    for i, breakable in enumerate(breakables):
        for first, second in digraphs:
            if s.endswith(first, 0, i) and s.startswith(second, i):
                yield 0
                break
        else:
            yield breakable

# from https://sq.wiktionary.org/wiki/Speciale:PrefixIndex?prefix=dh
for text in ('dhallanik', 'dhelpëror', 'dhembshurisht', 'dhevështrues', 'dhimbshëm', 'dhjamosje', 'dhjetëballësh', 'dhjetëminutësh', 'dhogaç', 'dhogiç', 'dhomë-muze', 'dhuratë', 'dhëmbinxhi', 'dhëmbçoj', 'dhëmbëkatarosh'):
    print(list(grapheme_clusters(text, albanian_digraphs)))

#['dh', 'a', 'll', 'a', 'n', 'i', 'k']
#['dh', 'e', 'l', 'p', 'ë', 'r', 'o', 'r']
#['dh', 'e', 'm', 'b', 'sh', 'u', 'r', 'i', 'sh', 't']
#['dh', 'e', 'v', 'ë', 'sh', 't', 'r', 'u', 'e', 's']
#['dh', 'i', 'm', 'b', 'sh', 'ë', 'm']
#['dh', 'j', 'a', 'm', 'o', 's', 'j', 'e']
#['dh', 'j', 'e', 't', 'ë', 'b', 'a', 'll', 'ë', 'sh']
#['dh', 'j', 'e', 't', 'ë', 'm', 'i', 'n', 'u', 't', 'ë', 'sh']
#['dh', 'o', 'g', 'a', 'ç']
#['dh', 'o', 'g', 'i', 'ç']
#['dh', 'o', 'm', 'ë', '-', 'm', 'u', 'z', 'e']
#['dh', 'u', 'r', 'a', 't', 'ë']
#['dh', 'ë', 'm', 'b', 'i', 'n', 'xh', 'i']
#['dh', 'ë', 'm', 'b', 'ç', 'o', 'j']
#['dh', 'ë', 'm', 'b', 'ë', 'k', 'a', 't', 'a', 'r', 'o', 'sh']

For starter, you do not need to create you digraphs list each time the function is called: they won't change, so better create them once as a global constant. You also forgot to add capitalized versions in the list so that 'HELLO' is split into ['H', 'E', 'LL', 'O'] instead of the current ['H', 'E', 'L', 'L', 'O'].

Second, your linear research in the digraphs list can be time consuming when all you want to know is if characters at position i-1 (if any) and i forms a digraph present in your list. I’d rather write it as yield 0 if s[i-1:i+1] in digraphs else breakable. Of course, for this to work efficiently, you will need digraphs to be a set instead of a list where lookups are \$\mathcal{O}(1)\$ instead of \$\mathcal{O}(n)\$.

Lastly, I would only consider lowercase variants and turn the selected two characters lowercase before checking:

from uniseg.graphemecluster import grapheme_clusters

DIGRAPHS = {"dh", "gj", "ll", "nj", "rr", "sh", "th", "xh", "zh"}

def albanian_digraphs(s, breakables):
    for i, breakable in enumerate(breakables):
        yield 0 if s[i-1:i+1].lower() in DIGRAPHS else breakable

if __name__ == '__main__':
    # from https://sq.wiktionary.org/wiki/Speciale:PrefixIndex?prefix=dh
    for text in ('dhallanik', 'dhelpëror', 'dhembshurisht', 'dhevështrues', 'dhimbshëm', 'dhjamosje', 'dhjetëballësh', 'dhjetëminutësh', 'dhogaç', 'dhogiç', 'dhomë-muze', 'dhuratë', 'dhëmbinxhi', 'dhëmbçoj', 'dhëmbëkatarosh'):
        print(list(grapheme_clusters(text, albanian_digraphs)))

Also note the use of if __name__ == '__main__' to separate the actual code from the tests.


Efficient smart pointer implementation in C++ The idea behind this is mainly educational but I might even consider using it in reality if turns out to be good. Here's my first try at implementing smart pointers:

template<typename T>
class smart_pointer{
    T* pointer;
    std::size_t *refs;

    void clear(){
        if (!--*refs){
            delete pointer;
            delete refs;
        }
    }

public:
    smart_pointer(T* p = NULL)
        : pointer(p), refs(new std::size_t(1))
    {}
    smart_pointer(const smart_pointer<T>& other)
        : pointer(other.pointer), refs(other.refs)
    {
        ++*refs;
    }
    ~smart_pointer(){
        clear();
    }

    smart_pointer<T>& operator=(const smart_pointer<T>& other){
        if (this != &other){
            clear();

            pointer = other.pointer;
            refs    = other.refs;
            ++*refs;
        }
        return *this;
    }

    smart_pointer<T>& operator=(T* p){
        if (pointer != p){
            pointer = p;
            *refs = 1;
        }
        return *this;
    }

    T& operator*(){
        return *pointer;
    }

    const T& operator*() const{
        return *pointer;
    }

    T* operator->(){
        return pointer;
    }

    const T* operator->() const{
        return pointer;
    }

    std::size_t getCounts(){
        return *refs;
    }
};

I have tested this under valrind and its clean. Also, to see how much the "smartness" makes things slower, I did the following test:

struct foo{
    int a;
};

template<typename pointer_t>
class bar{
    pointer_t f_;

public:
    bar(foo *f)
        :f_(f)
    {}

    void set(int a){
        f_->a = a;
    }
};

int main()
{
    foo *f = new foo;

    typedef smart_pointer<foo> ptr_t;
//    typedef boost::shared_ptr<foo> ptr_t;
//    typedef foo* ptr_t;

    bar<ptr_t> b(f);
    for (unsigned int i = 0; i<300000000; ++i)
        b.set(i);

//    delete f;
    return 0;
}

Here is some timing between my implementation, boost, and raw pointers: (code compiled with clang++ -O3)

typedef smart_pointer<foo> ptr_t;

real   0m0.006s
user   0m0.001s
sys    0m0.002s

typedef boost::shared_ptr<foo> ptr_t;

real    0m0.336s
user    0m0.332s
sys     0m0.002s

typedef foo* ptr_t;

real    0m0.006s
user    0m0.002s
sys     0m0.003s

My implementation seems to be running almost as fast as raw pointers, which I think is a good sign.  What worries me here is why Boost is running slower.  Have I missed something in my implementation that is important and I might get into trouble for later?

So the idea behind this is mainly educational...

Cool. Always good to try and understand how things work under the covers.

  ...but I might even consider using it in reality if turns out to be good.

Please rethink this. Smart pointer implementations are incredibly difficult to get right. Scott Myers, of Effective C++ fame, famously tried to implement a shared_ptr. After feedback from something like 10 iterations, it was still wrong.

Let's go through a few things here.

smart_pointer(T* p = NULL)
  : pointer(p), refs(new std::size_t(1))

This constructor should be explicit to avoid implicit type conversions through construction.

Your operator=(T* p) leaks. Here's a small example:

int main()
{
    int *x = new int(5);
    {
        smart_pointer<int> sp(x);
        sp = new int(6);
    } // sp destroyed here

    std::cout << *x << "\n"; // This should have been deleted but wasn't
}

To convince yourself that this is the case, modify your clear() method:

void clear(){
    if (!--*refs){
        std::cout << "deleteting " << *pointer << " at " << pointer << "\n";
        delete pointer;
        delete refs;
    }
}

This will print out "deleting 6 at <some memory address>".

Here's just a small rundown of what boost::shared_ptr (or std::shared_ptr) provides that is missing here:

Constructors allowing a Deleter, which is used instead of a raw delete.
Constructor allowing construction from <template Tp1> shared_ptr(Tp1 * ...) where Tp1 is convertible to type T.
Copy constructor allowing construction from a shared_ptr with convertible template type <T1>.
Copy constructor taking weak_ptr, unique_ptr.
Move constructor and assignment operator.
Usage of nothrow where it is required to be.
reset, which will replace the currently managed object.
All the non-member operators like operator<, operator==, etc that allow containers and algorithms to work correctly. For example, std::set will not work correctly with your pointer class, neither will things like std::sort.
A specialization of std::swap.
A specialization of std::hash. Your pointer class won't work correctly with std::unordered_set or std::unordered_map.
explicit operator bool conversion for nullptr tests, so that one can write if (<some_shared_ptr>) { ... }.
Utility functions like std::make_shared which you should pretty much always use when creating a shared_ptr.

The big pain point, and the reason why boost::shared_ptr or std::shared_ptr will be slower is the fact that they are thread-safe. Your smart_ptr when accessed by multiple threads could very easily delete the pointer it holds before another class is done with it, leading to dereferencing of a deleted object, and thus undefined behaviour. Furthermore, all of the atomic operations are specialized for shared_ptr.

The above is not an exhaustive list.

I don't write this to discourage you, but merely to try and reinforce the fact that writing something like this is a lot of work. It is very technical, and really, really easy to get wrong. In terms of the standard library, shared_ptr may be one of the most difficult things to write correctly.


PHP magic function for accessors and mutators I have implemented the following PHP magic method behavior to save my time of creating accessor and mutators for properties of any class that extends this base class.

I am looking for suggesiongs on improving it for my application that assumes the following:

Properties of the form:

$_first_name
$_user_id

Methods of the form:

getUserId()
setFirstName("Mohammad")

I have some points that I think need more research:

Is it a secure way? What are the insecurities here?
Is my way of handling errors by using throw new Exception('my message'); a good practice? What are the alternatives?
Is there a best practice or a design pattern that addresses such a solution for reusability?
Am I exaggerating the steps? Are there any steps can be combine into one step with the same results?

The __call function of my Base class:

class Base
{
    private $_first_name; // just an example

    public function __call($name, $args)
    {
        // The begining of a method must be in all lower case characters.
        // E.g. set, get
        $result = preg_match('/^[a-z]*/', $name, $matches);

        if($result !== 1)
        {
            throw new Exception('Action not recognized.');
        }

        // Hold the found action.
        $action = $matches[0];

        // Find the rest of the method name.
        $result = preg_match_all('/[A-Z][a-z]*/', $name, $matches);

        // $matches will hold a multi-dimensional array
        // we need the first 1D array only.
        $matches = $matches[0];

        if($result < 1 || $result === FALSE)
        {
            throw new Exception('Malformed method name.');
        }

        // Construct the property name.
        $property = '_' . strtolower(implode('_', $matches));

        if(! property_exists($this, $property))
        {
            throw new Exception("Property: '{$property}' not found.");
        }

        switch($action)
        {
            // Mutator
            case 'set':
            {
                if(count($args) === 1)
                {
                    $this->$property = $args[0];
                    return;
                }
                else
                {
                    throw new Exception('You must provide 1 argument only.');
                }
            }

            // Accessor
            case 'get':
            {
                return $this->$property;
            }
        }
    }
}

Exceptions are fine, but consider using less general Exception classes, like BaseException or whatever. This will allow for more flexible error reporting.

First thing that strikes me as odd about your code is that you're using underscore to name your properties, but CamelCase to name your methods. This is quite inconsistent and leads to unnecessary complexity here. Using naming convention that actually helps you is a good idea.

With that in mind, a little change in naming convention could turn into something like this:

function __call($method, $args)
{
    $action = substr($method, 0, 3);
    $prop = strtolower(substr($method, 3));
    $getter = "get".ucfirst($prop);
    $setter = "set".ucfirst($prop);

    switch ($action) {
        case "get" :
            if (isset($this->_fields[$prop])) {
                return $this->_fields[$prop];
            } else throw new EntityException("Unknown property {$prop}");
            break;
        case "set" :
            if (isset($this->_fields[$prop])) {
                $this->_fields[$prop] = $args[0];
                return null;
            } else
                throw new EntityException("Unknown property {$prop}");
            break;
        default : throw new EntityException("Unknown action {$action[0]}");
    }
}

Notice i use a an array $this->_fields to store all my fields. This goes for my second point - Safety and Reusability.

Consider this piece of code:

class User
{
    private $_name;

    function getName(){
        return $this->_name;
    }
}

We use our getName method everywhere, but later we need to add another field - second name. We can safely insert this second field inside our getName and the code would change without taking any further actions. This is kinda the whole point behind getters and setters - incapsulating logic away.

Now with your code you'll have complications with overriding your setters, and that doesn't make it safe in any way.

Now, reusability. In your base class, you write all your fields explicitly, which doesn't seem very reusable to me. Look this code (warning, large wall of text incoming).

abstract class Entity
{
    private $_fields;
    protected $schema = array();

    function __construct(array $data = array())
    {
        /** Check that our concrete implementation has its Schema */
        if (!$this->schema) {
            throw new EntityException("Class ".get_called_class()." Schema not defined");
        }
        /**
         * mapping fields value to our schema
         *
         * @TODO Move to separate method to do this not only on Construct;
         */

        $this->_fields = array_reduce(
            $this->schema,
            function ($fields, $element) use ($data) {
                $fields[$element] = $data[$element];
                return $fields;
            }
        );
    }

    function __call($method, $args)
    {
         /** Same call implementation here as above */
    }

    /**
     * Inner getter for getter/setter overrides
     */

    protected function getField($name)
    {
        if (!isset($this->_fields[$name])) {
            throw new EntityException ("Property {$name} not found");
        }
        return $this->_fields[$name];
    }

    /**
     * Inner setter for getter/setter overrides
     */

    protected function setField($name, $value)
    {
        if (!isset($this->_fields[$name])) {
            throw new EntityException ("Property {$name} not found");
        }
        $this->_fields[$name] = $value;
    }
}

/**
 * Class User
 *
 * Concrete implementation of Entity class
 *
 */
class User extends Entity
{
    /**
     * List of our entity fields
     * @var array
     */
    protected $schema = array("id", "name", "email", "phone");

    /**
     * method getName concrete implementation;
     *
     * @return string
     */

    function getName()
    {
        return "My name is ".$this->getField("name");
    }
}

try {
    $userData = array("id" => 1, "name" => "Andrew", "phone" => "000000000", "email" => "test@test.com");

    $user = new User($userData);

    echo $user->getName()."
";
    echo $user->getEmail()."
";
    $user->setPhone("111111111");
    echo $user->getPhone();

} catch (Exception $e) {
    echo $e->getMessage();
}

Now, if you want to create a News entity, you can simply write:

class News extends Entity
{
   protected $schema = array("id", "title", "text", "time");
}

And use it at your leisure. Later you can add active record methods to Entity, like save, delete, update and so on, and still use them on any entity you might want.

As to why i used array $_fields instead of simply listing fields - array is much more convenient when you have to map values to your fields and build queries from them.


Find the longest sequential same character array I am a new guy to Java. I want to find the longest sequential same character array in a input character arrays.

For example, this character array:

bddfDDDffkl

The longest is DDD. Next example:

rttttDDddjkl

The longest is tttt.

I use the following code to deal with this problem but I want to improve my code. Could you tell me if there are any edge cases here? For example, if there are two same length arrays (for example rtttgHHH, there are two longest: ttt and HHH), how to solve this situation?

My following code:

public class SeqSameChar {
    public static void main (String[] args) {
     int subLength = 0;
    Scanner sc = new Scanner(System.in);
    String[] num = null;
    num = sc.nextLine().split(" ");
    String[] number = new String[num.length];
    for(int i = 0; i< number.length;i++) {
     number[i] = String.valueOf(num[i]);
    }
    subLength =length(number,num.length);
    System.out.println(subLength);
    for(int i = index; i < index+subLength; i++) {
     System.out.print(number[i]);
    }
    }
    public static int index;
    //to calculate the longest contiguous increasing sequence
    public static int length(String[] A,int size){
    if(size<=0)return 0;
    int res=1;
    int current=1;
    for(int i=1;i<size;i++){
       if(A[i].equals(A[i-1])){
           current++;
       }
       else{
           if(current>res){
               index=i-current;
               res=current;
           }
           current=1;
       }
     }
    return res;
    }
}

The code has  a bug:

q q q u u u u u u
3
qqq

It should be

6
uuuuuu


Python Weighted Object Picker I've designed a class ObjectPicker that can be given objects and weights. Objects can be picked randomly out of the ObjectPicker, with heavier weighted objects having a higher chance of being selected.

The pick() method is designed to be recursive and provide \$O(\log n)\$ speed. What I'm not sure about is the defaulting that's currently in place. I'm wondering if there is a better way to handle it. However, I would appreciate any feedback you are willing to provide beyond the specific.

class ObjectPicker:
    """
    An object that stores other objects with weights, and is able to
    pick out one stored object, with a higher chance for higher weighted
    objects.
    """

    def __init__(self):
        """
        Initialize the Object Picker object. Create the instance variable
        bucket for storing objects.
        """
        self.bucket = []

    def add(self, item, weight=1):
        """
        Add an item to the bucket, with given weight.
        :param item: Object to be stored.
        :param weight: Weight given to the object to determine odds.
        """
        if self.bucket:  # If there is anything in the bucket.
            total = self.bucket[-1][-1]  # Get the end number of the last item.
        else:  # If the bucket is empty.
            total = 0  # End number is 0.

        # Object stored in Tuple.
        # (Object, Weight, Starting Num, Ending Num)
        self.bucket.append((item, weight, total, total + weight))

    def pick(self, choice=-1, storage=None):
        """
        Pick an object from the bucket recursively,
        taking weight into account.
        :param choice: Number of choice.
        :param storage: Storage Object to choose from.
        """
        if not storage:  # If no storage is given.
            if not self.bucket:  # If bucket is empty.
                return None  # Return None.
            else:
                storage = self.bucket  # Storage is bucket
                total = storage[-1][-1]  # Get final weight.

        if choice < 0:  # If choice is < 0,
            # Randomly choose a number to represent the choice.
            choice = random.random() * total

        # Start binary search in middle of storage object.
        index = len(storage) // 2
        start, end = storage[index][2:]

        # If the choice is lower, recursively search left half.
        if choice < start:
            return self.pick(choice, storage[:index])
        # If the choice is higher, recursively search right half.
        elif choice > end:
            return self.pick(choice, storage[index + 1:])
        # Otherwise, choice is in number spread and return object.
        else:
            return storage[index][0]

doc strings

Good. I like seeing doc strings. But, let's take a closer look.

class ObjectPicker:
    """
    An object that stores other objects with weights, and is able to
    pick out one stored object, with a higher chance for higher weighted
    objects.
    """

Ok, that is a lot of text. Can we separate this out into two sections with a short title?

class ObjectPicker:
    """
    Choose a random element taking weights into account.

    Elements with a higher weight have a higher change of being chosen.
    """

On to the init.

    def __init__(self):
        """
        Initialize the Object Picker object. Create the instance variable
        bucket for storing objects.
        """
        ...

There are two things wrong with it. It is telling me the following:

It initialises the Object Picker object: That's what __init__ is supposed to do. Good. But that does not really warrant mentioning. So you can leave that portion out.
It creates an instance variable bucket for storing objects: That's an implementation detail. Where and when the bucket variable is initialized does not really matter. That it creates a variable bucket instead of storage or something else does not matter. Again, leave that out.

So we move to

    def __init__(self):
        ...

(That is, remove the doc string.)

add and pick can be given similar treatments. Importantly, add an extra empty line between the documentation, and the parameter description.

Comments

Look at the following code

if self.bucket:  # If there is anything in the bucket.
    total = self.bucket[-1][-1]  # Get the end number of the last item.
else:  # If the bucket is empty.
    total = 0  # End number is 0.

The comments for the if and else are really trivial. You can easily drop them. Please do.

Consider what your comments add to the code. If they detail how it's implemented, try very hard to find a way to remove them without reducing the clarity. The following is just as clear.

if self.bucket:
    total = self.bucket[-1][-1]  # Get the end number of the last item.
else:
    total = 0

You notice I left 1 comment in. That's because of the end number. If you follow the namedtuple advice below, you can replace that line with

total = self.bucket[-1].end

(without the comment). The code is just as clear.

Excessive commenting in general

Another comment I'd just like to point out. Somewhere in the code I see

return None  # Return None.

As an outsider, comments like this make me believe that you are a beginner in Python, or even programming in general. Probably even following a class where the lecturer dictates comments like that.

Commenting like this screams 'I know nothing'. However, when I read the code without the comments, it's very readable, and only a few comments remain that are actually necessary. Actually, just two:

# Get the end number of the last item.

and

# Start binary search in middle of storage object.

To remove them, you'd need to refactor your code. And later on I'll give you the ingredients needed in a bit more detail.

You might want to leave in some more comments, depending on how comfortable you are, but make sure that the comments add value. Commonly, that's done by explaining why a piece of code is written, not what it does.

Let me state the following a bit more clearly. It is the excessive commenting that makes me doubt your experience. Reading your code (and explanation) tells me a different story: You know how to handle a binary search (and from what I can see, correctly), you know something about complexity theory. This is somebody to be reckoned with.

Algorithm

Storing of elements.

You use a tuple to store the elements, leading to code such as

total = self.bucket[-1][-1]

and

start, end = storage[index][2:]

The tuple is always the same size, and the same form. So, why not make it a namedtuple? Put the following import at the top of your file:

from collections import namedtuple

RangedElement = namedtuple('RangedElement', ['value', 'length', 'start', 'end'])

(I cheated a bit, I moved from weights to lengths, because it somewhat makes more sense when talking about start and end.)

Then, instead of

self.bucket.append((item, weight, total, total + weight))

write

self.bucket.append(RangedElement(item, weight, total, total + weight))

Yes, it's more typing. But, instead of

total = storage[-1][-1]

you can now write

total = storage[-1].end

Which is much better.

Getting the (random) element.

Defaulting

You already mentioned you were not sure about the defaulting. Important should be the question 'why the defaulting'?

From what I can see, the only reason for the defaulting is the recursive algorithm you use, which is a recursive bisection algorithm (or binary search algorithm). It would be good to separate the bisecting from the choosing.

def _bisect(self, choice, storage):
    """
    Bisect to find the stored object.
    """
    # Bisection is currently done recursively

    # Start binary search in middle of storage object.
    index = len(storage) // 2
    start, end = storage[index][2:]

    # If the choice is lower, recursively search left half.
    if choice < start:
        return self._bisect(choice, storage[:index])
    # If the choice is higher, recursively search right half.
    elif choice > end:
        return self._bisect(choice, storage[index + 1:])
    # Otherwise, choice is in number spread and return object.
    else:
        return storage[index][0]

def pick(self):
    """
    Pick an object from the bucket recursively,
    taking weight into account.

    :param choice: Number of choice.
    :param storage: Storage Object to choose from.
    """
    if not self.bucket:  # If bucket is empty.
        return None  # Return None.
    else:
        storage = self.bucket  # Storage is bucket
        total = storage[-1][-1]  # Get final weight.

    # Randomly choose a number to represent the choice.
    choice = random.random() * total

    return self._bisect(choice, storage)

(Why I chose this: I saw that some arguments were recursion-specific, and were basically saying "Don't do this when you're already recursing". A clear pointer that we were mixing two concerns, which are now better separated).

Note that splitting like this can be somewhat dangerous when this method is called somewhere else as well with the extra parameters. Sufficient unit tests would detect it.

Bisecting

You probably saw I named the helper method _bisect. That is because it uses a fairly trivial bisection algorithm. The algorithm works recursively. Recursive algorithms are really nice, but Python has a recursion-limit, no tail-call optimisations, and you are also building subslices of bucket.

In fact, look at the following lines:

index = len(storage) // 2
....
    return self.pick(choice, storage[:index])
....
    return self.pick(choice, storage[index + 1:])
....

In the first call, you're copying exactly half the list. Slicing a list is linear in the length of the resulting slice. So this alone is O(n/2) = O(n). Due to recursion, you get O(n/2 + n/4 + n/8 + ...) (still O(n), though).

So your algorithm is actually linear due to copying. However, this is fixable. Instead of slicing storage, pass in offsets (hi/lo), calculate mid = lo + (hi - lo) // 2. Then recurse using hi = mid or lo = mid + 1.

However, instead of re-inventing the wheel, why not look at the Python bisect module?

https://hg.python.org/cpython/file/3.5/Lib/bisect.py

(I'm intentionally pointing at the source, because I'm going to copy the code, not call the module)

def _bisect(self, choice, storage):
   lo, hi = 0, len(storage)
    while True:
        mid = (lo + hi) // 2
        if storage[mid].end < choice:
            lo = mid + 1
        elif storage[mid].start > choice:
            hi = mid
        else:
            return storage[mid].value

Now I just hope I got my boundary conditions right (that's always a problem with a bisection algorithm). Just write plenty of unit tests for this specific part, please!

Choosing from an empty storage?

I'm specifically talking about the following lines:

if not self.bucket:  # If bucket is empty.
    return None  # Return None.

Is None also an allowed value in your buckets? If so, this could cause confusing bugs. Better:

if not self.bucket:
    raise EmptyBucketError("Can't pick from an empty bucket.")

or something similar. But this is something you need to decide yourself.

Conclusion

Great code, but it could use some (very) minor improvements with great benefits:

namedtuple
extract binary search from picking an element.
removing comments.


Combine lists from two lists of list into a single list of lists def combine_in_place(first, second):
    combined = []
    maxLength = max(len(first), len(second))
    for i in range(maxLength):
        if len(first) < i+1:
            combined.append(second[i])
        elif len(second) < i+1:
            combined.append(first[i])
        else:
            if not first[i]:
                combined.append(second[i])
            elif not second[i]:
                combined.append(first[i])
            else:
                combined.append(first[i] + second[i])

    return combined

print combine_in_place([['f'], ['j', 't'], [], ['s'], [], []], [['h', 'd'], [], [], []])
    # prints: [['f', 'h', 'd'], ['d', 't'], [], ['s'], [], []]

So I know to program (been programming in Java for some time now). I am trying to learn Python but I am not sure what's the best way to do some stuff. My question is:
How to write the function in a more Pythonic way?

I would write something like that:

def combine(first, second):
    ret_list = []
    for i, j in zip(first, second):
        add = []
        if i is not None:
            add.extend(i)
        if j is not None:
            add.extend(j)
        ret_list.append(add)
    return ret_list

I don't know if it is the most python-ish way but it is ok I think...

I mean you can also write something like this:

def combine(first, second):
    return [(i.extend(j) if not (i is None or j is None) else i if j is None else j) for i, j in zip(first, second)]

But you cannot read and debug that. So I would strongly recommend against it.

Btw: Did not test the last one so I hope I did not forget a closing bracket ;)

Edit:

In order to keep all elements you have to add something to the end. The zip() function stops iterating after the shortest element reached its end.
This is why the following method is needed instead of zip:

def combine(first, second):
    ret_list = []
    for i, j in itertools.zip_longest(first, second):
        add = []
        if i is not None:
            add.extend(i)
        if j is not None:
            add.extend(j)
        ret_list.append(add)
    return ret_list

Note: Itertools simply must be imported first. (For Python2 it is itertools.izip_longest()...)


Generate die rolls using coin flips Problem

I've seen this basic interview question in a couple of places, but never implemented it.

  Create an algorithm to output a fair die roll (values between 1 and 6, inclusive),
  given a function that outputs the outcome of a fair coin toss (0 or 1).

Approach

The idea here is to use three coin flips to generate the binary values from 000 to 111 (0 to 7 in Base 10).
Then, ignore the values 110 (6) and 111 (7) so that we are only considering the binary values from 000 to
101 (0 to 5 in Base 10).

Note that 110 and 111 will be the only values that start with two 1s, so any time the first two rolls start with
two 1s we know to ignore the previous values and try rolling again.

If the first two rolls do not start with two 1s, then we roll a third time, concatenate the three values to form our
binary representation and then convert the binary value into Base 10 (while adding one since the value will be 0-5
and we want 1-6).

Implementation Discussion

Most of my questions might be coming from the over-engineering part of my brain.

That part of my brain basically wants to create Enums for coin toss outcomes and for the die roll outcomes
(DieRoll.ONE, DieRoll.TWO, etc.).

Partly because it feels kind've weird that getCoinToss returns an int when it really represents a binary
(i.e. boolean) value.
Is it better to return a boolean here? I thought about it, but ultimately ended up not doing so because I would've
had to convert the booleans into 1s and 0s when generating the String binary representation.

Same thing (but to a lesser extent) with generateValue - the possible values are really 1-6.

Implementation

public class RandomDieValueGenerator {
    public static int generateValue() {
        int firstCoinToss = RandomDieValueGenerator.getCoinToss();
        int secondCoinToss = RandomDieValueGenerator.getCoinToss();

        if (firstCoinToss * secondCoinToss == 1) {
          return RandomDieValueGenerator.generateValue();
        }

        String binaryRepresentation = String.valueOf(firstCoinToss) + secondCoinToss + RandomDieValueGenerator.getCoinToss();
        return Integer.parseInt(binaryRepresentation, 2) + 1;
    }

    private static int getCoinToss() {
        return Math.toIntExact(Math.round(Math.random()));
    }
}

I think your solution still has some potential but you don't need an Enum for the #getCoinToss method because you don't have any advantage over using an Integer. Not using simple integers might even lead to your code getting more complex than it should, as you would have to implement some kind of way to convert the Integer to an Enum constant. This would be possible could affect the time complexity of your code even tho it would not be heavily noticable.

Binary

For handling the binary values I would not use an integer but instead a byte for obvious reasons:

The values of the two integral types are integers in the following ranges:

For byte, from -128 to 127
For int from -2147483648 to 2147483647

As you only need zero and one for representing binary data I would suggest using the "smaller" data type.

#generateValue Return Value

For this method, I would stick with an Integer as this method is public and might be used by other methods later on and an int is the most common data type. If you use a byte as the return value here you might have to cast the return value of this method when using it because most other methods don't use bytes as their arithmetic differs.

Still, you could use an Enum here which would have the advantage of the user not being able to create for example if-Statements that can't be true:

if (RandomDieValueGenerator.generateValue() == 7)

This problem would be solved if you provide an Enum for this return value because then, the user could only check if the return value of this method is one of the six defined possible enum constants.

Putting it all together

public class RandomDieValueGenerator {

    enum DieValue {
        ONE, TWO, THREE, FOUR, FIVE, SIX
    }

    public static DieValue generateValue() {
        int firstCoinToss = RandomDieValueGenerator.getCoinToss();
        int secondCoinToss = RandomDieValueGenerator.getCoinToss();

        if (firstCoinToss * secondCoinToss == 1) {
            return RandomDieValueGenerator.generateValue();
        }

        String binaryRepresentation = String.valueOf(firstCoinToss) + secondCoinToss + RandomDieValueGenerator.getCoinToss();
        return DieValue.values()[Integer.parseInt(binaryRepresentation, 2)];
    }

    private static byte getCoinToss() {
        return (byte) Math.toIntExact(Math.round(Math.random()));
    }
}


FASM assembly: Write a program which checks if a number is a prime number Coming from high-level languages like PHP and JavaScript, I'm doing an Assembly course on Udemy.

Yesterday I tried to implement the following task:

  Write a program that takes a number m as input, and prints back 1 to the
  console if m is a prime number. Otherwise, it prints 0.

Original text can be found here (Exercise 3.0).

format PE console
entry start

include 'win32a.inc'

; ===============================================
section '.text' code readable executable

start:
    call read_hex
    mov esi, eax ; Store the provided number in esi.
    mov edi, 1

    cmp eax, 2
    je finish

    mov ebx, 2 ; ebx stores the divisor.
    mov edx, 0 ; Will contain the remainder. Therefore: Make sure it's empty.
go_forward:
    ; Divisor has reached the size of the number and it was still
    ; not possible to divide the number without a rest : Got a prim number.
    cmp ebx, esi
    je finish ; Jump out of the loop.

    div ebx

    cmp edx, 0
    je no_prim

    ; Preparations for the next iteration.
    inc ebx ; Increase the divisor.
    mov edx, 0
    mov eax, esi ; Restore the user entered value.

    jmp go_forward ; Restart the loop.
no_prim:
    mov edi, 0 ; No prim : Then set the result to 0 !
finish:
    mov eax, edi
    call print_eax ; Provided by teacher: Print content eax to stdout.

    push    0
    call    [ExitProcess]

include 'training.inc'

I have tried to comment a lot. I hope that makes my thoughts understandable. I'm still very much a beginner and the program could (without doubt) be improved.

Overall your code is very reasonable for somebody learning Assembly, I would say quite nice work.

I can produce something what will look more advanced (I hope), but it's more like tiny details improved here and there, and some of them for the price of worse readability of source.

I mean on the syntax and low level. On the algorithmic level your code can be improved considerably, as Paweł already answered, but he missed another opportunity to halve number of divisions.

My variant of code, showing:

how to use the result of DIV ebx (which has to be done anyway to test remainder) as approximation of suggested sqrt(input) test, for earlier exit.
xor reg,reg used to set zero into register (shorter opcode than mov reg,0 plus modern CPU recognizes it as idiom of "set to zero" and optimizes for it)
test edx,edx to check if remainder is zero (shorter opcode than cmp edx,0 plus modern CPU will recognize it as idiom of "test for zero value" and optimizes for it)
resolving even numbers at the beginning (even numbers have zero in least significant bit: isOdd(n) == (n&1). And the only even number being prime is 2, no need to test other even numbers by DIV.
but that means only odd divisors have to be tested in the loop, because (odd_input % even_divisor) != 0 for sure. So my loop is testing divisors 3, 5, 7, 9, 11, 13, 15, ...
some non-trivial usage of arithmetic instructions and flags to check for input values [0, 1, 2, 3] ahead of loop, as those would break the loop logic and produce invalid result. It may be nice exercise for you to "decipher" how it works.
you should try to keep things "together" (if possible). For example you set up edx:eax for DIV at the end of your loop code block, so it's A) not together with DIV, interleaved by JMP (hard to read for human, no problem for CPU) B) you have code duplicity (mov edx,0), because you have to init the register values ahead of very first DIV separately. => So I do initialize edx:eax right ahead of DIV, only single time in code, and it's easier to read IMO.

format PE console
entry start

include 'win32a.inc'

; ===============================================
section '.text' code readable executable

start:
    call    read_hex
    xor     ecx, ecx    ; return value = 0 (0 = not prime, 1 = prime)
    mov     esi, eax    ; copy provided number in esi
    shr     eax, 1      ; eax = eax div 2, remainder into CF
    jz      finish      ; 0 and 1 are not primes
    adc     ecx,ecx     ; temporarily ecx=1/0 for odd/even input
    dec     eax
    jz      input_was_prime ; 2 and 3 are primes, special test needed
                            ; because they would break loop logic
    jecxz   finish      ; other even numbers (but not 2) are *not* primes

    ; input number was odd and above 3, test odd divisors in generic loop
    mov     ebx,ecx     ; start with divisor 3 (ebx=1, loop will +2 first)
    dec     ecx         ; result = 0 (not prime)
test_divisors_loop:
    add     ebx,2       ; next odd divisor to test
    mov     eax,esi
    xor     edx,edx     ; edx:eax = input number (zero extended to 64b)
    div     ebx         ; eax = quotient, edx = remainder
    test    edx,edx
    jz      finish      ; no prime, when remainder is zero
    cmp     eax,ebx
    ja      test_divisors_loop  ; quotient > divisor -> try next
    ; when quotient <= divisor, it's safe to end loop, esi is prime number
    ; it's basically cheap "sqrt" end condition, reusing DIV result
input_was_prime:
    mov     cl,1        ; result = 1 (is prime) (ecx was 0 or 1 before)
finish:
    mov     eax, ecx    ; result was in ecx, print it
    call    print_eax

    push    0
    call    [ExitProcess]

include 'training.inc'

(I did debug it in NASM, so if I'm unlucky, something may fail in FASM due to syntax, but it should be easy to fix. But it's highly unlikely, they should have identical syntax for instructions and I don't use any macros/etc)


Update database, reload page then show a success message I have spent a long time trying to find a way to do the following:

Update a MySQL database with AJAX
Reload the Page to show changes
Show a success message

I think I have finally found a way!  Let me know of any improvements.

The method:

Make your Ajax call and reload on success
Set a session in your php code with a variable
Check the session is set and output the necessary message
Empty the session

The in-page AJAX call:

$(document).on("click",".active_button",function() {

    var articleID = $(this).attr("name");
    var active = $(this).val();

    $.ajax({
        type: "POST",
        url: "controllers/articlecontrol.php",
        data: { articleID: articleID, active:active },
        success: function(data){
            document.location.reload(true);
        }
    });
});

The PHP (articlecontrol.php):

// ACTIVATE / DEACTIVATE w/Button
if(isset($_POST['active'])) {
    $active = $_POST['active'];
    $articleID = $_POST['articleID'];
    if($active == 'YES') {
        $update = array('article_active' => '0');
        $where = array('article_id' => $articleID);
        $sql = $database->update('wcx_articles', $update, $where);
        if($sql) {
            $_SESSION['status'] = 'deactivated';
        }
    }
    if($active == 'NO') {
        $update = array('article_active' => '1');
        $where = array('article_id' => $articleID);
        $sql = $database->update('wcx_articles', $update, $where);
        if($sql) {
            $_SESSION['status'] = 'activated';
        }
    }
}

The success message (in same page as AJAX):

if(isset($_SESSION['status'])) {
    if($_SESSION['status'] == 'activated') {
        echo '
Activated Successfully
';
        $_SESSION['status'] = '';
    }
    if($_SESSION['status'] == 'deactivated') {
        echo '
Deactivated Successfully
';
        $_SESSION['status'] = '';
    }
}

What you're doing is extremely typical. So typical that a language was made specifically for it: Ajax!
I'll make some comments here, starting from the top.
Your method
It's close, but it can be simplified even more. For reference, here's your method:

Make your Ajax call and reload on success
Set a session in your php code with a variable
Check the session is set and output the necessary message
Empty the session

and then I'll make my own so we can see the differences.

Make an Ajax call
Update the database and return a response
Output the respective message

A quick re-write
I thought that rewriting this would be the best way to show how things could be done differently.
Ajax
$(".active_button").on("click", function() {

    var articleID = $(this).attr("name");
    var active = $(this).val();
    $.post("controllers/articlecontrol.php", {articleID: articleID, active: active}, function(data) {
        var response;
        var class = "success";
        if (data === "deactivated") {
            response = "Deactivated Successfully";
        } else if (data === "activated") {
            response = "Activated Successfully";
        } else {
            response = "Whoops!";
            class = "failure";
        }
        $("Some DOM item").before('<div>' + response + '</div>').addClass(class);
    });
});

articlecontrol.php
if (isset($_POST['active'])) {
    $where = array('article_id' => $_POST['articleID']);

    $active = ($_POST['active'] == 'YES') ? '0' : '1';

    $update = array('article_active' => $active);

    if ($database->update('wcx_articles', $update, $where)) {
        if ($active == '0') {
            echo "deactivated";
        } else {
            echo "activated";
        }
    }
}

Inline PHP
Gone! Not even needed anymore!
Changes made:
A lot, right? Well it's okay, I'll try and explain why I modified your code like this.
First off, I placed the button handler directly onto the class and avoided dealing with the document call. It cleared things up a little!
I changed your $.ajax call into a $.post call. For your implementation here, I found it to fit real nice. It's the exact same functionality, just in a way I believe to be more clear and readable.
Inside of this Ajax call, I added a lot of business logic. It now accepts data from the PHP page and then decides how to display the action. The error handler on this is very primitive and could be improved. Note that you must define where in the page the messages will pop up. It appears you have the knowledge to know what I've done here, however if you need me to reference anything or clarify, just ask!
Look at that! Done with the jQuery already!
The biggest change I made with your PHP was taking out the need for sessions. Everything else is simply topical and hopefully more clear than the original code. Let me know if you don't understand anything. Do note however, I have echoed the response that the Ajax will receive. If you needed, you could also format it in JSON, more on that here.
And since we cleared out the sessions, the inline PHP is gone!
I don't normally just rewrite people's code, but I felt this could have used multiple alterations which wouldn't make sense if I din't have complete context!


Speed problems with SetValue, ToType and Reflection I have a 'speed problem' with a program I'm currently developing. I already have encountered the methods which costs some time.

public void ConvertDataFrameToValueFields()
    {
        List<byte[]> items = new List<byte[]>() { new byte[2], new byte[2], new byte[2], new byte[2] };

        items[0][0] = this.DataBytes[0];
        items[0][1] = this.DataBytes[1];
        items[1][0] = this.DataBytes[2];
        items[1][1] = this.DataBytes[3];
        items[2][0] = this.DataBytes[4];
        items[2][1] = this.DataBytes[5];
        items[3][0] = this.DataBytes[6];
        items[3][1] = this.DataBytes[7];

        int i = 0;
        IEnumerable<PropertyInfo> result = this.determineCanMessageValueFields();

        foreach (var prop in result)
        {
            if (prop.PropertyType == typeof(MessageStatusValueField))
            {
                prop.SetValue(this, new MessageStatusValueField(BitConverter.ToUInt16(items[i], 0), prop.Name, this.UnitType), null);
            }
            else if (prop.PropertyType == typeof(MessageValueField))
            {
                prop.SetValue(this, new MessageValueField(BitConverter.ToUInt16(items[i], 0), prop.Name), null);
            }
            else if (prop.PropertyType == typeof(MessageValueFieldInt32))
            {
                prop.SetValue(this, new MessageValueFieldInt32(BitConverter.ToUInt16(items[i], 0), prop.Name), null);
            }
            else if (prop.PropertyType.GetCustomAttributes(false).Count(x => x.GetType() == typeof(BitmaskValueFieldClass)) > 0)
            {
                var tempField = new MessageValueField(BitConverter.ToUInt16(items[i], 0), prop.Name);
                prop.SetValue(this, Convert.ChangeType(tempField, prop.PropertyType), null);
            }
            i++;
        }
    }

     private IEnumerable<PropertyInfo> determineCanMessageValueFields()
            {
                //Get all properties with the
                //custom attribute [IsValueField]
                return this.GetType().GetProperties()
                                     .Where(p => p.GetCustomAttributes(false)
                                     .Count(x => x.GetType() == typeof(IsValueField)) > 0)
                                     .OrderBy(x => ((IsValueField)x.GetCustomAttributes(false)[0]).ValueFieldIndex);
            }

 public object ToType(Type conversionType, IFormatProvider provider)
        {
            //...only convert if the conversion type is a BitmaskValueFieldClass
            if (conversionType.GetCustomAttributes(false).Count(x => x.GetType() == typeof(BitmaskValueFieldClass)) == 0)
            {
                throw new NotImplementedException("The conversion is only available for conversion types with the BitmaskValueFieldClass attribute");
            }

            //...create an instance of the conversion type
            var instance = Activator.CreateInstance(conversionType);

            //...go thru each property and set the value
            foreach (var item in instance.GetType().BaseType.GetProperties())
            {
                item.SetValue(instance, this.GetType().GetProperty(item.Name).GetValue(this, null), null);
            }

            return instance;
        }

The method ConvertDataFrameToValueFields() is the sticking point. I know it is because of the prop.SetValue() and the ToType() IConvertible method calls. When I am removing the ConvertDataFrameToValueFields() method I gain the missing time I need to fulfill the project.

Is there a way to improve the whole thing? Problem is that the idea has to be as it is. In the ConvertDataFrameToValueFields() method I have to map the returning DataFrame, the DataBytes array, to the MessageValueField properties of the specific class. I'm getting all containing messagevaluefields by calling the ConvertDataFrameToValueFields().

Style

Based on the naming guidlines method names should use PascalCasing casing.

determineCanMessageValueFields()

Instead of calling Count() and checking if it is greater than 0 you should call Any().

So determineCanMessageValueFields() would become

private IEnumerable<PropertyInfo> determineCanMessageValueFields()
{
    return this.GetType().GetProperties()
                            .Where
                                (p => p.GetCustomAttributes(false)
                                            .Any(x => x.GetType() == typeof(IsValueField))
                                )
                            .OrderBy(x => ((IsValueField)x.GetCustomAttributes(false)[0]).ValueFieldIndex);
}

ConvertDataFrameToValueFields()

The creation of the List<byte[]> is not needed at all. If you increment the i counter by 2 you can just use this.DataBytes for the calling BitConverter.ToUInt16(this.DataBytes,i).

This leads after using Any()instead of Count() > 0 to

public void ConvertDataFrameToValueFields()
{

    int i = 0;

    foreach (var prop in DetermineCanMessageValueFields())
    {
        ushort value = BitConverter.ToUInt16(this.DataBytes, i);

        if (prop.PropertyType == typeof(MessageStatusValueField))
        {
            prop.SetValue(this, new MessageStatusValueField(value, prop.Name, this.UnitType), null);
        }
        else if (prop.PropertyType == typeof(MessageValueField))
        {
            prop.SetValue(this, new MessageValueField(value, prop.Name), null);
        }
        else if (prop.PropertyType == typeof(MessageValueFieldInt32))
        {
            prop.SetValue(this, new MessageValueFieldInt32(value, prop.Name), null);
        }
        else if (prop.PropertyType.GetCustomAttributes(false).Any(x => x.GetType() == typeof(BitmaskValueFieldClass)))
        {
            var tempField = new MessageValueField(value, prop.Name);
            prop.SetValue(this, Convert.ChangeType(tempField, prop.PropertyType), null);
        }

        i += 2;
    }
}

and applying the Any() change to the ToType() method

public object ToType(Type conversionType, IFormatProvider provider)
{

    if (!conversionType.GetCustomAttributes(false).Any(x => x.GetType() == typeof(BitmaskValueFieldClass)))
    {
        throw new NotImplementedException("The conversion is only available for conversion types with the BitmaskValueFieldClass attribute");
    }

    var instance = Activator.CreateInstance(conversionType);

    foreach (var item in instance.GetType().BaseType.GetProperties())
    {
        item.SetValue(instance, this.GetType().GetProperty(item.Name).GetValue(this, null), null);
    }

    return instance;
}

Comments

Comments should be used to explain why something is done, not what is done. The what part should be done by the code itself.


config.properties, Static Classes and Reflection I wanted an opinion about an idea I had regarding the use of config.properties, static classes and the use of reflection.

I have to load some preferences at application startup, for that I will be using config.properties. Certain properties within config.properties will be changed during various points in the program. So, instead of opening, searching for the property, updating and closing properties.config - I was thinking I could use a static class to read and write values, use reflection to write values back and forth between static class and properties.config file during application startup and close.

Is there a better approach I should be considering? Should I be avoiding static class?

Here is simplified code

//Class with static variables, the values get used and updated at
//various points within the program. For example prop1 gets updated and used
//at a different points in the program. Also, they are active at
//different  points in the program than prop1 and prop2.
//values need to be saved for application startup.

public class StaticProp {
    public static String prop1 = "a";
    public static String prop2 = "b";
    public static String prop3 = "c";
}

During application close, using Reflection - write values to properties.config

    private void writeConfigProp() {
    Properties prop = new Properties();
    OutputStream outputStream = null;
    try {
        outputStream = new FileOutputStream("resources/config.properties");
        Field[] fields = StaticProp.class.getDeclaredFields();
        for (Field field : fields) {
            try {
                prop.setProperty(field.getName(), Utility.tryParseVal(field.get(null)));
            } catch (IllegalArgumentException e) {
                e.printStackTrace();
            } catch (IllegalAccessException e) {
                e.printStackTrace();
            }
        }
        prop.store(outputStream, "Config for UI");
    } catch (IOException io) {
        io.printStackTrace();
    } finally {
        if (outputStream != null) {
            try {
                outputStream.close();
            } catch (IOException e) {
                e.printStackTrace();
            }
        }
    }
}

During application startup, using Reflection - read properties.config to static class.

    /**
 * Read Config Properties
 */
private void readConfigProp() {
    Properties prop = new Properties();
    InputStream inputStream = null;

    try {
        inputStream = new FileInputStream("resources/config.properties");
        prop.load(inputStream);

        Field[] fields = PropConfig.class.getDeclaredFields();
        for (Field field : fields) {
            try {
                try {
                    field.set(null, prop.getProperty(field.getName()));
                } catch (IllegalAccessException e) {
                    e.printStackTrace();
                }
            } catch (IllegalArgumentException e) {
                e.printStackTrace();
            }
        }
    } catch (IOException ex) {
        ex.printStackTrace();
    } finally {
        if (inputStream != null) {
            try {
                inputStream.close();
            } catch (IOException e) {
                e.printStackTrace();
            }
        }
    }
}

Do you have an actual, measured performance issue with accessing the properties file every time? If not, you're just introducing complexity to no end. What happens if the application crashes? All your updated properties are gone.

If you do have an actual, measured performance issue, consider keeping an in-memory copy of the properties. When one changes, update the in-memory version and spawn a low-priority thread to handle the write to the file. If you do, then you have to worry about the order those threads run in.

Definitely don't use Strings. If you're going to enumerate all your properties, use an enum. If you do, you shouldn't need reflection .. just walk down the values() of the enum.


Navigation item selection Using Bootstrap 3 and PHP, I create my navigation bars in the following way:

<?php
$page_url = $_SERVER['REQUEST_URI'];
$active1 = (strpos($page_url, "one.php") !== false ? " class=\"active\"" : "");
$active2 = (strpos($page_url, "two.php") !== false ? " class=\"active\"" : "");
$active3 = (strpos($page_url, "three.php") !== false ? " class=\"active\"" : "");
$active4 = (strpos($page_url, "four.php") !== false ? " class=\"active\"" : "");
?>

      <button class="navbar-toggle" data-toggle="collapse" data-target=".navbar-collapse">

      </button>
      Company Name

        <li<?=$active1?>>Link One

        <li<?=$active2?>>Link Two

        <li<?=$active3?>>Link Three

        <li<?=$active4?>>Link Four

Then, depending on which page you're on, a link within the navigation has class="active" applied to it. However, this feels like a very messy way of building this functionality to the <navbar>. Lots of empty variables at one time, and a lot of almost identical code for each navigation element.

How best can I improve on this code?

Use an array and a function

You've got quite a few pieces of code that look very similar. The more menu items you have, the worse this gets. This is almost always a sign you should start using a function. Here's how to do that:

<?php

  $menuItems = array('Link One'   => 'one.php',
                     'Link Two'   => 'two.php',
                     'Link Three' => 'three.php',
                     'Link Four'  => 'four.php');

  function navigationItem($title,$page)
  {
    $class = (strpos($_SERVER['REQUEST_URI'],$page) !== false) ? ' class="active"' : '';
    return '
'.$title.'
'.PHP_EOL;
  }

?>

      <button class="navbar-toggle" data-toggle="collapse" data-target=".navbar-collapse">

      </button>
      Company Name

<?php

  foreach ($menuItems as $title => $page) echo navigationItem($title,$page);

?>

As you can see I've put both lines, which you repeat four times, inside the function and simply call that function four time. I could have done that by simply writing:

<?php
  echo navigationItem('Link One','one.php');
  echo navigationItem('Link Two','two.php');
  echo navigationItem('Link Three','three.php');
  echo navigationItem('Link Four','four.php');
?>

but I prefer to have the menu at the top of the file in an array. This makes it easier to find and edit.

This was probably the next step you were looking for. But it should not be your last.

You could easily generalize the navigationItem() function for all list items you will make. And the foreach loop basically builds a list, why not make a function out of that?

After that you could make a class which could build a whole menu bar for you. The functions will become methods of that class.

Why not put the array in a seperate file with the other stuff that defines your site? Perhaps eventually create a database table with menu items?

There are many ways in which you can improve this tiny piece of code.

What you should definately NOT do is: Build a whole site with Bootstrap HTML code interspersed by piece of loose PHP code. That will be chaos, and very difficult to maintain and expand. Be systematic about it.

Preferably try to avoid putting piece of PHP code inside HTML pages. It's just so ugly. Personally I prefer to write in pure PHP, and generate the HTML using objects similar to the class I suggested for your menu bar.


'do { statement; } while(0)' against 'statement' when writing C macro? Which one of the following is preferrable and why when writing C macro?

Type 1

#define I2C_START()                 I2C_WAIT_IDLE(); SSP1CON2bits.SEN  = 1

Type 2

#define I2C_START()               \
    do                            \
    {                             \
        I2C_WAIT_IDLE();          \
        SSP1CON2bits.SEN  = 1;    \
    }while(0)

One of my colleagues prefers Type 1 while I prefer Type 2. So when both of us work in a project, both of the types are seen throughout the project.

I prefer the second one because it can be used with normal compound stamens without modifications or accidentally being used incorrectly.
if (condition)
    I2c_START;

Type 1: FAIL
Type 2: OK
Of course with good coding standards that can be avoided (always use '{' '}' on if). But every now and then people can be lazy (or just careless) and the second version prevents accidents from happening.
The best solution is not to use macros. Use functions and let the compiler work out when to inline.
Edited:
For billy: (Type 3) no do while
#define I2C_START()   \
{                             \
    I2C_WAIT_IDLE();          \
    SSP1CON2bits.SEN  = 1;    \
}

Unfortunately this fails if you use the else part of the if statement:
The trailing semi-colon (on type 3) marks the end of the if statement thus else is not syntactically allowed. At least this gives you a compilation error unlike above. But Type 2 still works as expected.
if (condition)
    I2C_START();
else
    std::cout << "FAIL this test\n";

Type 1: FAIL
Type 2: OK
Type 3: FAIL


Log and Count the exception in a Single method I am tring to count the number of exceptions happening and log those exceptions as well. So what I did is, I created one method addException in which I am counting all the exceptions.

addException method accepts two parameters, one is the String, and other is the boolean flag which means whether we want to terminate the program or not because of any exceptions. Meaning, if that flag is true, then I need to terminate the program whenever there are any exceptions.

So if you take a look into my below catch block, I have addException method call for counting the exceptions and below that method call I am logging the exceptions as well.

catch (ClassNotFoundException e) {
    addException(e.getCause() != null ? e.getCause().toString() : e.toString(), Read.flagTerminate);
    LOG.error("Threw a ClassNotFoundException in " + getClass().getSimpleName(), e);
} catch (SQLException e) {
    addException(e.getCause() != null ? e.getCause().toString() : e.toString(), Read.flagTerminate);
    //DAMN! I'm not....
    LOG.error("Threw a SQLException while making connection to database in " + getClass().getSimpleName(), e);
}

/**
 * A simple method that will add the count of exceptions and name of
 * exception to a map
 *
 * @param cause
 * @param flagTerminate
 */
private static void addException(String cause, boolean flagTerminate) {
    AtomicInteger count = exceptionMap.get(cause);
    if (count == null) {
        count = new AtomicInteger();
        AtomicInteger curCount = exceptionMap.putIfAbsent(cause, count);
        if (curCount != null) {
            count = curCount;
        }
    }
    count.incrementAndGet();

    if(flagTerminate) {
        System.exit(1);
    }
}

Problem Statement:-

Now what I am looking for is-

Is there any more cleaner way of doing the same thing? Meaning right now I am counting the exceptions in a method and then printing out the exceptions in the next line inside the catch block.

Is it possible to do the both of the things in the same addException method? And if the flag is true to terminate the program, then terminate the program with the proper logging as well.

What could be the best way to re write addException method do this?

Is there any more cleaner way of doing the same thing? Meaning right now I am counting the exceptions in a method and then printing out the exceptions in the next line inside the catch block.
  Is it possible to do the both of the things in the same addException method?

It depends. You could configure the logger and add a new a appender. Then your logger serves two appenders: The (probably) println and the counting.
Otherwise, you could add the log call inside the addException method (could be renamed to handleException(...) then).

  And if the flag is true to terminate the program, then terminate the program with the proper logging as well.

I highly suggest to avoid boolean flag arguments. If you have a public api, this suggestion is a must. Look at java for a lot of bad examples.
Use two methods:

private static void addException(String cause) {
  ...
}

private static void addExceptionAndTerminate(String cause) {
  addException(...);
  System.exit(1);
}

(And I do not see the point of adding the exception if you terminate anyway, but it could be for some multithreading things.)

  What could be the best way to re write addException method do this?

I would change the method signature from String, Boolean to Exception and put all exceptions inside a synchronized list.
Only if I want to get some statistics, I would start to parse them. Which makes the live easier for everyone who is using this.
At the moment, the functionality depends on the format of the string. If you or someone else, does not use the correct format, it does not work.
therefore, you sould document this at least in the javadoc. And the correct formatting is a pain, noone will like to write like this.
And I am not sure if the implementation for concurrent map access is correct.

If you just allow an exception object, noone has to care about formatting, the synchronization is a lot easier (just add to the list) and you have all the freedom in some other method how to count, parse
and do whatever you want with this exceptions.


Extract and sort all numbers that appear after ', I am currently learning Python and I wrote the following code.
The code searches the string for all digits that appear after ',, sorts them and joins them into a string.
I have a strong feeling that it can be shortened...
Can anyone suggest how?

s = "'bhhd',12 'kjubk',2 'bjki',98 'khjbjj',4"
res = re.findall(r"(',)(\d+)", s)
all = []
for r in res:
   all.append(r[1])

print(",".join(sorted(all, key=int)))

re.findall() behaves differently depending on whether the regex contains capturing parentheses.  If it doesn't contain capturing parentheses, then it just returns the matching text as a flat list.

So, how can you avoid capturing?  Just rewrite your regex to use a positive lookbehind assertion.

s = "'bhhd',12 'kjubk',2 'bjki',98 'khjbjj',4"
all = re.findall(r"(?<=',)\d+", s)
print(",".join(sorted(all, key=int)))


Unique random number generation in Swift In the process of learning Swift, I'm building a lotto numbers generator that generates a random number and appends it into an array, if the number already exists then it should re-generate a random numbers and append that one to the array.

I've come up with this. I want to be able to have a reusable class where I can decide how many numbers I want it to generate.

How would you guys solve this in a simpler way?

class EuroMillions {

    var randomNumber:Int = 0
    var numberArray = [Int]()

    func generateRandomNumber (numbers: Int, repetitions: Int) -> (Int, Int, Int) {

    for number in 1...6 {
        randomNumber = Int(arc4random_uniform(UInt32(numbers)))
        checkArray(numbers: numbers)
    }
        return (randomNumber, numbers, repetitions)

    }

func checkArray(numbers: Int) {
if numberArray.contains(randomNumber){
    //re-generate
    print("Number \(randomNumber) exists!")
    randomNumber = Int(arc4random_uniform(UInt32(numbers)))
    checkArray(numbers: numbers)

        print("Replacing it with \(randomNumber)")
    } else {
        numberArray.append(randomNumber)
    }
  }
 }
}

let firstNumbers = EuroMillions()
firstNumbers.generateRandomNumber(numbers: 50, repetitions: 6)
let finalNumbers = firstNumbers.numberArray
print(finalNumbers)

Your code made me think of a few improvements I could made: the data workflow is not easy to follow and this complexity is not necessary for this task, and this is what will change in my version. Other than that, I'm just proposing general improvements.

Let's start by listing what we need:

a dedicated class
generate x times a random number
each random number must be unique
each random number has a max value

For this, I start by making a class template:

class EuroMillions {

    var numbers: [Int] = []

    func generateNumbers(repetitions: Int, maxValue: Int) -> [Int] {
        // here, generate a random number x times
        // then populate the 'numbers' array and return it
        return numbers
    }

}

We have what we need: parameters for how many random numbers to generate and a maximum value.

Now how do we populate the array?

First we need to ensure that the maxValue is more or equal the number of repetitions, otherwise the task is not achievable with unique random numbers:

func generateNumbers(repetitions: Int, maxValue: Int) -> [Int] {

    guard maxValue >= repetitions else {
        fatalError("maxValue must be >= repetitions")
    }

    // work to do here

    return numbers
}

We also need to actually generate a random number:

private func random(maxValue: Int) -> Int {
    return Int(arc4random_uniform(UInt32(maxValue + 1)))
}

This will generate a number between 0 and maxValue, the latter included. If you don't want the maxValue being inclusive, remove the + 1.

We also need to check if the generated number is not already in our array.

For this, I've chosen to use a repeat while loop inside the counting loop and to use contains for checking the array:

for _ in 1...repetitions {
    var n: Int
    repeat {
        n = random(maxValue: maxValue)
    } while numbers.contains(n)
    numbers.append(n)
}

We loop x number of times (number of iterations = number of numbers to get). Since we don't need the index we discard it with _.

We prepare an Int, and populate this Int with a random value in a while loop.

The repeat loop generates a number then checks, in the while, if this value is already in the array with .contains.

If the number is not in the array, we break out of the inner loop and add it. If already present we continue generating until we have one that isn't in the array.

Another security check is to set the array to empty before generating, just in case the user would use our method several consecutive times (we could also avoid that by returning the result directly instead of populating an array property, and we'll see that later).

Complete code:

class EuroMillions {

    var numbers: [Int] = []

    func generateNumbers(repetitions: Int, maxValue: Int) -> [Int] {
        guard maxValue >= repetitions else {
            fatalError("maxValue must be >= repetitions for the numbers to be unique")
        }

        numbers = []

        for _ in 1...repetitions {
            var n: Int
            repeat {
                n = random(maxValue: maxValue)
            } while numbers.contains(n)
            numbers.append(n)
        }

        return numbers
    }

    private func random(maxValue: Int) -> Int {
        return Int(arc4random_uniform(UInt32(maxValue + 1)))
    }

}

Usage:

let lottery = EuroMillions()
let numbers = lottery.generateNumbers(repetitions: 6, maxValue: 49)
print(numbers)

Possible result:

  [7, 12, 0, 9, 24, 35]

This was my main answer to your question.

How could we improve this class a bit more, or have fun, or both?

I would add the possibility to choose different lotteries, for example, and have defaults for them (methods prepared with preset values).

We also should set access privileges to methods and properties.

And I also like to have a printable identifier for the class.

For the identifier I like to use CustomStringConvertible which makes the class printable by adding a description:

class EuroMillions: CustomStringConvertible {

    // ...

    public var description: String {
        // return something relevant here
    }

}

And for the different lottery modes I would use an enum:

public enum LotteryMode: String {
    case euroMillions = "Euro Millions"
    case lotto = "Lotto"
}

public class Lottery {

    private var mode: LotteryMode

    public init(mode: LotteryMode = .euroMillions) {
        self.mode = mode
    }

    public func generate() -> [Int] {
        switch mode {
        case .euroMillions:
            return generateNumbers(repetitions: 5, maxValue: 49)
        case .lotto:
            return generateNumbers(repetitions: 6, maxValue: 49)
        }
    }

    // ...

}

Now we make generateNumbers(repetitions:, maxValue:) private because in this version of the class we only use the new preset method generate(), but this is not mandatory, it's just an example.

I also make the array private so that it can't be tempered with from outside, and add a public computed property to get its contents again once generated (since the class keeps state).

Complete code:

public enum LotteryMode: String {
    case euroMillions = "Euro Millions"
    case lotto = "Lotto"
}

public class Lottery: CustomStringConvertible {

    private var numbers: [Int] = []

    private var mode: LotteryMode

    public init(mode: LotteryMode = .euroMillions) {
        self.mode = mode
    }

    public var results: [Int] {
        return numbers
    }

    public func generate() -> [Int] {
        switch mode {
        case .euroMillions:
            return generateNumbers(repetitions: 5, maxValue: 49)
        case .lotto:
            return generateNumbers(repetitions: 6, maxValue: 49)
        }
    }

    private func generateNumbers(repetitions: Int, maxValue: Int) -> [Int] {
        guard maxValue >= repetitions else {
            fatalError("maxValue must be >= repetitions")
        }

        numbers = []

        for _ in 1...repetitions {
            var n: Int
            repeat {
                n = random(maxValue: maxValue)
            } while numbers.contains(n)
            numbers.append(n)
        }

        return numbers
    }

    private func random(maxValue: Int) -> Int {
        return Int(arc4random_uniform(UInt32(maxValue + 1)))
    }

    public var description: String {
        if numbers.isEmpty {
            return "\(mode.rawValue) - No result"
        } else {
            let content = numbers.map { String($0) }.joined(separator: ", ")
            return "\(mode.rawValue) - Result: \(content)"
        }
    }

}

Usage examples:

let euros = Lottery() // or Lottery(mode: .euroMillions)
print(euros)
let eurosNumbers = euros.generate()
print(euros)
print(eurosNumbers)
print(euros.results)

let lotto = Lottery(mode: .lotto)
print(lotto)
let lottoNumbers = lotto.generate()
print(lotto)
print(lottoNumbers)
print(lotto.results)


Refactoring Android fragments I have an Android app that shows some fragments, but I'm repeating lots of code.

MainActivity.java

public class MainActivity extends FragmentActivity {
    MyPagerAdapter pageAdapter;
    ViewPager pager;
    @Override

    protected void onCreate(Bundle savedInstanceState) {
        super.onCreate(savedInstanceState);
        setContentView(R.layout.activity_main);

        pageAdapter = new MyPagerAdapter(getSupportFragmentManager());
        pager = (ViewPager)findViewById(R.id.myViewPager);
        pager.setAdapter(pageAdapter);
    }

    public void homeButtonPressed(View v) {
        // go home
        pager.setCurrentItem(0);
    }
}

MyPagerAdapter.java

public class MyPagerAdapter extends FragmentPagerAdapter {
    public List<Fragment> fragments;

    public MyPagerAdapter(FragmentManager fm) {
        super(fm);
        this.fragments = new ArrayList<Fragment>();
        fragments.add(new Page1());
        fragments.add(new Page2());
        fragments.add(new Page3());
        fragments.add(new Page4());
        fragments.add(new Page5());
        fragments.add(new Page6());
        fragments.add(new Page7());
        fragments.add(new Page8());
    }

    @Override
    public Fragment getItem(int position) {
        return fragments.get(position);
    }

    @Override
    public int getCount() {
        return fragments.size();
    }
}

I'm loading in the page adapter the fragments to show, but they are all the same with a few differences.

Layout for page 1

<RelativeLayout xmlns:android="http://schemas.android.com/apk/res/android"
    xmlns:tools="http://schemas.android.com/tools"
    android:layout_width="match_parent"
    android:layout_height="match_parent"
    android:background="#F7F4E7">
    <ImageView
        android:id="@+id/imageView1"
        android:layout_width="match_parent"
        android:layout_height="match_parent"
        android:layout_alignParentLeft="true"
        android:layout_alignParentTop="true"
        android:src="@drawable/page_1" />
</RelativeLayout>

Page1.java

public class Page1 extends Fragment {

    @Override
    public View onCreateView(LayoutInflater inflater, ViewGroup container, Bundle savedInstanceState) {

        View view = inflater.inflate(R.layout.page1_layout, container, false);
        return view;
    }
}

The only difference on each page is the

  android:src="@drawable/page_1"

and showing a button or hiding it for certain fragments.

I come from Objective-C, and I understand that I need to abstract this, as I have created all of this classes PageN, that are the same! ,but I don't yet understand how to load them on the pagerAdapter .

The main problem is the difficulty of abstracting resources. After compilation, they can only be accessed by their unqiue identifiers, accessible through the generated class R which you already use: R.layout.page1_layout.

Step 1

Let's be strict about the well-known concept DRY: If all of your PageN classes share the same content and the only difference is the referenced layout, you can replace all of your classes PageN with this single class:

public class PageFragment extends Fragment {

    private int layout;

    public PageFragment(int layout) {
        this.layout = layout;
    }

    @Override
    public View onCreateView(LayoutInflater inflater, ViewGroup container, Bundle savedInstanceState) {
        View view = inflater.inflate(this.layout, container, false);
        return view;
    }
}

You can now use this code to initialize the fragments:

fragments.add(new PageFragment(R.layout.page1_layout));
fragments.add(new PageFragment(R.layout.page2_layout));
fragments.add(new PageFragment(R.layout.page3_layout));
// etc...

Step 2

If the layout files differ only in a resource and the visibility of a button, you should abstract it further. Let's think about how to structure the information: All we need in our fragment is the layout file (which should always be the same), the @drawable resource and a boolean for the button visibility.

Let's assume you created a new layout file containing the image view with android:id="myImageView" and the button with android:id="myButton". Everything else should be equal from page to page.

public class PageFragment extends Fragment {

    private int drawable;
    private boolean showButton;

    public PageFragment(int drawable, boolean showButton) {
        this.drawable= drawable;
        this.showButton = showButton;
    }

    @Override
    public View onCreateView(LayoutInflater inflater, ViewGroup container, Bundle savedInstanceState) {
        // Always the same layout
        View view = inflater.inflate(R.layout.page_layout, container, false);
        // Apply per-page configuration
        ((ImageView) view.findViewById(R.id.myImageView)).setImageResource(this.drawable);
        if(!this.showButton) {
            view.findViewById(R.id.myButton).setVisibility(View.INVISIBLE);
        }
        return view;
    }
}

The initialization code is not really pretty:

fragments.add(new PageFragment(R.drawable.page1, true));
fragments.add(new PageFragment(R.drawable.page2, false));
fragments.add(new PageFragment(R.drawable.page3, true));
// etc...

Step 3

Last but not least, you could abstract the initialization. That's not as simple as the previous steps and not necessarily an improvement. Depending on the total number of pages, you might want to keep the initialization code from the second step. It's actually a commonly used practice to avoid abstraction in similar situations, in other words, when the effort of storing the data in an appropriate form is unjustifiable.

In my opinion, it might be a good idea not to call fragments.add in every single line (although it causes a little overhead).

PageFragment[] fragments = {
    new PageFragment(R.drawable.page1, true),
    new PageFragment(R.drawable.page2, false),
    new PageFragment(R.drawable.page3, true)
};
for(PageFragment fragment : fragments) {
    this.fragments.add(fragment);
}

You already declared fragments as a List<Fragment>, so this code should work as well:

PageFragment[] fragments = {
    new PageFragment(R.drawable.page1, true),
    new PageFragment(R.drawable.page2, false),
    new PageFragment(R.drawable.page3, true)
};
this.fragments = Arrays.asList(fragments);

Strictly following DRY ("Every piece of knowledge must have a single, unambiguous, authoritative representation within a system"), this approach should be preferred over the last ones.

However, the most important steps are the first ones, don't waste time trying to optimize the initialization.