Simple object oriented design for a patient system I want to model this scenario in object oriented language:

A hospital has number of patients
A patient can have zero or more medical conditions
A medical condition is identified by name and date

Please review my code and tell me:

What can I do to incorporate object oriented fundamentals like reusability and extensibility?
Do I need to implement an equals method in the MedicalCondition class?
Do I need to initialize all the instance variables in constructor?

import java.util.Date;
import java.util.*;
import java.text.*;
class Patient
{
    private String name;
    private String streetAdress1;
    private String phoneNumber;
    private String streetAdress2;
    private String zipCode; //I really dont need to preform any numerical operations.
    private List<MedicalCondition> mcList;

    public Patient(String name) //Public for constructor.
    {
        this.name = name;
        mcList = new ArrayList<MedicalCondition>();
    }

    public void setName(String name)
    {
        this.name = name;
    }

    public String getName()
    {
        return name;
    }

    //All setter and getters

    public void addMedicalCondition(MedicalCondition mc)
    {
        mcList.add(mc);
    }

    public void deleteMedicalCondition(MedicalCondition mc)
    {
        mcList.remove(mc);
    }

    public List<MedicalCondition> getMedicalConditions()
    {
        return mcList;
    }

    public String toString(){
        return name;
    }
 }

class MedicalCondition
{
      private String nameOfCondition;
      Date dateOfReport;

      public MedicalCondition(String nameOfCondition,Date dateOfReport)
      {
          this.nameOfCondition = nameOfCondition;
          this.dateOfReport = dateOfReport;
      }

      public String toString()
      {
          return nameOfCondition;
      }
 }

 public class HelloWorld{

     public static void main(String args[]) throws ParseException{
        Patient p1 = new Patient("Mike");

        DateFormat format = new SimpleDateFormat("mm/dd/yyyy");
        MedicalCondition mc1 = new MedicalCondition("Diabetes",format.parse("03/23/2000"));
        p1.addMedicalCondition(mc1);

        System.out.println(p1);
        for(MedicalCondition mc : p1.getMedicalConditions())
        {
            System.out.println(mc);
        }

        p1.deleteMedicalCondition(mc1); //Delete the condition

         System.out.println(p1);
        for(MedicalCondition mc : p1.getMedicalConditions())
        {
            System.out.println(mc);
        }
     }
}

Bracing style

You started off with Allman-style bracing approach, but switched to the Java convention for HelloWorld and its main() method. Regardless of the styles you choose, please be consistent on this front. :)

Medical condition dates

Your medical conditions carry a start date, but should they also have an end date? Or are we only talking about incurable medical conditions here? While we're at it, Java 8 has new 'Time' APIs with a different (and better) approach to modeling chronology, so you may want to use the newer LocalDate class.

OK, now I get it... you delete medical conditions when they are cured. If you need to preserve medical history, you now know what you can do too...

Throwing Exceptions

public static void main(String args[]) throws ParseException

I understand HelloWorld is something like a test class to see your implementation in action, but still try to avoid doing something like this, especially when it's your main() method. One wrong input will simply cause your test class to quit with a stacktrace. It will be better if you have a method to handle the parsing and any possible errors. For example, if you want to default to the current date in case of parsing errors:

private static LocalDate parse(String date) {
    // using the new time APIs
    try {
        return DateTimeFormatter.ISO_LOCAL_DATE.parse(date, LocalDate::from);
    } catch (DateTimeParseException e) {
        return LocalDate.now();
    }
}

equals() implementation

  Do I need to implement an equals method in the MedicalCondition class?

Your call. :) The longer answer is, what does it mean when a medical condition is-equal to another? Same condition and date? How helpful will such an equivalence be? To 'group' patients by the same medical conditions? If you don't have a strong case for this, you probably wouldn't want to implement it first.

Adding medical conditions

In this regard, I will recommend your existing approach over @Amr Ayman's answer. This is mainly because your Patient class should know what it means to add a medical condition, instead of letting callers of your Patient instances 'figure that out'. A better refinement is to wrap the instance returned by getMedicalConditions() with a Collections.unmodifiableList(): this will prevent callers of the method to modify the list's contents directly, such as wiping them outright.

Actually, on this note, you can also consider making your MedicalCondition class immutable, and I shall leave that as an exercise for the healthy reader...


Exercise 6.1 from ANSI C book - getword function I have a following exercise from ANSI C book:

  Exercise 6.1. Our version of getword does not properly handle
  underscores, string constants, comments, or preprocessor control
  lines. Write a better version.

Here's my improved version of getword function and code that test if it works:

#include <ctype.h>
#include <stdbool.h>
#include <stdio.h>
#include <string.h>

#define BUFSIZE 100
#define MAXWORD 100
#define NKEYS (sizeof keytab / sizeof(keytab[0]))

int buf[BUFSIZE];
int bufp = 0;

struct key {
    char * word;
    int count;
} keytab[] = {
        { "#define", 0 },
        { "#elif", 0 },
        { "#else", 0 },
        { "#endif", 0 },
        { "#error", 0 },
        { "#if", 0 },
        { "#ifdef", 0 },
        { "#ifndef", 0 },
        { "#include", 0 },
        { "#line", 0 },
        { "#pragma", 0 },
        { "auto", 0 },
        { "break", 0 },
        { "case", 0 },
        { "char", 0 },
        { "const", 0 },
        { "continue", 0 },
        { "default", 0 },
        { "do", 0 },
        { "double", 0 },
        { "else", 0 },
        { "enum", 0 },
        { "extern", 0 },
        { "float", 0 },
        { "for", 0 },
        { "goto", 0 },
        { "if", 0 },
        { "int", 0 },
        { "long", 0 },
        { "register", 0 },
        { "return", 0 },
        { "short", 0 },
        { "signed", 0 },
        { "sizeof", 0 },
        { "static", 0 },
        { "struct", 0 },
        { "switch", 0 },
        { "typedef", 0 },
        { "union", 0 },
        { "unsigned", 0 },
        { "void", 0 },
        { "volatile", 0 },
        { "while", 0 },
};

int getword(char *, int);
int binsearch(char *, struct key *, int);
int getch(void);
void ungetch(int);

int main(void) {
    int n,  c;
    char word[MAXWORD];

    while ((c = getword(word, MAXWORD)) != EOF) {
//        printf("getword(word, MAXWORD) = %c %d\n", c, c);
//        printf("word = %s\n", word);
        if (isalpha(word[0]) || word[0] == '_' || word[0] == '#') {
            if ((n = binsearch(word, keytab, NKEYS)) >= 0) {
                keytab[n].count++;
            }
        }
    }
    for (n = 0; n < NKEYS; n++)
        if (keytab[n].count > 0)
            printf("%4d %s\n",
                   keytab[n].count, keytab[n].word);
    return 0;
}

int getch(void) {
    return (bufp > 0) ? buf[--bufp] : getchar();
}

void ungetch(int c) {
    if (bufp >= BUFSIZE)
        printf("ungetch: too many characters\n");
    else
        buf[bufp++] = c;
}

int getword(char * word, int lim) {
    int c;
    char * w = word;
    static int last;
    while (isblank(c = getch()))
        ;
    if (c != EOF)
        *w++ = c;
    if (last == '/' && c == '/') {
        while (c = getch() != '\n')
            ;
        return c;
    }
    if (last == '/' && c == '*') {
        x:
        while (c = getch() != '*')
            ;
        if (c = getch() == '/')
            return c;
        else
            goto x;
    }
    if (c == '\'') {
        while (c = getch() != '\'')
            ;
        return c;
    }
    if (c == '\"') {
        while (c = getch() != '\"')
            ;
        return c;
    }
    if (!isalpha(c) && c != '_' && c != '#') {
        *w = '\0';
        last = c;
        return c;
    }
    for ( ; --lim > 0; w++) {
        if (!isalnum(*w = getch()) && *w != '_') {
            ungetch(*w);
            break;
        }
    }
    *w = '\0';
    last = word[0];
    return word[0];
}

int binsearch(char * word, struct key tab[], int n) {
    int cond;
    int low, high, mid;

    low = 0;
    high = n - 1;
    while (low <= high) {
        mid = (low + high) / 2;
        if ((cond = strcmp(word, tab[mid].word)) < 0)
            high = mid - 1;
        else if (cond > 0)
            low = mid + 1;
        else
            return mid;
    }
    return -1;
}

What are your opinions about my solution? Is it a correct approach? Is this code enough fast?

The last logic is quite hard to trace, and it is prone to bugs. Consider /* ... */*. Once the code reached the closing /, it returns, but last is not updated, and remains /, so the next * is still treated as a beginning of a comment.
Handling of string constants has a definite problem: it doesn't account for an escaped quote char in the middle of the string (e.g. "aaa\"bbb").

Combining the two above observations, I strongly recommend to revisit the design, and realize few more functions, such as get_line_comment, get_c_comment, get_quoted_string etc.

No word in keytab starts with an underscore. You don't need to go into binsearch in that case.
The preprocessor directive may not appear in the middle of the line. I am not sure if a directive in such case should be counted.


Multithreading to allow multiple clients on to a server and client requests handling I have to create a client-server JavaFX application using MVC which does many operations consisting mainly in messages exchange.
I'd like to know if this way to handle client acceptance and client requests is good or not.

public class ServerOperations {

// other code

public void serverConnection() {
    Runnable runnable = new AcceptingClients();
    Thread thread = new Thread(runnable);
    thread.setDaemon(true);
    thread.start();
}

public class AcceptingClients extends Thread {

    static final int PORT = 9090;
    private Socket socket = null;

    ServerSocket serverSocket = null;
    @Override
    public void run() {
        try {
            serverSocket = new ServerSocket(PORT);
        } catch (IOException e) {
            System.out.println("I/O error: " + e);
        }
        while (true) {

            try {
                socket = serverSocket.accept();

                // other code

                Runnable runnable = new ClientRequests(socket, inputStream, outputStream);
                Thread thread = new Thread(runnable);
                thread.setDaemon(true);
                thread.start();

            } catch (IOException e) {
                System.out.println("I/O error: " + e);
            } catch (ClassNotFoundException ex) {
                Logger.getLogger(ServerRequests.class.getName()).log(Level.SEVERE, null, ex);
            }
        }
    }

}

public class ClientRequest extends Thread {
  // other code
  public ClientRequest(Socket socket) throws ClassNotFoundException {
         // other code
  }

@Override
public void run() {
       // handling client requests
 }
// other code
}

I want to focus on few parts of this class ServerOperations:

1) Is good to put all those thread in one class for accepting clients and handling requests or is it bad coding (if not, what a good alternative would be)? Using this way may give me problems if I have a lot of clients connected?

2) If i don't extend the class AcceptingClients with Thread I have problem with JavaFX UI thread, I can't add text in TextArea for example etc. I can't understand from where this interference come from, not even using Platform.runLater() makes me append text or interact with my controller's components.

3) To let everything run I have a method in my model where this method serverConnection() is called. When the Server class starts, the first thing it does is to open the server UI and run this method called from the model. Good or not?!
Note: Server class is different from ServerOperations. ServerOperations is a separate classe which just handle client acceptance and requests.

Note:
It is easier to provide a more complete answer when faced with a complete problem. There are several //other code areas that you may be omitting for the sake of brevity, but these could very well be providing more context to the review, and their presence could likely improve the quality of answers received.

I would recommend posting complete code as is, using a more descriptive place holder of the logic there than //other code and/or perhaps an explanation of why the missing code is considered irrelevant to the reviewer

/rant

Request for clarification
I noticed that in your AcceptingClients Thread you make a call to new ClientRequests(socket, inputStream, outputStream). However, you also define the similarly named ClientRequest with constructor public ClientRequest(Socket socket). Are these meant to be the same class, just with the latter being un-implemented? or are these differing classes, in which case I would recommend better names

On to your questions 3:

  1) Is it good to put all those thread in one class

Having a class ServerOperations containing each of your Operation threads IMO is actually good design (so long as each operation is reasonably simple).

When it comes to denser and thus more complex logic, I would instead opt for grouping each Operation class into a subdirectory /operations and using ServerOperations like a repository; a layer for accessing your collection of operations, something like ServerOperations.getOperationThread("acceptClients").

The benefit here is readability and maintainability. IMHO It is easier to understand and thus maintain the individual processes if they are in separate class files, but this would only be recommended for a collection of non-trivial Operations. If they are super simple, feel free to group them up like this!

  Using this way may give me problems if I have a lot of clients connected?

Short answer, yes. Based on the present code, I would highly recommend using non-daemon threads for these purposes. This question on SO has a TON of discourse on daemon threads but IMO the most relevant information therein is :

  Daemon threads are service providers for user threads running in the same process.

This means any Thread with setDaemon(true) should be considered a service, something that is running in the background and waiting to service a request from another, non-daemon thread. This leads the most annoying impact:

  Daemon threads can shut down any time in between their flow, Non-Daemon i.e. user thread executes completely.

Normally, if your program were ending, it would hang on exiting the JVM until all user threads are complete. As these threads are daemons, the JVM will drop these Threads and exit immediately. This means that in the edge case of a request still processing on shutdown, you will lose requests processing in these daemons.

Without information on how this daemon is used in these //other code sections, I can only conclude that you might not need to use daemonThreads

  2) If i don't extend the class AcceptingClients with Thread I have problem with JavaFX UI thread

I am unfamiliar with JavaFX so I won't offer advice I am unqualified to give

  3) To let everything run I have a method in my model where this method serverConnection() is called. [..] Good or not?!

If I am understanding this correctly, you are asking if it is good to call this method from the Model at creation (IMO that isn't a bad choice in the general sense). This question is incredibly difficult to answer without having the Model code available for context.

IMHO, this last is a question belonging in a review of the Model, or of the project as a whole, as without seeing the complete context of HOW this method is used, any feedback would basically be personal opinions about the proper use of MVC models

Hope some if not all of this helps!


Implementing tabs with panes I am new to AngularJS.  Here is my basic code for implementing Tabs with Pane.  Eventually, each tab-pane will have a form and a ui-grid that will display data from the DB.

Can I use separate controllers to initialize each tab to load the pane?
Is it good to give separate controllers for implementing a pane? I have DB calls in each pane of the tab.
Should I use any directive?

Can you suggest better ways?

<!doctype html>

    <meta charset="utf-8">
    <meta http-equiv="X-UA-Compatible" content="IE=edge">
    <title>Bootstrap tab panel with Angular</title>
 <link rel="stylesheet" href="https://maxcdn.bootstrapcdn.com/bootstrap/3.3.5/css/bootstrap.min.css" />

One

Two

Three

Panel 1 Content

Panel 2 Content

Panel 3 Content

Can I use separate controllers to initialize each Tabs to load the Pane?

Yes. Actually, that's the beauty of Angular. You can do whatever you want.

  Is it good way to give separate controllers for Implementing Pane?

There's no right or wrong. It all depends on what you're trying to do.

Your code looks ok. However, I'd suggest moving your app code into an external JS file. This way, when it grows, it doesn't accumulate in your page.

Multiple controllers are fine. Actually, I recommend you to do it, especially when logic between tabs grow. This way, you can easily add a tab (and its controller) into the fray or easily pull one out without affecting the other tabs logic.

One thing to avoid though is making the parent controller a place of sharing among the controllers. In Angular, child controllers can see the parent controller, its data, its functions. The problem happens when your child controllers implicitly depend on stuff that's on the parent controller, i.e. expect some scope variable from the parent present. Best if you explicitly hand down properties across scopes.


An r-length permuter in c++ In relation to the question I asked here, I myself came up with an answer. I would like constructive criticism and tips to improve it to make it efficient like the other answers that have been posted.

#include <algorithm>
#include <iostream>
#include <set>
#include <vector>

int main() {
    std::vector<int> job_list;
    std::set<std::vector<int>> permutations;
    for (unsigned long i = 0; i < 7; i++) {
        int job;
        std::cin >> job;
        job_list.push_back(job);
    }
    std::sort(job_list.begin(), job_list.end());
    std::vector<int> original_permutation = job_list;
    do {
        std::next_permutation(job_list.begin(), job_list.end());
        permutations.insert(std::vector<int>(job_list.begin(), job_list.begin() + 3));
    } while (job_list != original_permutation);

    for (auto& permutation : permutations) {
        for (auto& pair : permutation) {
            std::cout << pair << " ";
        }
        std::endl(std::cout);
    }

    return 0;
}

In general, the code is nice and readable.  Here's some small suggestions:

This:

for (unsigned long i = 0; i < 7; i++) {
    int job;
    std::cin >> job;
    job_list.push_back(job);
}

can be simplified (?) to

std::copy_n(std::istream_iterator<int>{std::cin}, 7, std::back_inserter(job));

Introduce variables when you use them.  For example, the declaration of permutations may be moved after reading and sorting the numbers.
Marking original_permutation as const indicates that it is used for comparison only.
Use emplace:

permutations.emplace(job_list.begin(), job_list.begin() + 3);

instead of

permutations.insert(std::vector<int>(job_list.begin(), job_list.begin() + 3));

const auto& in the for loops.  Also, std::cout << '\n' instead of std::endl(std::cout).
Consider extracting the functionality into a function and changing magic numbers to arguments.

Of course, there are more efficient ways to do this.  Your code generates
$$ 7! = 7 \times 6 \times 5 \times 4 \times 3 \times 2 \times 1 = 5040 $$
permutations, but only end up with
$$ 7^{\underline{3}} = 7 \times 6 \times 5 = 210 $$
results, and let std::set discard the remaining \$5040 - 210 = 4830\$ permutations.  You can modify your code so that it only stores one in \$4! = 24\$ permutations, but the other answers to the linked question provide better algorithms.


Simple string-split by root and sufix algorithm A classic problem to split a string into its "root and suffix":

(Stemming) The word "walk" is the base form for word "walking" (suffix is "ing");
The file path /var/www is the base path of /var/www/myApp;
The string doi:10.1038/ncomms is the URN-prefix for doi:10.1038/ncomms7368 and doi:10.1038/ncomms7666;
The word "hello" is not a root for "walking" (suffix is "walking");

There are many ways to implement the same algorithm, so, what is the best? Examples in PHP, but valid for any language.

/**
 * Splits a string by its root and sufix.
 * @param $str string input
 * @param $root string empty when no root, or start string
 * @return array (rootFlag,sufix)
 */
function  str_splitByRoot($str, $root){
    ...
}

Algorithms str_splitByRoot1(), str_splitByRoot2(), ..., str_splitByRoot5()  or other (show more if you know). All do the same thing, are valid solutions.

function  str_splitByRoot1($str, $root){
    if (strpos($str,$root)===0)
        return array($root, substr($str, strlen($root)) );
    else
        return array( '', $str );
}

function  str_splitByRoot2($str, $root){
    $rootLen = strspn($str ^ $root, "\0");
    return array( substr($root,0,$rootLen), substr($str,$rootLen) );
}

function  str_splitByRoot3($str, $root){
    $s = explode($root,$str);
    return ( count($s)>1  && !array_shift($s) )?
      array($root,join($root,$s)):
      array('',$str);
}

function  str_splitByRoot4($str, $root){
    // to generalize need a secure regex, something like
    // $regex = str_replace(array('/','.','-'),array('\\/','\\.','\\-'),$root);
    $suffix = preg_replace("/^$root/",$str,1,$n);
    return $n?
      array($root,$suffix):
      array('',$str);
}

function  str_splitByRoot5($str, $root){
    // need also $root translating as algorithm 4.
    if (preg_match("/^$root(.+)$/",$str,$m))
          return array($root,$m[1]);
    else
          return array('',$str);
}

The first is the traditional way. The last (algorithms 4 and 5) use regular expression, the second trim excess garbage from strings that are null terminated; and the  third  remember that the algorithm  is a kind of split (explode function).

All can be used with

function str_sepByRoot($str,$root){
   return join(' * ',str_splitByRoot($str,$root));
}
print "\n".str_sepByRoot("walking","walk");
print "\n".str_sepByRoot("hello","walk");
print "\n".str_sepByRoot("walking-walk-walk","walk");
print "\n".str_sepByRoot("/var/www/myApp","/var/www/");
print "\n".str_sepByRoot("10.1038/ncomms7368","10.1038/ncomms");

returning

   walk * ing
    * hello
   walk * ing-walk-walk
   /var/www/ * myApp
   10.1038/ncomms * 7368

About "best thing"... The "principle of simplest is best"  seems valid. The number of commands, and the computational simplicity of each command.

str_splitByRoot1: 2 fast commands (strpos and substr) and 1 condition and strlen
str_splitByRoot2: 2 fast commands (substr) and 1 to be checked (strspn+str ^ str)
str_splitByRoot2 optimized: 1 fast command (substr), 1 to be checked (strspn) and 1 condition.
...

So, my doubt was about strspn+str ^ str performance... And its time  struck me most,  was near time for str_splitByRoot1...

  $str_splitByRoot = array(
       function  ($str, $root){  // str_splitByRoot1()
            if (strpos($str,$root)===0)
                return array($root, substr($str, strlen($root)) );
           else
                   return array( '', $str );
       },

       function  ($str, $root){ //str_splitByRoot2()
           $rootLen = strspn($str ^ $root, "\0");
           return array( substr($root,0,$rootLen), substr($str,$rootLen) );
       },
       function  ($str, $root){ //str_splitByRoot2() optimized
           $rootLen = strspn($str ^ $root, "\0");
           return array( $rootLen?$root:'' , substr($str,$rootLen) );
       },

       function  ($str, $root){  //str_splitByRoot3()
         $s = explode($root,$str);
         return ( count($s)>1  && !array_shift($s) )?
             array($root,join($root,$s)):
             array('',$str);
       }

       function ($str, $root){ // str_splitByRoot5
            $root2 = str_replace(array('/','.','-'),array('\\/','\\.','\\-'),$root);
           if (preg_match("/^$root2(.+)$/",$str,$m))
                 return array($root,$m[1]);
           else
                 return array('',$str);
       }
  );

  function str_sepByRoot($i,$str,$root){
       global $str_splitByRoot;
       global $debug;
            $func = $str_splitByRoot[$i];
            $s = join(' * ', call_user_func($str_splitByRoot[$i],$str,$root)); // $func($str,$root));
            if ($s &&  $debug)
                 print "\n ___ $s";
  }

  $debug=0;
  for ($i=0; $i<count($str_splitByRoot); $i++) {
       print "\n--- $i ---";
       $time = microtime(true); // time in Microseconds
       for($j=0;($j<333 && !$debug) || ($debug && $j<1); $j++){
            str_sepByRoot($i,"walking","walk");
            str_sepByRoot($i,"hello","walk");
            str_sepByRoot($i,"walking-walk-walk","walk");
            str_sepByRoot($i,"/var/www/myApp","/var/www/");
            str_sepByRoot($i,"DOI:10.1038/ncomms7368","DOI:10.1038/ncomms");
       }
       echo "\n", (microtime(true) - $time) . ' elapsed';
  }

Result:
--- 0 ---
0.0037140846252441 elapsed
--- 1 ---
0.0045409202575684 elapsed
--- 2 ---
0.0040040016174316 elapsed
--- 3 ---
0.0088369846343994 elapsed
--- 4 ---
0.0075829029083252 elapsed
--- 5 ---
0.0037930011749268 elapsed

Conclusion, using str_splitByRoot1 as reference (100%),

str_splitByRoot2 optimized  is ~105% , so for practical use it's as good as  str_splitByRoot1.
str_splitByRoot2  is ~115%, something good when compared with str_splitByRoot3 (~125%).
str_splitByRoot5 is 225%... as expected, the worst (even without  str_replace regex escapes).

... But, the performance changes (!) when using a big root, an Lorem ipsum of 800 words. The performance profile changed to:

str_splitByRoot2 (optimized or not)  ~244%... bad!
str_splitByRoot3  (with direct split by explode) is ~120%, very good!
str_splitByRoot5 (regex)  ~390%, the worst of course.

So, the only that shows more "stability" (~120%)  about root length was str_splitByRoot5, and str_splitByRoot2 is not a good replacement for  str_splitByRoot1 in the context of big roots.


Automate the Boring Stuff - Collatz Exercise I wrote the following code for the Collatz exercise: The Collatz Sequence

Any feedback is appreciated.

  Write a function named collatz() that has one parameter named number. If number is even, then collatz() should print number // 2 and return this value. If number is odd, then collatz() should print and return 3 * number + 1.

def collatz(n):
    while n > 1:
        if n % 2 == 0:
            n = int(n // 2)
            print (n)
        elif n % 2 == 1:
            n = int(3 * n + 1)
            print (n)

n = int(input("Enter a number: "))
collatz (n)

A few simple observations:

    if n % 2 == 0:
        ...
    elif n % 2 == 1:
        ...

Here, we know that if n % 2 isn't 0, it must be 1, since n is an integer.  So that elif can be simply else, making it simpler to read.  More experienced programmers will reverse the test, knowing that 1 is the only truthy result of n % 2, so write if n % 2: ... ; else ....

    if ...:
        ...
        print (n)
    else:
        ...
        print (n)

The print that's present in both branches could be placed after the if/else, since it doesn't depend on the condition.
The results of the arithmetic expressions are already int, so no conversion is necessary.
The if/else could be reduced to a single line:

n = 3 * n + 1 if n % 2 else n // 2

but to me, that looks less clear, so I don't recommend that level of terseness here.

Modified code

Applying the observations above, I get a simpler version of the function:

def collatz(n):
    while n > 1:
        if n % 2:
            n = 3 * n + 1
        else:
            n = n // 2
        print(n)


jQuery toggle buttons to show matching content in sliding panel I have a sliding panel that I want to show content in. The content that I want to appear depends on the button that triggers the sliding panel. My project is growing rapidly and I would like to reusable jQuery code or perhaps a plugin. My code works, for example sake I have 6 buttons and 6 hidden divs in side of a sliding panel as shown in the image below:

Upon clicking a button i want to open the sliding panel and show the corresponding content. As of now I have done so by doing this:

Buttons code:

    <button id="p1" data-toggle="slidingPanel"></button>
    <button id="p2" data-toggle="slidingPanel"></button>
    <button id="p3" data-toggle="slidingPanel"></button>
    <button id="p4" data-toggle="slidingPanel"></button>
    <button id="p5" data-toggle="slidingPanel"></button>

Sliding Panel and content divs

jQuery

var slidingPanel = $('#sliding-panel');
$(body).on('click', '[data-toggle="slidingPanel"]', function() {

    //if slider is already open
    if(slidingPanel.hasClass('open')){
        slidingPanel.hide("slide", { direction: "right" }, 500).removeClass('open');
        $('.buttonContent').hide();
        return;
    }
    //if slider is close, open slider
    slidingPanel.show("slide", { direction: "right" }, 500).addClass('open');
    //show content in slider based on button pressed
    $('#' + $(this).attr('id') + 'content').show();
});

Is this the best route. Is there a better way to abstract the code?

In the click event we will see right away from reading the if statement that you are performing a check to see if the slidingPanel is open by looking for a class of open on the element, so get rid of that first comment, you should also got rid of the //if slider is close, open slider  because that is also obvious from reading the code.  I am unsure of The final comment so I won't make a judgment on it.

Remove the return statement and make it into an if/else statement, because that is essentially what you are doing and it will read nicer.

This is what it turns into

var slidingPanel = $('#sliding-panel');
$(body).on('click', '[data-toggle="slidingPanel"]', function() {
    if(slidingPanel.hasClass('open')){
        slidingPanel.hide("slide", { direction: "right" }, 500).removeClass('open');
        $('.buttonContent').hide();
    } else {
        slidingPanel.show("slide", { direction: "right" }, 500).addClass('open');
        //show content in slider based on button pressed
        $('#' + $(this).attr('id') + 'content').show();
    }
});


Automate The Boring Stuff - Character Picture Grid I understand there is another question on this already, however this is what I coded and wanted to know if this would raise any red flags?
Regarding the Character picture exercise located at the end the following page:
https://automatetheboringstuff.com/chapter4/

Say you have a list of lists where each value in the inner lists is a
one-character string, like this:
grid = [['.', '.', '.', '.', '.', '.'],
        ['.', 'O', 'O', '.', '.', '.'],
        ['O', 'O', 'O', 'O', '.', '.'],
        ['O', 'O', 'O', 'O', 'O', '.'],
        ['.', 'O', 'O', 'O', 'O', 'O'],
        ['O', 'O', 'O', 'O', 'O', '.'],
        ['O', 'O', 'O', 'O', '.', '.'],
        ['.', 'O', 'O', '.', '.', '.'],
        ['.', '.', '.', '.', '.', '.']]

You can think of grid[x][y] as being the character at the x- and
y-coordinates of a “picture” drawn with text characters. The (0, 0)
origin will be in the upper-left corner, the x-coordinates increase
going right, and the y-coordinates increase going down.
Copy the previous grid value, and write code that uses it to print the
image.
..OO.OO..
.OOOOOOO.
.OOOOOOO.
..OOOOO..
...OOO...
....O....

It's different in that it doesn't hard-code the length of the list.
grid = [['.', '.', '.', '.', '.', '.'],
        ['.', 'O', 'O', '.', '.', '.'],
        ['O', 'O', 'O', 'O', '.', '.'],
        ['O', 'O', 'O', 'O', 'O', '.'],
        ['.', 'O', 'O', 'O', 'O', 'O'],
        ['O', 'O', 'O', 'O', 'O', '.'],
        ['O', 'O', 'O', 'O', '.', '.'],
        ['.', 'O', 'O', '.', '.', '.'],
        ['.', '.', '.', '.', '.', '.']]

x = 0
y = 0

for y in range(len(grid[x])):
    for x in range(len(grid)):
        print(grid[x][y],end='')
    print()

Thanks for the help and let me know if this is not appropriate to post!

It's wasteful to use range() and then only use the result for indexing.
Instead, just iterate directly over the values:
for col in range(len(grid[0])):
    for row in grid:
        print(row[col],end='')
    print()

We can simplify, though - use the zip function to transpose the array:
transposed = zip(*grid)

for row in transposed:
    for col in row:
        print(col,end='')
    print()

The next simplification is instead of printing one character at a time, we can use join to form strings for printing:
for row in zip(*grid):
    print(''.join(row))

And we can join all the rows using '\n'.join(), giving this final version:
print('\n'.join(''.join(x) for x in zip(*grid)))

Equivalently, using map:
print('\n'.join(map(''.join, zip(*grid))))


Mocking pathlib.Path I/O methods in a maintainable way which tests functionality not implementation I have a function that constructs a path based on a function of inputs and creates the parent directory.

When writing a unit test for this function, I want to mock the I/O part of the function, namely the .exists() and .mkdir() methods of the pathlib.Path objects.  However, since both are not called until the path has been constructed, I find myself reconstructing the implementation using a long chain of return_value.__truediv__ accesses, such as below:

import pathlib
import unittest.mock
import datetime

def get(base, timestamp, channel):
    base = pathlib.Path(base)
    path = base / "tofu" / timestamp.strftime("%Y%m%d") / f"{channel:>02d}" / "quinoa.txt"
    if path.exists():
        print(path, "exists")
    else:
        path.parent.mkdir(exist_ok=True, parents=True)
    return path

@unittest.mock.patch("pathlib.Path", autospec=True)
def test_get(pP):
    pP.return_value.__truediv__.return_value.__truediv__.return_value.\
                    __truediv__.return_value.__truediv__.return_value.\
                    exists.return_value = False
    get("quorn", datetime.datetime.now(), 42)
    pP.return_value.__truediv__.return_value.__truediv__.return_value.\
                    __truediv__.return_value.__truediv__.return_value.\
                    parent.mkdir.assert_called_once()

This test passes, but I have several concerns:

It is testing implementation rather than functionality.  If I were to rewrite the second line in get to read path = base / timestamp.strftime("tofu%Y%m%d") / f{channel:>02d} / "quinoa.txt", the functionality would be identical, but the test would fail as I wouldn't be mocking the right value anymore.
It is hard to read and easy to miscount the depth of __truediv__.return_value chain.

Apart from moving the I/O out of get, can I mock this in another way, that does not share those limitations?  Or does the difficulty of writing a good test (with mock) telling me my function is too complex, despite it's small size?

Mocking system calls, especially those with side effects (mkdir()), is difficult and error prone.

I would rather use a temporary directory for test purposes. This approach has the added advantage of really testing your FS manipluation code.

import datetime
import os
import pathlib

from contextlib import contextmanager
from tempfile import TemporaryDirectory

@contextmanager
def temporary_test_dir():
    oldpwd = os.getcwd()
    with TemporaryDirectory("test-path") as td:
        try:
            os.chdir(td)
            yield
        finally:
            os.chdir(oldpwd)

def get(base, timestamp, channel):
    base = pathlib.Path(base)
    path = base / "tofu" / timestamp.strftime("%Y%m%d") / f"{channel:>02d}" / "quinoa.txt"
    if path.exists():
        print(path, "exists")
    else:
        path.parent.mkdir(exist_ok=True, parents=True)
    return path

@temporary_test_dir()
def test_get():
    # All relative FS operations will be made in a temporary directory
    result = get("quorn", datetime.datetime.now(), 42)
    assert result.parent.exists()

I'm not sure what is the point of the tested function, but it doesn't matter for this example's sake.

If you really need to mock the filesystem part, I would suggest using an existing library such as pyfakefs, although I have never had such a need.


jQuery plugin to toggle truncated text I have from a recommendation on my other question here:

My other review post

Developed a truncate plugin, and was wondering if you guys could help me improve it or see where I have gone wrong.

Plugin code:

$.fn.vishTrunc = function(options) {

    var settings = $.extend({
            size: 100,
            omission: '...',
        }, options);

    return this.each(function() {

        var elements = $(this),
            regex    = /\s+/,
            init     = function() {
                elements.each(function() {
                var textDefault = $(this).text();

                    if (textDefault.length > settings.size) {
                        var words = textDefault.trim().split(regex).slice(0,settings.size);
var replacement = $("<p class='replacement'/>").text(words.join(" ") + settings.omission);

        $(this).parent().find('.read-more').click(function(){
   if($(this).text() === 'Read More'){
    replacement.hide();
    $(this).parent().find('p:eq(1)').show();
   } else if($(this).text() === 'Read Less'){
       replacement.show();
    $(this).parent().find('p:eq(1)').hide();
   }
    $(this).text(function(i,old){
    return old==='Read More' ?  'Read Less' : 'Read More';
});
return false
});

$(this).before(replacement);
$(this).hide();

                    }
                });
            };
        init();
    });
};

all of your indentation makes it very difficult to read the code, this is what your code should look like.  you were also missing a Semi-Colon after return false

$.fn.vishTrunc = function(options) {
    var settings = $.extend({
            size: 100,
            omission: '...',
        }, options);

    return this.each(function() {
        var elements = $(this),
            regex    = /\s+/,
            init     = function() {
                elements.each(function() {
                    var textDefault = $(this).text();
                    if (textDefault.length > settings.size) {
                        var words = textDefault.trim().split(regex).slice(0,settings.size);
                        var replacement = $("<p class='replacement'/>").text(words.join(" ") + settings.omission);

                        $(this).parent().find('.read-more').click(function(){
                            if($(this).text() === 'Read More'){
                                replacement.hide();
                                $(this).parent().find('p:eq(1)').show();
                            } else if($(this).text() === 'Read Less'){
                                replacement.show();
                                $(this).parent().find('p:eq(1)').hide();
                            }

                            $(this).text(function(i,old){
                                return old==='Read More' ?  'Read Less' : 'Read More';
                            });

                            return false;
                        });

                        $(this).before(replacement);
                        $(this).hide();

                    }
                });
            };
        init();
    });
};

Personally, I think this is a lot of indentation and a lot of nested functions.

I think that you should break out the functions, this would reduce the amount of nesting and help document what each function is doing by giving them names.


Javascript image carousel I wrote the following code for creating a simple image carousel, that shows multiple images at the same time in the same order, but starting at a different position when the "next" or "prev" button is clicked.

This code works, but does it contradict programming principles and algorithms?

Is there a better solution?

var images = [
   "1.jpg",
   "2.jpg",
   "3.jpg"
];
let slider = document.getElementById("slider");
function next(){
  images.forEach((item , index)=>{
    if(slider.getAttribute("src") == item)
      if(index != images.length-1)
        nextIndex = index+1;
      else
       nextIndex = 0;
    });
   slider.setAttribute("src",images[nextIndex]);
}
function prev(){
  images.forEach((item , index)=>{
    if(slider.getAttribute("src") == item)
      if(index != 0)
         prevIndex = index-1;
      else
        prevIndex = images.length-1;
    });
   slider.setAttribute("src",images[prevIndex]);
}

  <button onClick = "prev()"> Prev </button>

  <button onClick = "next()"> Next </button>

There are several problems with your code right now.

Don't create global variables. nextIndex and prevIndex are both globals that are implicitly created when you first assign to them.
Don't abuse not using braces on if statements. It is fine to skip them for a very simple statement (assuming it is allowed in your style guide) but nesting multiple ifs without braces is a recipe for disaster.

// Much safer
if (slider.getAttribute("src") == item) {
  if(index != images.length-1)
    nextIndex = index+1;
  else
   nextIndex = 0;
}

Don't use setAttribute / getAttribute if you are accessing a standard property on an element. slider.src is shorter and easier to read than slider.getAttribute('src').
Consider making your code more modular. What happens if you now want two sliders on the page? Ideally, it should just be another line of code to add the functionality (or maybe even no more lines of code!)
Consider preloading images. TimSparrow mentioned this in the comments - with your current code the next image won't be loaded until the user clicks on the next button. This can result in a bad experience on slow connections.

There are a few more things I could say, but I believe it would be more helpful to just provide an alternative implementation.

This implementation has a couple advantages:

You can just add another image-slider div wherever you want another set of images.
By using an internal hidden div for storing the images, you automatically get preloaded images.

function createSlider(container) {
  const images = container.querySelectorAll('.images > img');

  const displayImage = container.querySelector('[data-display]')

  // State
  let imageIndex;

  function setImage(index) {
    imageIndex = index;
    displayImage.src = images[index].src;
  }

  // Show the first image initially
  setImage(0);

  // Button listener
  container.addEventListener('click', event => {
    if (event.target.hasAttribute('data-prev')) {
      setImage(imageIndex === 0 ? images.length - 1 : imageIndex - 1)
    } else if (event.target.hasAttribute('data-next')) {
      setImage(imageIndex === images.length - 1 ? 0 : imageIndex + 1)
    }
  });
}

document.querySelectorAll('.image-slider').forEach(createSlider);
.image-slider .images {
  display: none;
}

  <button data-prev>Prev</button>

  <button data-next>Next</button>


React.js Nested Nav Bar This was written for a coding challenge for a company I recently starting working for. I'm looking for any suggestions on how to clean up the code, as well as any issues anyone thinks may occur as it is written now. Fairly new to React.js, always looking to improve.

Currently the data is passed through the app component where I render the Nav Container (functional stateless component). The Nav container creates the classNames and passes data + classNames to the NestedNav that I've put below.

It started to feel a little sloppy while mapping twice below the render, not sure if that's a problem?

The data object example shows how I modeled my data. This example has a parent with children that also have children.

The parentClass is for the outer parent.
The nestedParentClass is for the children of the outer parent.
The childClass is reserved for any children of the nestedParent.

export const data = [
{
    id: 1,
    icon: <FontAwesomeIcon className='i' icon={faHeartbeat} />,
    title: 'SAFETY',
    children: [
      {title: 'Reporting', children: [
        {id: 11, title: 'I-21 Injury Reporting', url: '/safety/report'},
        {id: 12, title: 'ASAP Reporting', url: '/safety/asap-report'},
        {id: 13, title: 'General ASAP Information', url: '/safety/general'},
        {id: 14, title: 'Flight Attendant Incident Report', url: '/safety/flight-attendant-incident'},
      ]},
      {title: 'Agriculture and Customs', children: [
        {id: 15, title: 'Product Safety Data Search', url: '/safety/data-search'}]},
      {id: 16, title: 'Known Crewmember', url: '/safety/known-cremember'},
      {id: 17, title: 'Product Safety Data Search', url: '/safety/data-search'},
    ],
  },
 ]

class NestedNav extends Component {
  constructor(props) {
    super(props)
    this.state = {
      selected: '',
      nestSelect: '',
      children: [],
      active: '',
    }
  }

 handleClick = (title) => {
   this.setState({
     selected: '',
     nestSelect: '',
     children: [],
     active: '',
   })
 }

 render() {

   const {data, parentClass, nestedParentClass} = this.props
   const renderedChildElements = this.state.children
   const active = this.state.active === 'true' ? 'true' : ''

   const mappedChildren = (child, title) => {
     let childElement
     if(child) {
       childElement = child.map((child, i) =>
<Link to={'/'}>{child.title}</Link>
         {child.children ?
           <FontAwesomeIcon
             onClick={() => mappedChildren(child.children, this.state.select)}
             className='i button'
             icon={faArrowCircleDown} /> : null}

)
       this.setState({
         selected: title,
         children: childElement,
         active: 'true',
       })
     }
     return ''
   }

   const navListItems = data.map((collection, i) =>
{collection.icon}<Link to={'/'}>{collection.title}</Link>
     {collection.children ?
       <FontAwesomeIcon
         onClick={() => mappedChildren(collection.children, collection.title)}
         className='i button'
         icon={faArrowCircleRight} /> : null}

)

   return (

{this.state.selected}

         {this.state.children}

{navListItems}

   )
 }
}
export default NestedNav

it's clear and readable but i would like to point out some things :

Avoid using functions inside Render()

  A function in the render method will be created each render which is a
  slight performance hit. t's also messy if you put them in the render, which is a much  > bigger reason

Use the child.id instead of key as index when you map through an array in render() because Index as a key is an anti-pattern.

Keep your code as DRY as possible, the initialState can be an object outside the class instead of writing it twice or more, especially if it's a big object.

Optional : use Destructuring assignment to indicate which keys you will use in the function.

const initialState = {
  selected: '',
  nestSelect: '',
  children: [],
  active: '',
}

class NestedNav extends Component {
  constructor(props) {
    super(props)
    this.state = initialState
  }

  handleClick = title => {
    this.setState({ ...initialState })
  }

  mappedChildren = (child, selectedTitle) => {
    const { nestedParentClass } = this.props
    let childElement

    if (child) {
      childElement = child.map(({ id, title, children, url }) =>
<Link to={'/'}>{title}</Link>
        {children ?
          <FontAwesomeIcon
            onClick={() => this.mappedChildren(children, this.state.select)}
            className='i button'
            icon={faArrowCircleDown} /> : null}

)
      this.setState({
        selected: selectedTitle,
        children: childElement,
        active: 'true',
      })
    }
    return ''
  }

  navListItems = data => data.map(({ id, url, children, title, icon }) => {
    const { parentClass } = this.props
    return (
{icon}<Link to={'/'}>{title}</Link>
      {children ?
        <FontAwesomeIcon
          onClick={() => this.mappedChildren(children, title)}
          className='i button'
          icon={faArrowCircleRight} /> : null}

)
  });

  render() {
    const { data } = this.props
    const active = this.state.active === 'true' ? 'true' : ''

    return (

{this.state.selected}

          {this.state.children}

{this.navListItems(data)}

    )
  }
}
export default NestedNav


Encoding and decoding exercise I was issued the following coding challenge from a company to write an encoding and decoding function:

  Encoding: This function needs to accept a signed integer in the 14-bit
  range [-8192..+8191] and return a 4 character string. The encoding
  process is as follows:

  Add 8192 to the raw value, so its range is translated to [0..16383]. Pack that value into two bytes such that the most significant bit of each is cleared.
  Unencoded intermediate value (as a 16-bit integer):
  00HHHHHH HLLLLLLL

  Encoded value:
  0HHHHHHH 0LLLLLLL
  Format the two bytes as a single 4-character hexadecimal string and return it.

  Decode:

  Your decoding function should accept two bytes on input, both in the
  range [0x00..0x7F] and recombine them to return the corresponding
  integer between [-8192..+8191]

Here is my solution to both functions:

public static String encode(int num){
    if(num < -8192 || num > 8191) {
        throw new IllegalArgumentException("This function only takes ints within the 14-bit range (-8192 to 8191)");
    } else {
        //step 1
        //System.out.println("Unencoded Decimal Value: "+ num);

        num += 8192;
        //System.out.println("Intermediate Decimal: " + num);

        //step 2
        String numString = Integer.toHexString(num);
        //System.out.println("Intermediate Hex: " + numString);

        //step 3
        String theBinary = Integer.toBinaryString(Integer.parseInt(numString,16));

        while(theBinary.length() < 14){
            String zero = "0";
            zero += theBinary;
            theBinary = zero;
        }
        //Build the updated Binary
        String newBinary = "";
        for(int i=theBinary.length() -1; i>=0; i--){
            newBinary += theBinary.charAt(i);
            if(i== 7 || i == 0){
                newBinary += "0";
            }
        }
        newBinary = new StringBuilder(newBinary).reverse().toString();

        StringBuilder sb = new StringBuilder();
        sb.append(Integer.toHexString(Integer.parseInt(newBinary,2)));
        while(sb.length() < 4) {
            sb.insert(0, '0'); // pad with leading zero if needed
        }
        String hex = sb.toString();

        System.out.println("The Encoded Hex: " + hex);

        System.out.println();
        return hex;
    }
}

public static int decode(byte byte1, byte byte2){
    if(byte1 < 0x00 || byte2 < 0x00 || byte1 > 0x7F || byte2 > 0x7F){
        throw new IllegalArgumentException("This function only takes bytes between 0x00 and 0x7F");
    }
    //Append Zeros
    String pt1 = Integer.toBinaryString(byte1);
    while(pt1.length() < 7){
        String zero = "0";
        zero += pt1;
        pt1 = zero;
    }
    //Append Zeros
    String pt2 = Integer.toBinaryString(byte2);
    while(pt2.length() < 7){
        String zero = "0";
        zero += pt2;
        pt2 = zero;
    }
    String fullBinary = pt1 + pt2;
    //System.out.println("Full Binary: "+fullBinary);
    int decimalValue = Integer.parseInt(fullBinary, 2);
    //System.out.println("Decimal Value: "+decimalValue);
    int originalValue = decimalValue - 8192;
    System.out.println("Original Value: " + originalValue);

    //System.out.println();
    return originalValue;
}

I recieved that this solution "did not demonstrate the level of expertise required". The only requirements issued in the challenge were to write the functions and to return the specified value (which it does) and to do it quickly (which it does). My main question is where could I have improved this code to to "demonstrate more expertise". I'm just looking for an entry level programming position, but I keep getting responses like this that are vague even when I write code that completes the task issued.

You're taking the task too literally. All the binary stuff is just for explanation, it's not meant to be implemented.

No time now to comment it... will do it later. Learn about bit operations in the meantime.

public static String encode(int num) {
    final int translated = num + 8192;
    final int lowSevenBits = translated &  0x007F; // 0b0000_0000_0111_1111
    final int highSevenBits = translated & 0x3F80; // 0b0011_1111_1000_0000
    final int composed = lowSevenBits + (highSevenBits << 1);
    return String.format("%04X", composed);
}

You also should always write a test:

public void test() {
    assertEquals("4000", encode(0));
    assertEquals("0000", encode(-8192));
    assertEquals("7F7F", encode(8191));
    assertEquals("5000", encode(2048));
    assertEquals("2000", encode(-4096));
}


C# Update account based on user order data. Nested if-else optimization with almost similar values The method works, but I would like to know if there is any way to make it more readable, optimized?
I have user data (i want to import/update it).
Accounts is finded by user data.
========
User order data is entered into the system.
userData.Id - user Id; userData.OrderNumber - user order number.
The "UpdateAccount" method below is used by the user.
The user enters their Id and OrderNumber into the system (there are FirstName, LastName, etc., but they are irrelevant here because they are used in the UpdateAllUserDataToAccount method).
Therefore, it is necessary to discover whether an account with the relevant data exists in the system.
You can update account data if the account has userData.OrderNumber or userData.Id.
If no account under userData.OrderNumber or userData.Id is found in the system then create a new account and update its data.
Do not allow anything if userData.OrderNumber, userData.Id is in different account (can not exist in the system in different account).
An account can have userData.OrderNumber in the system and not userData.Id because the employee registered the parcel but did not have a user id.
An account can have userData.Id in the system and not userData.OrderNumber because the user has previously been registered in the system.
The user data userData.OrderNumber and userData.Id are unique and belong to only one user.
bool _isSpecialData - to indicate that the user is special (set before this code).
private void UpdateAccount(UserModel userData)
{
    Account accountById = _accountController.GetAccountById(userData.Id);
    Account accountByNumber = _accountController.GetAccountByNumber(userData.Number);
    bool _isSpecialData = accountByNumber != null ? accountByNumber.Vip : false;

    if (accountById != null)
    {
        if (accountByNumber != null)
        {
            if (accountById.Id == accountByNumber.Id)
            {
                if (_isSpecialData)
                {
                    AddPartUserDataToAccount(userData, accountByNumber);

                    if (userData.Status == Blocked) return;
                }
            }
            else
            {
                _log.Error($"User data can not be in different accounts");
                return;
            }
        }
        else
        {
            AddPartUserDataToAccount(userData, accountById);
        }
    }
    else
    {
        if (accountByNumber != null)
        {
            if (accountByNumber.Id == null)
            {
                accountByNumber.Id = userData.Id;

                if (_isSpecialData)
                {
                    AddPartUserDataToAccount(userData, accountByNumber);
                    if (userData.Status == Blocked) return;
                }
            }
            else
            {
                _log.Error($"accountByNumber.Id can be just with the same value as userData.Id or with null (because it was not set in first place)");
                return;
            }
        }
        else
        {
            CreateNewAccount(userData);
        }
    }

    UpdateAllUserDataToAccount(userData);
}

This code is hard to read, deeply nested and hard to understand. But you already know that.
Since you have a lot of binary cases (if-else) I suggest creating a truth table
on paper or whiteboard https://en.wikipedia.org/wiki/Truth_table#Binary_operations for your different combinations of input and output.
This should help you figure out which cases can be simplified, short-circuit, etc, and then you can rewrite the code to make it more readable. Use early returns and maybe refactor it into functions that allow you to check error conditions and return/exit as soon as possible for each case.
The end goal should be a code structure that doesn't nest 4 5 levels deep, preferrably no more than 2.


TCP Client Class implementation in C# I tried to implement following class which should handle connecting to a server, as well as sending and receiving data.

Your feedback is welcome. Please note I am beginner in .NET and don't give too advanced feedback. I tested it and it seems to work. So in case you see some major problems please let me know, otherwise this class seems to fulfill my needs. I may add some small functionalities though.

Also can I better release resources than I am doing now? (maybe automatically close all resources I am using if that's possible?)

 class DppTCPClient
    {
        public TcpClient m_client = new TcpClient();

        public void Connect(string address, int port)
        {
            if (m_client.Connected)
                throw new Exception("Connect: Already connected");

            m_client.Connect(address, port);
        }

        public void WriteBytes(byte[] data, int length)
        {
            if (data.Length != length)
                throw new Exception("WriteBytes: Length should be same");

            // Get access to network stream
            Stream stm = m_client.GetStream();
            stm.Write(data, 0, data.Length);

        }

        public void ReadAllBytes(byte[] buffer, int length)
        {
            if (buffer.Length != length)
                throw new Exception("ReadAllBytes: Length should be same");

            Stream stm = m_client.GetStream();

            // Start reading
            int offset = 0;
            int remaining = length;
            while (remaining > 0)
            {
                int read = stm.Read(buffer, offset, remaining);
                if (read <= 0)
                    throw new EndOfStreamException
                        (String.Format("ReadAllBytes: End of stream reached with {0} bytes left to read", remaining));
                remaining -= read;
                offset += read;
            }

        }

        public void CloseDppClient()
        {
            if (m_client.Connected)
            {
                m_client.GetStream().Close();
                m_client.Close();
            }
        }
    }

It looks like you should be implementing IDisposable here. See on MSDN.

Then, in your Dispose(bool) method, you can call CloseDppClient (or, better yet, do them the other way around and call Dispose() from your CloseDppClient method).

You should also be putting explicit access modifiers on your classes, properties, methods, fields, events, etc. As it stands now, DppTCPClient is internal.

Also, it's usually expected to follow PascalCasing throughout C# programs for public, class, interface and enum definitions.

I've left DppTcpClient as internal, but if you want it accessible outside the assembly simply replace internal with public.

An example would look like:

internal class DppTcpClient
    : IDisposable
{
    // Flag: Has Dispose already been called?
    bool disposed = false;

    // Original code

    public void CloseDppClient()
    {
        Dispose();
    }

    public void Dispose()
    {
        Dispose(true);
        GC.SuppressFinalize(this);
    }

    protected virtual void Dispose(bool disposing)
    {
        if (disposed)
        {
            return;
        }

        if (disposing)
        {
            if (m_client.Connected)
            {
                m_client.GetStream().Close();
                m_client.Close();
            }
            // Free any other managed objects here.
        }

        // Free any unmanaged objects here.
        disposed = true;
    }

    ~DppTcpClient()
    {
        Dispose(false);
    }
}

Then, you should be using your DppTcpClient where you need it:

using (DppTcpClient client = new DppTcpClient())
{
    // Code to use `client` here
}

So long as you always wrap any DppTcpClient objects with a using, you'll never run into a stale client left open.

Always use braces.

if (data.Length != length)
    throw new Exception("WriteBytes: Length should be same");

Should be written as:

if (data.Length != length)
{
    throw new Exception("WriteBytes: Length should be same");
}

Braces can't prevent bugs, but they can help prevent bugs. (If you know what I mean.)

From what I can tell, your ReadAllBytes method is actually not supposed to read all the bytes in the stream (or so it sounds), by this manner you should be changing how you work with it internally.

Instead of having the user supply a buffer, you should supply it by use of either:

A return type;
An out parameter;

The return type would be more appropriate here, so you could do something like:

public byte[] ReadAllBytes()
{
    using (MemoryStream ms = new MemoryStream())
    {
        m_client.GetStream().CopyTo(ms);
        return ms.ToArray();
    }
}

See: https://stackoverflow.com/questions/19387979/get-length-of-data-available-in-networkstream for more information on reading the entire stream.

This simplifies it greatly, and takes a large brunt of the work off your shoulders.

Just as well, you can completely remove the int length parameter from the WriteBytes method:

public void WriteBytes(byte[] data, int length)
{
    // Get access to network stream
    Stream stm = m_client.GetStream();
    stm.Write(data, 0, data.Length);
}

Lastly, I would rename your ReadAllBytes method to ReadBytes, since that's what it's doing.


Calculating the cube root Inspired by a question on SO I was checking how to calculate the cube root of an integer and found a C-ish solution on Hacker's Delight:

// Program for computing the integer cube root.
// Max line length is 57, to fit in hacker.book.
#include <stdio.h>
#include <stdlib.h>     //To define "exit", req'd by XLC.

// Execution time is 3 + (11 + mul)11 = 124 + 11*mul (avg) cycles.
// ------------------------------ cut ----------------------------------
int icbrt1(unsigned x) {
   int s;
   unsigned y, b;

   y = 0;
   for (s = 30; s >= 0; s = s - 3) {
      y = 2*y;
      b = (3*y*(y + 1) + 1) << s;
      if (x >= b) {
         x = x - b;
         y = y + 1;
      }
   }
   return y;
}

I tried to adapt this to Common Lisp and came up with this:

(defun icbrt (x)
  "Returns the integer cube root of X."
  (assert (plusp x) (x) "Please provide a positive integer! ~D < 0" x)
  (loop for s downfrom 30 to 0 by 3
        for y of-type integer = 0 then (* 2 y)
        for b of-type integer = (ash (1+ (* 3 y (1+ y))) s)
        when (>= x b)
        do (incf y)
           (setf x (- x b))
        finally (return y)))

While it works I am still wondering if it could be express more idiomatic. I am especially unsure about the setf within the loop construct.

Any comment is gratefully acknowledged.

There is not much to say about this code, it is fine.

You can get rid of incf and setf; the varying x is replaced by a variable named z; I express the comparison as a boolean variable greater (for lack of a better name), which gives:

(defun icbrt (x)
  "Returns the integer cube root of X."
  (check-type x (integer 1))
  (locally (declare (type (integer 1) x))
    (loop
       for s downfrom 30 to 0 by 3
       for z of-type integer = x then (if greater (- z b) z)
       for y of-type integer = 0 then (* 2 (if greater (1+ y) y))
       for b of-type integer = (ash (1+ (* 3 y (1+ y))) s)
       for greater = (>= z b)
       finally (return y))))

Note also that I removed the assertion and used check-type instead. While the additional comment is nice in the assert expression, it adds to the things developers have to maintain (think consistency of error messages), whereas check-type is supposedly already displaying the right amount of information to the user, and throws the right kind of exception.


Login and account system So here I wanted to make a system that registered a person's account. It remembers the person's email, account, first and last names, by writing to a file. You can login to that account by typing the email and password according to that email and then you would get a hello message, saying your first and last name that you used to register your account with.

data_text = []
first_names = []
last_names = []
emails = []
passwords = []

def user_choice():
    print('Hello user, create an account or login to an existing one.')
    choice = input('Insert "1" if you wish to create an account or "2" if you wish to login: ')
    print('\r')
    if choice == '1':
        create_account()
        user_choice()
    else:
        login_account()
        user_choice()

def register_info():
    with open('Login_Data.txt', 'r') as login_data:
        global data_text, first_names, last_names, emails, passwords
        data_text = login_data.readlines()
        for i in data_text:
            data_text[data_text.index(i)] = i.strip()
        emails = (data_text[2::4])

def create_account():
    with open('Login_Data.txt', 'a') as login_data:
        first_name = input('First name: ')
        last_name = input('Last name: ')
        email = input('Insert your Email adress: ')
        while email in emails:
            print('That email is already registered')
            email = input('Insert another Email adress: ')
        password = input('Create a password: ')
        passwordc = input('Confirm your password: ')
        info = [first_name, last_name, email, password]
        while passwordc != password:
            print('The passwords do not match.')
            passwordc = input('Reinsert your password: ')
        for i in info:
            login_data.write(i)
            login_data.write('\n')
    print('Nice! Your account was registered.')
    print('\r')
    register_info()

def login_account():
    register_info()
    with open('Login_Data.txt', 'r'):
        login_email = input('Email: ')
        while login_email not in emails:
            print('Invalid Email')
            login_email = input('Reinsert your Email: ')
        login_password = input('Password: ')
        while login_password != data_text[data_text.index(login_email) + 1]:
            print('Invalid password')
            login_password = input('Reinsert your password: ')
        print('Hello {} {}, welcome back!'.format(data_text[data_text.index(login_email) - 2], data_text[data_text.index(login_email) - 1]))
        print('\r')
user_choice()

user_choice is your main loop, and so it would be better described as main. Whilst fairly undescriptive on what it does you can add a docstring to add information on what it performs.
It is more idiomatic to use loops in Python rather than recursion. This is partly due to the recursion limit, and partially that the iterator pattern has a lot of support in Python.
I've rarely seen '\r' in Python. I don't see why you would need it and so have just removed them.
I like the way that you got a users email. The use of a while loop here is pretty clean.
I would change the prompted text when getting an email as it's different to the rest of the prompts.
I would DRY your do while loop. There's not really a point to having two different input prompts with the above suggestion.
Recently I was creating an account using passwd and multiple times I kept messing up the first password entry. It was rather vexing as I knew I messed up and I just had to hope I pressed backspace the correct amount of times to fix my mess up. Unfortunately I wasn't skilled enough to fix my mistake.

However with your solution I would have to quit out of your entire program, removing the data I have already to fix my mistake. You should instead get again the password and the confirmation.
There is no point in opening login_data until the user input has been confirmed to be correct. You should try to keep with statements as small as necessary.
I would prefer to build a dictionary or an abstract datatype for the user information.
You can remove one of your login_data.writes by passing i + '\n'.
"Login_Data.txt" seems ok, but it's not great. You can use JSON via import json and make a much easier and standard file type to work with.
Your interactions with your globals seem really poor. If you want a global, than I suggest you only have one. This is easy when using dictionaries.

I would also suggest changing your code to have no globals.
I would change login_account to align with the above, changed do-while loops, no globals and using JSON.
Since we are using JSON we can easily change the format to display the first and last name easier.
Your solution is in no way safe, it's prone to over the shoulder attacks by not using getpass and you're storing passwords in plain-text. This is not safe. Do not use this for anything more than a toy project that will only ever have you entering a password unique to this toy.

import json

def main():
    """Create an account or log in."""
    while True:
        print('Hello user, create an account or login to an existing one.')
        choice = input('Insert "1" if you wish to create an account or "2" if you wish to login: ')
        print()
        if choice == '1':
            create_account()
        else:
            login_account()

def input_user_email(emails):
    """Get an unowned email."""
    while True:
        email = input('Email: ')
        if email not in emails:
            return email
        print('That email is already registered.')

def input_user_password():
    """Get a user's email."""
    while True:
        password = input('Password: ')
        confirmation = input('Confirm password: ')
        if password == confirmation:
            return password
        print('The passwords do not match.')

def get_user_information(emails):
    """Get a user's information."""
    return {
        'first_name': input('First Name: '),
        'last_name': input('Last Name: '),
        'email': input_user_email(emails),
        'password': input_user_password(),
    }

def create_account():
    """Create user account."""
    with open('login_data.json', 'w+') as f:
        users = json.load(f)
        emails = {user['email'] for user in users}
        user = get_user_information(emails)
        users.append(user)
        json.dump(users, f)
    print('Nice! Your account was registered.\n')

def login_account():
    """Log into a user account."""
    with open('login_data.json') as f:
        users = json.load(f)
    by_email = {user['email']: user for user in users}

    while True:
        email = input('Email: ')
        if email in by_email:
            user = by_email[email]
            break
        print('Invalid Email')

    while True:
        password = input('Password: ')
        if password == user['password']:
            break
        print('Invalid password')

    print('Hello {0.first_name} {0.last_name}, welcome back!\n'.format(user))

if __name__ == '__main__':
    main()


Making C++11 range-based for loops a bit more useful C++11 is great. Probably one of the most beautiful features (in my opinion) is the so-called range-based-for-loop.
Instead of

for ( std::size_t i(0); i < range.size(); ++i )
{
   // do something to range[i];
}

or

for ( Range::iterator it(range.begin()); it != range.end(); ++it )
{
   // do something to *it;
}

or simplified with C++11 auto:

for ( auto it(range.begin()); it != range.end(); ++it )
{
   // do something to *it;
}

we can say this:

for ( auto& entry : range )
{
   // do something with entry.
}

This is very expressive: We talk about the entry, rather than the i^th position in the range or the iterator pointing to an entry. However, this syntax lacks the ability of having subranges (e.g. ignoring the last entry). In a series of small helper structs / methods, I want to add this functionality in a clean way.

So much for my motivation ;-) Now, for the real deal.

In this post, I address conditions on the entries of range. Basically, the helper code should perform the equivalent of

for ( auto& entry : range )
{
   if ( condition(entry) )
   {
       // do something to entry.
   }
}

but without that level of indentation.

Here is the code for ConditionalRange:

template <typename Range, typename Runnable>
ConditionalRange<Range, Runnable> makeConditionalRange(Range& range, Runnable&& condition)
{
   static_assert(std::is_same<decltype(condition(*std::declval<Range>().begin())), bool>::value, "Condition must return a boolean value.");
   return ConditionalRange<Range, Runnable>(range, std::forward<Runnable>(condition));
}

template <typename Range, typename Runnable>
struct ConditionalRange
{
   public:
      friend ConditionalRange makeConditionalRange<>(Range&, Runnable&&);
   public:
      using iterator_type = ConditionalIterator<decltype(std::declval<Range>().begin()), Runnable>;
      iterator_type begin() const
      {
         auto b = range.begin();
         while ( b != range.end() && !condition(*b) )
         {
            ++b;
         }
         return ConditionalIterator<decltype(std::declval<Range>().begin()), Runnable>(b, range.end(), condition);
      }
      iterator_type end() const
      {
         return ConditionalIterator<decltype(std::declval<Range>().begin()), Runnable>(range.end(), range.end(), condition);
      }
   private:
      ConditionalRange(Range& range_, Runnable&& condition_)
      :
         range(range_),
         condition(std::forward<Runnable>(condition_))
      {
      }
   private:
      Range& range;
      Runnable condition;
};

This is the helper struct for the iterator:

template <typename Iterator, typename Runnable>
struct ConditionalIterator
{
   private:
      Iterator iterator;
      Iterator end;
      const Runnable& condition;
   public:
      ConditionalIterator(Iterator b, Iterator e, const Runnable& r)
      :
         iterator(b),
         end(e),
         condition(r)
      {
      }
      auto operator*() -> decltype(*iterator)
      {
         return *iterator;
      }
      ConditionalIterator& operator++()
      {
         do
         {
            ++iterator;
         }
         while ( iterator != end && !condition(*iterator) );
         return *this;
      }
      bool operator==(const ConditionalIterator& second)
      {
         return iterator == second.iterator;
      }
      bool operator!=(const ConditionalIterator& second)
      {
         return !(*this == second);
      }
};

The intended usage is:

std::vector<int> ns{0, 1, 2, 3, 4, 5, 6, 7, 8, 9};
for ( const auto& n : makeConditionalRange(ns, [](int a) { return a % 2 == 0; }) )
{
   std::cout << n << " ";
}
std::cout << "\n";

Demo: click here!

What are weaknesses of my approach? With what kind of arguments does it fail, although it shouldn't in the perfect world?

You can use braces to return from your functions. That way, you won't have to repeat long and cumbersome types (DRY, as much as possible). For example, ConditionalRange::end:

iterator_type end() const
{
    return { range.end(), range.end(), condition };
}

Also, you probably want your function to accept C-style arrays too. Therefore, you should use std::begin and std::end instead of calling the member functions:

iterator_type end() const
{
    return { std::end(range), std::end(range), condition };
}

With that (everywhere in your code) and some trivial changes, your code will also work for C-style arrays (tested here and it works fine).

Naming your function

The name makeConditionalRange is quite long. The function you created already exists in other libraries and programming languages (Python comes to my mind) under the name filter. You should consider changing its name in order for many users to recognize the name at first glance (moreover, it will be shorter).


Print a word, one character per line, backwards I have a problem that I solved in 2 different ways and I would like to know which one is better, cleaner and easier to read.
Problem:
Write a while loop that starts at the last character in the
string and works its way backwards to the first character in the string,
printing each letter on a separate line.
Example 1:
char = 'Wednesday'
cont = -1
cont2 = len(char)
cont3 = len(char) + 1
while cont < cont2:
    print(char[cont2:cont3])
    cont2 = cont2 - 1
    cont3 = cont3 - 1

Example 2:
char = 'Wednesday'
cont = len(char) - 1
while cont >= 0:
    cont2 = char[cont]
    print(cont2)
    cont -= 1

Looking at the two implementations side by side, ignoring the goal of the exercise and alternative implementations, I'd say the second example is better:

It uses one fewer variables.
It does a simple index lookup rather than a slice.
The implementation is easier to follow.

For simple problems like this it's often useful to think of the complexity of the solution as the number of "tokens", or distinct language elements, necessary to write it down. For example, print(cont2) is four tokens: print, (, cont2 and ). By a quick read the first example has about twice as many tokens as the second.


Typewriter animation, implemented using recursive asynchronous function In this animation, I am wondering if it's a good idea using async/await with recursive calls and the effect may be caused to the javascript event loop.

Core function:

const wait = (ms) => new Promise(r => setTimeout(r, ms));

async function typeWriter(text, n, elementId, waitAfter) {
    const el = document.getElementById(elementId);
    if (n < text.length) {
        requestAnimationFrame(() => {
            el.innerHTML = (text.substring(0, n + 1));
        }
        n++;
        await wait(waitAfter) //wait after letter stroke
        await typeWriter(text, n, elementId, waitAfter) //recursion till text finish
    }
}

Full code

https://github.com/Microsmsm/microsmsm.github.io/blob/master/js/typewriter.js

Result:

https://microsmsm.com/

Recursion is a state stack.
Function context
Every time a function is called JS creates a new context (function state), even if the function does nothing, creates no variables, has no arguments, only calls another function, it still requires its own context. The process of creating and pushing to the heap costs memory and processing time.
The minimum memory cost varies between JS engines, but 1K is a reasonable estimate for an empty context.
Note The exception is Proper tail call Functions that by nature of the return, may not require their own context, they use the calling functions context. (No browser current lets you use this ES6 required standard feature)
Recursive state stack
In general recursion is used as a way create a state stack. Each iteration (recursive) call creates a new function context, with closure that is pushed to the heap.
When the a function exits its state is deleted popped from the heap and the calling function's state is reinstated from the heap.
Example showing states pushed and popped
The following illustrates the state stack. The state includes a random value. Each iteration waits 200ms before creating the next. When the recursion exits each state is popped from the heap, the functions complete execution until the stack is clear.

    function resolveAfter200ms(x) {
      return new Promise(resolve => {
        setTimeout(() => resolve() , 200);
      });
    };
    async function test(count){
        const rand = Math.random() * 100 | 0;
        log(` push ${rand}`);
        if(count < 10){

            await resolveAfter200ms(count);
            await test(count+1);
        } else {
           log("
Top of stack
Popping function context states from heap
");
        }
        log(` pop ${rand}`);

        if(count === 0){
           log("
Iteration complete");
        }
    }
    function log(data){
        document.body.innerHTML += `<span>${data}</span>`;
    }
    test(0);

Why this makes your function bad.
Saving state to a stack is great when you have complex or branching data structures that you need to iterate.
Saving an irreverent state on the other hand is not so great. In fact I would say using an aysnc function to step over animation frames via recursion is about the worst way to create an animation.
If you have 200 characters to animate by the time they have all completed (the last recursive call) you have 200 function states on the heap.
I would estimate that the memory usage of a 200 character animation to be about 200K +, though that is nothing, I have seen people chew 1Meg just to add two integers in JavaScript (crazy eh!).
Luckily JavaScript makes such wasteful resource usage transparent, by providing ample memory and a gaggle of resource management threads to contain and clean up so from your point of view everything is running slick..
Another way.
I am a little long in the tooth and come from a time where 1K was all we had. I can not write code without a device angel whispering thoughts of "conserve, speed and memory"
So I would have written your code as follows if the requirement was to use async functions, and assuming that the waitAfter value is many times greater than 60fps.
// Note dont need text position (n)
async function typeWriter(text, elementId, waitAfter) {
    var n = 0;
    // Following DOM query only done once saving lots of time
    const el = document.getElementById(elementId);

    const wait = () => new Promise(r => setTimeout(r, waitAfter));

    // Preventing re flow overhead by using textContent rather than innerHTML
    const render = () => el.textContent = (text.substring(0, n + 1));

    while (n < text.length) {
        requestAnimationFrame(render);  // Calls existing function
                                        // thus avoid unneeded function state capture
                                        // via closure

        await wait();

        n++;                            // add after await so render gets
                                        // the correct value
    }
}

From an external view its behaviour is identical, from a resource point of view its is remarkably different, will much lower memory use and associated GC overhead upon exit. It can also handle any size text, unlike the recursive method was limited to the available call stack size.


Display element in a grid and conditionally add to redux or remove if not selected I have some logic on my JSX for an element in a grid, if element is selected, then add to redux, if element is not selected then remove from redux...
is working ok, but I sense a bad smell...
<GridSelector
          items={items}
          onChange={(categoryId, isSelected) => {
            favorites[categoryId] = isSelected;
            const favsArraya = Object.values(onboarding.favoriteCategories);
            let index;

            if(isSelected){
              if (favsArraya.indexOf(categoryId) === -1){
                //doesnt exist, add it
                 favsArraya.push(categoryId)
              }
            }else{
              if (favsArraya.indexOf(categoryId) !== -1){
                // exists, delete it
                index = favsArraya.indexOf(categoryId);
                favsArraya.splice(index, 1);
              }
            }

            dispatchData({ [DATA_KEY.FAVORITE_CATEGORIES]: favsArraya})

          }}
        />

First of all, there's way too much going on here for an inline function. Let's seperate that out. Also, the splicing can be replaced with the more expressive filter.
const addIfMissing = (array, item) =>
    array.includes(item) ? array : array.concat([item]);

const without = (array, exclude) =>
    array.filter(x=> x!==exclude);

const onGridSelectorChange = (categoryId, isSelected) => {
    favorites[categoryId] = isSelected;
    const categoryIds = Object.values(onboarding.favoriteCategories);
    const newCategoryIds = isSelected ?
        addIfMissing(categoryIds,categoryId) :
        without(categoryIds,categoryId);

    dispatchData({ [DATA_KEY.FAVORITE_CATEGORIES]: newCategoryIds})

}


Incrementing specified list values, with "catch-up" I completed a coding challenge recently in which my function was to be given two parameters: N, the number of counters to be used (numbered 1 through N) - and A, an array of integers. Iterating through the array A, when the index of a counter occurs, that counter is incremented by 1. When an index of N+1 occurs, all of the counters are reset to the maximum counter at that point.

def solution(N, A):
    li = [0] * N
    #print('A is %s' % A)
    #print('N is %s' % N)
    max_val = 0

    for i in A:
        i = i-1
        #print('i is %s' % i)
        #print('li is %s' % li)
        if i == N:
            #print('  in if')
            li = [max_val] * N
        else:
            #print('  in else')
            li[i] = li[i] + 1
            if li[i] > max_val:
                max_val = li[i]

    return li

[Using Python 3.6]
(See related question.)

li is the name of your list of counters. The fact they are counters matters more than the fact it's a list, so, I'd call it counters.
I like the clever way you updated all elements in the counters list by creating a new one, but it may be slower than looping through the list and changing all elements in place. It'll also use twice as much memory (before the old one is GC'ed).
Your previous version didn't update max_val. This was fixed since this was originally posted.


Sieve of Atkins algorithm I got the inspiration to write this algorithm from this question. The OP cited the Sieve of Atkin as one of the fastest ways to generate primes. So I figured I would give it a try:

def sieve_primes(limit=100):
    sieve = [False]*limit
    loop_range = range(1, int(limit**(1/2)))

    for x, y in [(x,y) for x in loop_range for y in loop_range]:
        for x_coef, y_coef, mods in [(4, 1, [1, 5]), (3, 1, [7]), (3, -1, [11])]:
            if y_coef == -1 and x <= y:
                continue

            value = x_coef * x**2 + y_coef * y**2
            if value > limit:
                continue

            if value % 12 in mods:
                sieve[value] = not sieve[value]

    for value in loop_range[4:]:
        if sieve[value]:
            for square in [i * value**2 for i in range(limit//value**2+1)
                           if i * value**2 < limit]:
                sieve[square] = False

    return sieve

The algorithm is based off of pseudocode given by the reference to the Sieve of Atkins above. The reference does say:

  This pseudocode is written for clarity. Repeated and wasteful calculations mean that it would run slower than the sieve of Eratosthenes. To improve its efficiency, faster methods must be used to find solutions to the three quadratics.

I wanted to make the pseudocode feel as Pythonic as possible. I also tried to optimize it as much as I could without delving too much into the mathematics.

Do you guys see any improvements, performance or style-wise?

Here's some profiling:

import timeit

for x in [100, 1000, 10000]:
    time = timeit.timeit('sieve_primes({})'.format(x),
                         setup='from __main__ import sieve_primes', number=10000)
    print('Number of primes found: {}'.format(x))
    print('\tTotal time: {}\n\tTime per iteration: {}'.format(time, float(time)/10000))

# Results
Number of primes found: 100
    Total Time: 2.3724100288364544
    Time per Iteration: 0.00023724100288364545
Number of primes found: 1000
    Total Time: 26.933066114727716
    Time per Iteration: 0.0026933066114727716
Number of primes found: 10000
    Total Time: 297.5023943142171
    Time per Iteration: 0.0297502394314171

for x, y in [(x,y) for x in loop_range for y in loop_range]:

Here you could avoid the list creation by using a generator (by changing the [] to ()) or you could use itertools.product. Probably no difference performance-wise, but the latter is less code.

    for x_coef, y_coef, mods in [(4, 1, [1, 5]), (3, 1, [7]), (3, -1, [11])]:

Again, you are creating lists which takes time. This whole thing you iterate over could be a constant nested tuple that's only allocated once at the top (or even outside the function).

        for square in [i * value**2 for i in range(limit//value**2+1)
                       if i * value**2 < limit]:

At this point you can probably guess that I'll recommend doing away with the list. A generator should work. If on Python 2, xrange is probably better than range as well.

(Note, I didn't mention loop_range. That could be changed into xrange invocations if you are on Python 2, but you seem to be on Python 3.)

There's also a question mark I have: int(limit**(1/2)) – that works in Python 3, but not Python 2, because 1/2 == 0. Since your code seemed to work for you, I assume you are on Python 3. I'm not sure if Python 3 people think making it portable with a dot/cast is a good idea or not (i.e. 1./2). There's always the explicit sqrt if you can't decide.


Path sum in binary tree Root to leaf path sum equal to a given number
Given a binary tree and a number, return true if the tree has a root-to-leaf path such that adding up all the values along the path equals the given number. Return false if no such path can be found.

I have written 3 solutions: one recursive, one using 2 stacks (DFS), and last one using 2 queues (BFS). I am looking for improvements and suggestions.
    // Recursive version
    public static boolean hasPathSum1 (Node root, int sum) {
          if (root == null) {
              return false;
          }
          if(root.left == null && root.right == null && root.data == sum) {
                return true;
          }
          return hasPathSum1(root.left, sum - root.data) ||
                 hasPathSum1(root.right, sum - root.data);
    }
    //===========================================================================
        // Iterative version (DFS) - Using 2 Stacks
        public static boolean hasPathSum2 (Node root, int sum) {
            if (root == null) {
                return false;
            }
        Stack<Node> nodes = new Stack<Node>();
        Stack<Integer> values = new Stack<Integer>();

        nodes.push(root);
        values.push(root.data);

        while (!nodes.isEmpty()) {
            Node current = nodes.pop();
            int sumValue = values.pop();

            if (current.left == null && current.right == null && sumValue == sum) {
                    return true;
            }
            if (current.left != null) {
                nodes.push(current.left);
                values.push(current.left.data + sumValue);
            }
            if (current.right != null) {
                nodes.push(current.right);
                values.push(current.right.data + sumValue);
            }
        }
        return false;
    }
//========================================================================
    // Iterative version (BFS) - Using 2 queues
    public static boolean hasPathSum3 (Node root, int sum) {
        if(root == null) {
            return false;
        }
        Queue<Node> nodes = new LinkedList<Node>();
        Queue<Integer> values = new LinkedList<Integer>();

        nodes.add(root);
        values.add(root.data);

        while(!nodes.isEmpty()){
            Node current = nodes.remove();
            int sumValue = values.remove();

            if(current.left == null && current.right == null && sumValue == sum){
                return true;
            }
            if(current.left != null){
                nodes.add(current.left);
                values.add(current.left.data + sumValue);
            }
            if(current.right != null){
                nodes.add(current.right);
                values.add(current.right.data + sumValue);
            }
        }
        return false;
    }

If recursion has reached a leaf node then either you have accumulated
the correct sum or not, but in no case it is necessary to check the
left or right child node. Therefore I would change the first
implementation slightly to

public static boolean hasPathSum1 (Node root, int sum) {
      if (root == null) {
          return false;
      }
      if (root.left == null && root.right == null) {
          return root.data == sum;
      }
      return hasPathSum1(root.left, sum - root.data) ||
             hasPathSum1(root.right, sum - root.data);
}

If the values of all nodes are non-negative then you can
stop the search if the accumulated sum along a path is greater than the target sum:

public static boolean hasPathSum1 (Node root, int sum) {
      if (sum < 0) {
          return false;
      }
      if (root == null) {
          return false;
      }
      if (root.left == null && root.right == null) {
          return root.data == sum;
      }
      return hasPathSum1(root.left, sum - root.data) ||
             hasPathSum1(root.right, sum - root.data);
}

(and similarly for the iterative versions).

Some advice from the answers to your previous question DFS in Binary Tree
applies here as well:

Better/consistent spacing: if and while is sometimes followed
by a space (good) and sometime not (bad).
Rename the root parameter to node.
Make those methods instance methods of the Node class instead of
static methods.


Ensure continuous date range I have two arrays

dates = ["20161010","20161013"]
values = [5,3]

which represent dates and a view count. To display them in a graph, I also need the values between the dates, which are 0:

["20161010","20161011","20161012","20161013"]
[5,0,0,3]

I came up with a fairly complicated solution:

dates = ["20161010","20161013"]
values = [5,3]
first_date = Date.new(Integer(dates.first[0..3]), Integer(dates.first[4..5]), Integer(dates.first[6..7]))
last_date = Date.new(Integer(dates.last[0..3]), Integer(dates.last[4..5]), Integer(dates.last[6..7]))

# create a continous range
range = (first_date..last_date).to_a.map {|d| d.strftime("%Y%m%d")}
# store the indexes of the original dates inside the new range
date_idxs = dates.map {|d| range.index(d) }

# create a zero filled array with length of our range
filled_values = Array.new(range.length, 0)

values.each_with_index do |value,idx|
  idx_in_filled_values = date_idxs[idx]
  filled_values[idx_in_filled_values] = value
end

puts range # ["20161010", "20161011", "20161012", "20161013"]
puts filled_values # [5, 0, 0, 3]

I think thats not a good solution. Any ideas on how to improve the code? Or a completely different approach?

Several aspects of your code could use improvement:

It would be nice if the code were packaged in a function.
Date.new(Integer(dates.first[0..3]), Integer(dates.first[4..5]), Integer(dates.first[6..7])) is rather verbose and ugly.

I recommend DateTime.strptime for date parsing — it's the inverse of strftime.
You can call Range#map directly; you don't need to do to_a first.
dates.map { |d| range.index(d) } could be problematic for performance, since it is O(n2).

Instead, you should make a Hash.

Here is one solution:

require 'date_core'

DATE_FMT = '%Y%m%d'

def filled_date_series(dates, values, default_value=0)
  data = Hash[dates.zip(values)]
  datetime_range = (DateTime.strptime(dates.first, DATE_FMT) ..
                    DateTime.strptime(dates.last,  DATE_FMT))
  date_range = datetime_range.map { |dt| dt.strftime(DATE_FMT) }
  [
    date_range,
    date_range.map { |d| data[d] || default_value }
  ]
end

dates = ["20161010", "20161013"]
values = [5, 3]
range, filled_values = filled_date_series(dates, values)