Program to delete directories that do not contain a file I'm an intern and have an ETL project I'm doing in python.

Part of it is to make sure after I move files I delete empty directories.

I wrote this code to clean up directories that are empty and in testing it works for me but I want to make sure it won't end up deleting folders full of files.

def folderCleanup(p_rootPath):
    for r, dirs, files in os.walk(p_rootPath):
        for folder in dirs:

            if len(os.listdir(os.path.join(r, folder))) == 0: #Checks if the folders have anything in them
                shutil.rmtree(os.path.join(r, folder))

            elif hasFile(os.path.join(r, folder)): #Checks if a subfolder will have a file in it
                    pass
            else:
                    shutil.rmtree(os.path.join(r, folder),ignore_errors=True)
            #end if
        #end for
    #end for
#end def

def hasFile(p_path): #Returns true if the given directory or any of its children directories have a file in them
    for r, d, f in os.walk(p_path):

        if len(f) > 0:
            if len(f) == 1 and f[0] == 'Thumbs.db':
                continue
            else:
                return True
            #end if
        #end if
    #end for
    return False
#end def

I am just looking for some feedback and whether there are any potential problems with this code.

I didnt check the correctness of code, but...

Small tips on comments:

Don't write #end if. You have identation. Why do you need it?
Comments for functions should look like

def hasFile(p_path):
    """Returns true if the given directory or any of its children directories have a file in them"""
    for r, d, f in os.walk(p_path):

https://saddlebackcss.github.io/tutorials/python/basic/2016/01/14/python-basics-4

Methods should be called like folder_cleanupand has_file. See https://visualgit.readthedocs.io/en/latest/pages/naming_convention.html

Do not name variables like this for r, d, f in os.walk(p_path):. Make names more meaningfull

Try to reduce nesting, may be  using early return


Check if sum can be found in array Given an array a, check to see if z numbers inside a total up to n.
If a = [1,2,3], n=3 and z=2, then this function should return true, since z numbers in that array combine to equal n.

I recently took a test for a job to fill out this function, and this was my answer. It was presumably rejected by the robots for being too slow, since one of the tests terminated with a timeout error.
var isSumPossible = function isSumPossible(a, n, z) {
  return +a.some(function (val, ix) {
    return z === 1 ? val === n : isSumPossible(a.slice(ix - 1, ix).concat(a.slice(ix + 1)), n - val, z - 1);
  });
};

How can this code be optimized to make it faster?

Wow, that is slow! Those array concatenations will completely kill performance. The canonical solution is to make a dictionary of each possible combination of y=z/2 values, then check every combination of z-y values against the dictionary. This has complexity of (K choose (z+1)/2)).


Python, given a word, generate a list with one letter changing for each letter in alphabet Here is my problem:

  Return the number of times that the string code appears anywhere in the given string, except we'll accept any letter for the d, so cope and cooe count.

I solved it by creating a list with all 26 possible co_e combinations and then used two for loops to iterate through the list and search for matches in the given string.

Is there another way I could have created that list? or is there a way to ignore the third character in word and only search for matching characters 1, 2 and 4?

Here is my solution:

if len(inputstring)<4:
    return 0
wordlist=['coae','cobe','coce','code'...etc]
count=0

for i in wordlist:
    for j in range(len(inputstring)):
        if inputstring[j:j+4] == i:
            count+=1
return count

Just use regular expressions!

import re
# creating regular expression with "any" letter at 3 position
r = re.compile("co[a-z]e")
# and taking all the matches:
matches = r.findall("some of my code, nope to come, cofe and cole, or cope and dome, rope and hope!")
count = len(matches)
print(count)


Implementing a generic Queue in C I wrote a generic Queue that could with work any data type you give it. The Queue can do any the basic operations that you would expect a Queue can do such as enqueue, dequeue, peek and so on.

I would like you to critique me on:

My general style
My ability to properly deallocate memory so that there is no memory leaks
Properly handling memory allocation failure
Anything else that you would like to add

Queue.h

#ifndef QUEUE_H_INCLUDED
#define QUEUE_H_INCLUDED

typedef struct Node
{
  void *data;
  struct Node *next;
}node;

typedef struct QueueList
{
    int sizeOfQueue;
    size_t memSize;
    node *head;
    node *tail;
}Queue;

void queueInit(Queue *q, size_t memSize);
int enqueue(Queue *, const void *);
void dequeue(Queue *, void *);
void queuePeek(Queue *, void *);
void clearQueue(Queue *);
int getQueueSize(Queue *);

#endif /* QUEUE_H_INCLUDED */

Queue.c

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include "Queue.h"

void queueInit(Queue *q, size_t memSize)
{
   q->sizeOfQueue = 0;
   q->memSize = memSize;
   q->head = q->tail = NULL;
}

int enqueue(Queue *q, const void *data)
{
    node *newNode = (node *)malloc(sizeof(node));

    if(newNode == NULL)
    {
        return -1;
    }

    newNode->data = malloc(q->memSize);

    if(newNode->data == NULL)
    {
        free(newNode);
        return -1;
    }

    newNode->next = NULL;

    memcpy(newNode->data, data, q->memSize);

    if(q->sizeOfQueue == 0)
    {
        q->head = q->tail = newNode;
    }
    else
    {
        q->tail->next = newNode;
        q->tail = newNode;
    }

    q->sizeOfQueue++;
    return 0;
}

void dequeue(Queue *q, void *data)
{
    if(q->sizeOfQueue > 0)
    {
        node *temp = q->head;
        memcpy(data, temp->data, q->memSize);

        if(q->sizeOfQueue > 1)
        {
            q->head = q->head->next;
        }
        else
        {
            q->head = NULL;
            q->tail = NULL;
        }

        q->sizeOfQueue--;
        free(temp->data);
        free(temp);
    }
}

void queuePeek(Queue *q, void *data)
{
    if(q->sizeOfQueue > 0)
    {
       node *temp = q->head;
       memcpy(data, temp->data, q->memSize);
    }
}

void clearQueue(Queue *q)
{
  node *temp;

  while(q->sizeOfQueue > 0)
  {
      temp = q->head;
      q->head = temp->next;
      free(temp->data);
      free(temp);
      q->sizeOfQueue--;
  }

  q->head = q->tail = NULL;
}

int getQueueSize(Queue *q)
{
    return q->sizeOfQueue;
}

TestQueue.c

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

int main()
{
    int val;
    Queue q;

    queueInit(&q, sizeof(int));

    for(val = 0; val < 10; val++)
    {
        enqueue(&q, &val);
        printf("The value %d has been enqueued.\n", val + 1);
    }

    printf("\n");

    queuePeek(&q, &val);

    printf("The value that is at the front of the queue is %d\n\n", val + 1);

    while(getQueueSize(&q) > 0)
    {
        dequeue(&q, &val);
        printf("%d has been dequeued.\n", val + 1);
    }

    return 0;
}

Output

Pattern-less function names.  For Queue.h, rather see function named  obviously make sense together like Queue_Init(), Queue_GetSize() than queueInit(), getQueueSize(), etc.
Comments with # preprocessing may not be portable

// #endif /* QUEUE_H_INCLUDED */
#endif

There is no need for a head and tail.  Alternative, only store tail and have the tail point to the head of the list.  End of list detected when p->next == tail->next.  This makes your head node one field smaller.  Important if code uses lots of queues.
Unclear why code uses int for the queue size type.  A signed type is not needed (could use unsigned) and on a system where size_t could be much wider than int, a queue size like size_t is more prudent.  Robust code would check for a queue exceed the max value of size in enqueue().
Good use of size_t for memSize.  Good error checking for malloc().  Good to have test case.  IMO, a commented sample usage in the *.h file is nice. ; the *.h being the public interface to your good code.
Pedantic: Robust would check for memSize==0 in queueInit() as that negates the correctness of the malloc() checks which should be if(newNode->data == NULL && q->memSize > 0).
A little documentation goes a long way.  suggest a line or two of comment preceding each function declaration in Queue.h.
Functions like queuePeek(Queue *q, void *data) that do not alter *q should be declared queuePeek(const Queue *q, void *data).  This self documents the unchanging nature of q in the function to users and allows some optimizations a compiler may not otherwise employ.  It is a check on the implementation of the function too.
For completeness, suggest q->memSize = 0 in clearQueue().
Change #include order.  Put "Queue.h" first as a check that Queue.h does not depend on the 3 <*.h> include files - unless that .h file is coded in Queue.h.

#include "Queue.h"
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
// #include "Queue.h"

Indicate failure.  If void dequeue(Queue *q, void *data) does not have anything in the queue to copy to data, there is no indication of that here.  Perhaps return true/false indicating success.  Same for queuePeek().
For debugging,  zero filling memory before free() I have found useful.  Errant code tends to fail faster with a 0 pointer/data than with its original data still potentially intact.  Faster failing code is easier to debug. YMMV.
Opinion:  Storing the queue size is of dubious value, unless of course that is the reason for the queue type - one with a quick size report.  Alternative, drop the size field and calculate when needed.  More often, I have found the need for bool Queue_IsEmpty(const Queue *q) sufficient than needing a quick size and prefer to drop the ever present size field.


Creating CAPTCHA in ColdFusion then showing image in Vue.js I am trying to create a CAPTCHA image on ColdFusion. ColdFusion will be producing REST services and hence will have to push the image via JSON. I want the image to be self contained. I don't want to expose any of the servers paths, so I am going to encode the image and push that via JSON. Last but not least, Vue.js will be used to show the image on the page.

Note: I have omitted the AJAX part for clarity.

<cfscript>
  // This is ColdFusion
  tempFile = "ram:///myImage.txt";

  myImage = ImageCreateCaptcha(100, 300, "Blackcat", "high");

  ImageWriteBase64(myImage, tempFile, "png",true, true);

  myfile = FileRead(tempFile);
  FileDelete(tempFile);
</cfscript>

<div id="image">
   <img :src="captcha">
</div>

<script>
  // this is JavaScript

  data = { 'captcha' : <cfoutput>'#myfile#'</cfoutput> };

  new Vue({
      el: '#image',
      data : data
      });
</script>

Can this process be improved?

Be careful with storing intermediate data to disk, including RAM disk. Because it is shared by the whole server, it is even worse than a global variable! The data you write may not be the data you read.


Static variable in method vs private class property I have a class with a method that will be called multiple times over an object's lifetime to perform some processing steps. This method operates on a mixture of immutable (does not change over the lifetime of the process) data and data that is passed as an argument. The immutable data is comparatively expensive to calculate, so I would like to cache it.

class Sample
{
    public function process($data)
    {
        $immutable = $this->getImmutable();
        $this->processImplementation($immutable, $data); // not interesting
    }
}

How should getImmutable be implemented?

Option #1 would be

public function getImmutable()
{
    static $cache;
    if ($cache === null) {
        $cache = "not interesting";
    }

    return $cache;
}

Option #2 would be

private $_cache;

public function getImmutable()
{
    if ($this->_cache === null) {
        $this->_cache = "not interesting";
    }

    return $this->_cache;
}

Option #2 is of course better OOP, but what I like about option #1 is that the "implementation detail" $cache is physically close to the only place where it is used. This means that it doesn't increase the mental load of someone reading the code unless that someone decides to read the body of getImmutable, in which case the implementation detail has become important.

In my mind, this purely practical argument is stronger than theoretical purisms.

I am also aware that the static version shares the cache across all instances of the class, which option #2 does not (and that's a good thing). However this is not an issue because

no more than one instance of the class will be created per process
PHP is not multithreaded so the shared cache will not be a problem even when unit testing

Is there some other argument for option #2 that could tip the scales?

I'd go with Option #3: extract $cache and getImmutable to a new class whose sole responsibility is providing the expensive data. Whether it caches it and how should be up to the new class. This new class will look just like Option #2 but be separated from the existing class that needs the data.

This provides encapsulation and increases testability.

Update

The ideal design that allows maximal flexibility must always be weighed against the current needs. If the former may give you 4 degrees of awesomeness but you only need 1 in the current application, you are probably better off going with a design that satisfies the latter until you need those extra features.

Encapsulation doesn't need to be applied at the class level in every instance. You can apply it at the method level until you need something more complex.

What would be more complex? One controller needs the data as a PHP model while another needs the JSON, but both may be required multiple times. In this case you'll get gains by caching each form separately. Now you'll want to separate the code to produce and transform the various data representations where the JSON producer can reuse the data-model producer.

By making the JSON controller depend on the JSON producer alone, that producer is free to reuse the data-model producer, hit the database directly, or use test data provided in a fixture for unit tests. Now you're ready to separate all these concerns.

Until then, however, a static variable hidden inside a controller method is sufficient for your needs while providing a basic level of encapsulation.

In other words, go with Option #1 and call it a day.


Is this proper usage of "safe publication"? With regards to Safe Publication, consider this piece of code:

public class Test {
    public static void main(String args[]) {
        ThreadB t = new ThreadB();
        t.start();
        t.Grab(new Bag("HI"));
    }
}

class Bag {
    private String item; // <-- no final here

    public String Item() {
        return item;
    }

    public Bag(String item) {
        this.item = item;
    }
}

class ThreadB extends java.lang.Thread {
    private Bag bag;

    public void Grab(Bag bag) {
        this.bag = bag;
    }

    @Override
    public void run() {
        try {
            while (true) {
                if (bag != null) {
                    System.out.println(bag.Item());
                }
            }
        } catch (Exception e) {
            e.printStackTrace();
        }

    }
}

Is it guaranteed that thread B sees the item ("HI") in bag,

Or is it true that thread B may in fact see the item in the bag as null, and will never see the item "HI" in the bag?

Chapter 3.5. Safe Publication in Java Concurrency in Practice contains a very similar example. To cut a long story short: it's not thread safe. ThreadB could see its bag reference as null. Furthermore, ThreadB could see its bag reference as not null, but the referenced Bag's item field could be seen as null by ThreadB.

On my machine sometimes it's an endless loop but I can't reproduce the second scenario when just the Bag.item is null.

Please note that making the Bag.item field final does not make it thread-safe, access to ThreadB.bag also have to be synchronized:

public class ThreadB extends java.lang.Thread {
    private Bag bag;

    public synchronized void setBag(Bag bag) {
        this.bag = bag;
    }

    @Override
    public void run() {
        try {
            while (true) {
                synchronized (this) {
                    if (bag != null) {
                        System.out.println(bag.getItem());
                        break;
                    }
                }
            }
        } catch (Exception e) {
            e.printStackTrace();
        }

    }
}

Or you can use an AtomicReference:

public class ThreadB extends java.lang.Thread {
    private final AtomicReference<Bag> bag = new AtomicReference<Bag>();

    public void setBag(final Bag bag) {
        this.bag.set(bag);
    }

    @Override
    public void run() {
        try {
            while (true) {
                Bag bag = this.bag.get();
                if (bag != null) {
                    System.out.println(bag.getItem());
                    break;
                }
            }
        } catch (Exception e) {
            e.printStackTrace();
        }

    }
}

Some other notes:

1,

public String Item() {
    return item;
}

From Code Conventions for the Java Programming Language:

  Methods should be verbs, in mixed case with the first letter
  lowercase, with the first letter of each internal word capitalized.

So, this method should be called getItem:

public String getItem() {
    return item;
}

2, According to the same document, chapter File Organization, methods should appear after constructors:

public class Bag {
    private String item;

    public Bag(String item) {
        this.item = item;
    }

    public String getItem() {
        return item;
    }
}


FizzBuzz implementation The common FizzBuzz implementation I saw is using a check for % 15 for printing "FizzBuzz"

Would you let me know if there is anything wrong / better with this approach?

public class FizzBuzz {
    public static void main(String[] args) {
        for (int i = 1; i <= 100; i++) {
            boolean fizzOrBuzz = false;
            if (i % 3 == 0) {
                System.out.print("Fizz");
                fizzOrBuzz = true;
            }
            if (i % 5 == 0) {
                System.out.print("Buzz");
                fizzOrBuzz = true;
            }

            if (fizzOrBuzz) {
                System.out.println();
            } else {
                System.out.println(String.valueOf(i));
            }
        }
    }
}

Let's compare your version to the % 15 version:

public class FizzBuzz {
    public static void main(String[] args) {
        for (int i = 1; i <= 100; i++) {
            if (i % 15 == 0) {
                System.out.println("FizzBuzz")
            } else if (i % 3 == 0) {
                System.out.println("Fizz");
            } else if (i % 5 == 0) {
                System.out.println("Buzz");
            } else {
                System.out.println(String.valueOf(i));
            }
        }
    }
}

The % 15 version is simpler and easier to read. This version neatly delineates the problem into the 4 different cases, and handles each case. In contrast, your version introduces a boolean logic flag (which I consider to be a significant anti-pattern) and a not entirely intuitive dependence on the order of the if statements.


The perfect PHP login script I posted this same question on StackOverflow but guys over there told me to post it here.

I have programming in PHP since past 2 years and yet haven't found a perfect script for logging the user in. It's very messy when it comes to deciding whether to use Session or Cookies or both. What value to assign to the Session ID? Whether to store that value in MySQL table or not. How to auto-login the user? And so on...

I've been using this code for few months. Please tell me whether this is fine.

This is the main signin function.

<?php
    //this function is called after sanitizing the user-input
    //$u is username and $p is password
function signin($u,$p) {
    global $conn, $current_user, $signin_stat, $incorrect;
    $q_string="SELECT uid FROM users WHERE (emailid='$u' OR uname='$u') AND AES_DECRYPT(pass,CONCAT(uname,'$p'))='$p'";
    $q=$conn->query($q_string);
    if($q->num_rows==1) {
        $c=$q->fetch_row();
        $uid=$c[0]; //getting uid
        $current_user=new User($uid); //creating $current_user object

        set_session($uid); //setting session

        $signin_stat=1;

        return true;
    } else {
        signout(0);
        $incorrect=1;
        return false;
    }
}
?>

This is the set_session() function :

<?php
function set_session($uid) {
    global $conn;
    $sid=sha1($uid+time()+$_SERVER['REMOTE_ADDR']);
    $q_string="UPDATE users SET sid='$sid' WHERE uid=$uid";
    $q=$conn->query($q_string);
    $_SESSION['user_session']=$sid;
    if($q) return true; else return false;
}
?>

This is the auto_signin script :

<?php
if(isset($_SESSION['user_session'])) {
    $sid=$_SESSION['user_session'];
    global $conn, $current_user;
    $q_string="SELECT uid FROM users WHERE sid='$sid'";
    $q=$conn->query($q_string);
    if($q->num_rows==1) {
        $c=$q->fetch_row();
        $uid=$c[0];
        $current_user=new User($uid);
        $signin_stat=1;
        $current_user->update_online_stat();
    } else {
        signout(0);
    }
}
?>

This is the signout() function

<?php
function signout($uid) {
    global $conn, $current_user, $signin_stat;
    $sid='NULL';
    $q_string="UPDATE users SET sid='$sid' WHERE uid=$uid";
    $q=$conn->query($q_string);

    $signin_stat=0;
    $current_user=0;

    $_SESSION['user_session']='';
    unset($_SESSION['user_session']);
    unset($_POST);
    session_destroy();
}
?>

notes :
$conn is the mysqli object

$current_user is the user object

sid is the field in mysql users table which holds the current session id of the respective user.

Is this script fine or would you suggest some changes?

Or is this completely crap and needs to be written again from scratch?

Please help me out.

I'm no security expert, not even close, but I can tell you what I use and have observed. I only use cookies to remember the username. Cookies aren't secure so you want to limit what you save in them. I also use sessions to create persistence by passing the user object on. Is this the right way? I don't know, but it has served me well and does not contradict anything I have read on the subject. As for auto-login, you can do something similar to what SO does. You can create a client side cookie with a unique ID and pair it to an account. If the login computer has that cookie then you can have it automatically login. An added security feature might be to also match IP addresses. Not sure if this is exactly what SO does, but its similar. Again no security expert, so I would gather more opinions and do your research.

If you are going to provide comments about a function then you should use doccomments as they are more useful in that IDEs can recognize them and provide them as auto-complete hints. Additionally, I would not use single letter variables except for incrementals or throwaways. The abstraction can lead to confusion if you are not careful and it makes it more difficult to tell what is going on at a glance.

/** this function is called after sanitizing the user-input
 *
 * @param string $user is username
 * @param string $pass is password
 */

As tsabz pointed out, you should never use globals. They are evil and have no place in this world. Help banish these foul demons by forgetting you ever heard of them. Especially don't use globals for something as important as a database connection. It is insecure. If you must use a variable in a function, then inject that variable into the function via the argument list. In this case that would add a very large list, so you might instead want to compact those values into an array and inject the array instead. However, I'll point out in a few that some of these variables are unnecessary, which will decrease the size of said list.

If you are returning from an if statement, then there is no reason to have an else statement, the else is implied. I would remove it to reduce the amount of indentation, however that is really a preference point and entirely up to you.

if( $q->num_rows == 1 ) {
    //etc...
    return TRUE;
}

//etc...
return FALSE;

There are a couple of different things you can do instead of using $current_user as a global. The easiest way would be to convert it to a session variable so that it can be shared. Another, more practical way would be to return the user object instead of a boolean and then later persist it if necessary. This later method makes sense for a number of reasons. First, it goes along with what the function says it is doing. By signing in I expect either a user object or a failure, not a TRUE/FALSE. Second, it allows you to do away with $signin_stat and $incorrect which were both redundant. You could accomplish the same thing with the boolean return (to illustrate: if( signin() ) { $signin_stat = 1; }). Third, it allows you to change how you persist the user object so you are not reliant on sessions.

$_SESSION[ 'current_user' ] = new User( $uid );
//or
if( $q->num_rows == 1 ) {
    //etc...
    return new User( $uid );
}

//etc...
return FALSE;

Always, always, always use braces. I know it is a feature and you can get away with it, but it can be confusing and can cause issues, especially if you combine two statements on one line like that. Not that an if/else statement is even necessary here. If you want to return a boolean value based on the return of an expression, just return the expression. Here it is a little different because there is no comparison to make this a true expression. Instead you are relying on PHP to automatically convert it to a boolean for the comparison. So you might want to typecast it in the return to get a true boolean.

//braces
if( $q ) {
    return TRUE;
} else {
    return FALSE;
}
//equivalent to above
return ( bool ) $q;

Some of this code is violating the "Don't Repeat Yourself" (DRY) Principle. As the name implies, your code should not repeat itself. If you look at your login function and compare it to your auto-login function you should be able to see the similarities. You could create a new function that accomplishes the shared task, but because you are using regular functions this would make bypassing the normal signin function a security concern. I don't really see a way around this, except to convert to using a class, which is something you might want to consider anyways. Many of the issues I pointed out above would be solved much easier in a class. For instance, you could convert those globals to class properties to the same effect. Then there is also the encapsulation that will allow you to hide properties and methods, such as the $conn property and shared login method I was mentioning. I'm not trying to pressure you into using a class, but I am suggesting you give it a look. Hope this helps.


Directory listing model I need a class to read and store directory structure. This is my first step in Objective C so I ask to you to say what you think.

Header file:

#import <Foundation/Foundation.h>

@interface FileStructureRepresentation : NSObject

- (id)initWithPath:(NSString *)path;
-(NSMutableDictionary*) parse;

@end

Code:

#import "FileStructureRepresentation.h"

@implementation FileStructureRepresentation

NSFileManager *fileManager = nil;
NSString *startPath;

- (id)initWithPath:(NSString *)path
{
    self = [super init];
    if (self) {
        fileManager = [NSFileManager defaultManager];
        startPath = path;
    }
    return self;
}

-(NSMutableDictionary *) parse {
    return [self parseDirectory:startPath];
}

-(NSMutableDictionary *) parseDirectory:(NSString *)path {
    NSMutableDictionary *result = [[NSMutableDictionary alloc] init];
    NSMutableArray *collectionOfFilesAndDirectories = [[NSMutableArray alloc] init];
    NSError *error = nil;
    NSArray *listOfFiles = [fileManager contentsOfDirectoryAtPath:path error:&error];

    if(error.code != 0) {
        NSLog(@"Error code:%d, Description:%@", error.code, error.description);
    }

    for(NSMutableString *s in listOfFiles) {
        NSString *temporaryPath = [NSString stringWithFormat:@"%@/%@", path, s];
        NSDictionary *fileAttributes = [fileManager attributesOfItemAtPath:temporaryPath
                                                                     error:&error];
        NSString *fileType = [fileAttributes valueForKey:NSFileType];
        if([fileType isEqualToString:@"NSFileTypeRegular"])
            [collectionOfFilesAndDirectories addObject:s];
        else
            [collectionOfFilesAndDirectories addObject:[self parseDirectory:temporaryPath]];
    }

    [result setObject:collectionOfFilesAndDirectories forKey:path];

    return result;
}

@end

Example:

FileStructureRepresentation *fileStructure = [[FileStructureRepresentation alloc] initWithPath:@"/Users/demas/temporary/tst"];
NSMutableDictionary *structureModel = [fileStructure parse];

I usually think that a mutable array or dictionary as a return value is a bad thing, unless there is a good reason. By this I mean the promise of the API, not the actual value returned. I don't see a reason that the value returned must be mutable, so if you're not planing to modify the return value often I'd change the return type of parse to NSDictionary (you can still return the mutable dictionary in its implementation, that's OK; if you'd like to feel save do return [result copy];).
Since your class actually only has just one method, and that can be made generic, I'd either turn that method into a class method or turn it into a free function altogether (unless you plan to add more functionality, of course). Like:

@interface FileStructureRepresentation : NSObject
+ (NSDictionary *)fileStructureRepresentationForPath:(NSString *)path;
@end

// or even

NSDictionary *FileStructureRepresentationForPath(NSString *path);

-[NSFileManager contentsOfDirectoryAtPath:error:] returns an array of NSString objects, so it should be for(NSString *s in listOfFiles).
When you detect an error (if(error.code != 0)) you should return. I'd return nil. A better way to check for error here would be if(error != nil) since the error code 0 might be a valid value in certain error domains.
In your if([fileType isEqualToString:@"NSFileTypeRegular"]) block you are not using braces, even though you do use them for if(error.code != 0) (both have only one line bodies). I'd make sure they're consistent, but especially with respect to goto fail I recommend to always use braces unless you have the if and the body on the same line. It's a matter of taste, though.
Instead of [fileAttributes valueForKey:NSFileType] you really want [fileAttributes objectForKey:NSFileType] or even simply fileAttributes[NSFileType]. See Difference between objectForKey and valueForKey?.
This is minor: instead of doing [NSString stringWithFormat:@"%@/%@", path, s] it's more explicit and "correct" to do [path stringByAppendingPathComponent:s].
You're not evaluating and handling the error from your call to [fileManager attributesOfItemAtPath:temporaryPath error:&error];. If you don't plan to, just pass NULL instead of &error. Otherwise, the question is how to react to the error, that's up to you and your requirements: maybe just skip the entry: if (error != nil) { /* Clear error for next iteration. */ error = nil; continue }


Count pairs with given sum I want to find the number of the pairs in an array that equal a given sum.

I've naively tried to implement brute force as a solution, but it is too slow and this task should not take more than one second.

def getPairsCount(numbers, shouldEqualTo):
    count = 0
    for i in range(len(numbers)):
        for j in range(i + 1, len(numbers)):
            if numbers[i] + numbers[j] == shouldEqualTo:
                count += 1
    return count

If you can use extra space :

# O(n) running time / O(n) memory
def get_pair_count(nums, target_sum):
    count = {}
    for num in nums:
        count[num] = count.get(num, 0) + 1

    total_double = 0
    for num in nums:
        complement = target_sum - num
        if complement in count:
            total_double += count[complement]
            if complement == num:
                total_double -= 1
    return total_double // 2

source : http://www.geeksforgeeks.org/count-pairs-with-given-sum/

If you can't use more space you could try this version I just made (at your own risk)

 # O(n log n) running time / O(1) memory
def get_pair_count_no_extra_memory(nums, target_sum):
    nums.sort()
    start = 0
    end = len(nums) - 1
    total = 0

    while start < end:
        current_sum = nums[start] + nums[end]
        if current_sum == target_sum:
            start_count = 1
            end_count = 1
            special_case = False
            if nums[start] == nums[end]:
                special_case = True

            while start+1 < len(nums) and nums[start] == nums[start+1]:
                start_count += 1
                start += 1
            while end-1 >= 0 and nums[end] == nums[end-1]:
                end_count += 1
                end -= 1

            if special_case:
                total += ((start_count - 1) * start_count) // 2
            else:
                total += start_count * end_count
            start += 1
            end -= 1
        elif current_sum < target_sum:
            start += 1
        else:
            end -= 1

    return total


Write a Kotlin-function isPrimeNumber Exercise:
Write a prime number-test isPrime(num: Int), which for integer m >= 2 checks, if the integer is a prime number or not.
My solution:
fun main(args: Array<String>) {
    var isPrime: Boolean = false

    for (i in 2..101) {
        isPrime = isPrime(i)

        if (isPrime) {
            println("$i => ${isPrime(i)}")
        }
    }
}

fun isPrime(num: Int): Boolean {
    val upperLimit = num / 2;
    var i = 2

    while (i <= upperLimit) {
        if (num % i == 0) {
            return false
        }
        i++
    }

    return true
}

Could my solution become improved concerning efficiency?

Two easy improvements
Check up to the square root of n rather than n/2.  So for 101 you only need to check n up to 10 not 50. If your number isn't prime then 1 of it's factors must be less than it equal to its square root.
Don't check multiples of 2, so do a single test to see if the number can be divided by 2 then only test odd numbers starting at 3.
So if you test 101 for primeness these's changes mean that instead of testing for divisibilty by  2..50 you only test  2,3,5,7,9
Other things to consider
Use the primes below square root n.  So if you know that 2,3,5,7 are the only primes below square root of 101 you only need to test for divisibilty by these numbers to show 101 is prime.
A more complex solution would be to look at a deterministic version of the Miller Rabin test as described here this works well if you just want to know if a specific value is prime.
Most efficient if you want all primes below n would be a prime number sieve


Remove \n newline from a list of string dumped from github used for a Italian stopwords vocabulary I have a set of Italian words in a txt file hosted on GitHub.
I use it as stopwords vocabulary for further NLP tasks.
When I dump from GitHub it returns me, obviously, also \n
Better or more efficient way to replace the special character compared with my working code?
import urllib.request

stopwords_link = "https://raw.githubusercontent.com/stopwords-iso/stopwords-it/master/stopwords-it.txt"
file_name = urllib.request.urlopen(stopwords_link)

list_nowords = [x.decode("utf-8") for x in file_name]
splitted = [x.split('\n')[0] for x in list_nowords]

As usual I am a self-taught and this is the best place where I can learn, discover new ways and aspects on how to code efficiently with Python

Code is very readable, and that's a good start.
file_name is in fact the file_content; splitted (split) should be just stop_words; stopwords_list is a constant, so by PEP 8, should be uppercase STOPWORDS_LIST.
The problem I see here is that you are decoding each word, when in fact you should decode the whole file, just once.
This is just personal preference, but I tend to always use the requests library, because, in my experience, is always a step ahead of what I need. requests tries to understand the encoding, based on the response, so there should be no need to decode.
A bit of data validation is also needed. The file could have been moved, the network or Github could be down, so it always better to be prepared for the worse. In this case, if the network is down (Connection Timeout) or the file is not there (404) there isn't a big difference: your application should handle the exception and the execution should not proceed.
While a connection timeout would normally raise an exception, a 404 (or any other HTTP error) would still be considered a valid HTTP response, that's why requests has a handy method when you just want 2** responses: raise_for_status.
Then, to remove the \n there is actually the method splitlines() which is exactly what you need.
import requests

STOPWORDS_LIST= "https://raw.githubusercontent.com/stopwords-iso/stopwords-it/master/stopwords-it.txt"

try:
    response = requests.get(STOPWORDS_LIST)
    response.raise_for_status()
    file_content = response.text
    words = file_content.splitlines()
except Exception as e:
    print(f"There was an error: {str(e)}")

This piece of code should put inside a function, so it is clearer what it does and restrict its scope. I'm also adding a type to the function, that can be useful to avoid errors at compile time. Maybe some day you will have stopwords in other languages, so I'm just writing a main as example.
from typing import List

import requests

def download_stop_words(url: str) -> List[str]:
    try:
        response = requests.get(url)
        response.raise_for_status()
        return response.text.splitlines()
    except Exception as e:
        print(f"There was an error: {str(e)}")

if __name__ == '__main__': # this avoids the program being executed when the module is imported
    STOP_WORDS = {
        "it": "https://raw.githubusercontent.com/stopwords-iso/stopwords-it/master/stopwords-it.txt"
    }
    for lang, url in STOP_WORDS.items():
        stop_words = download_stop_words(url)
        print(f"In '{lang}' there are {len(stop_words)} stop words")


Bruteforcing PCP's in Python I am trying to Bruteforce a PCP that is supposed to be up to the length 300.

I wrote a little Python program that is doing just that but it could take forever until my program finds the right PCP.

import sys

x1 = "aba"
x2 = "a"
x3 = "bab"
y1 = "a"
y2 = "ab"
y3 = "abaa"

def ternary(n):
    if n == 0:
        return ''
    else:
        e = n//3
        q = n%3
        return ternary(e) + str(q)

x = 1;

while True:
    ter = ternary(x)
    ter = ("%01d" % (float(ter),))
    #print("ter = " + str(ter))
    wordx = ""
    wordy = ""
    for i in range(0, len(str(ter))):
        if(str(ter)[i] == '0'):
            wordx = wordx + x1
        if(str(ter)[i] == '1'):
            wordx = wordx + x2
        if(str(ter)[i] == '2'):
            wordx = wordx + x3
    for j in range(0, len(str(ter))):
        if(str(ter)[j] == '0'):
            wordy = wordy + y1
        if(str(ter)[j] == '1'):
            wordy = wordy + y2
        if(str(ter)[j] == '2'):
            wordy = wordy + y3

    print(ter)

    if wordx == wordy:
        print("Wordx = " + wordx + " and wordy = " + wordy + " and the decimal number is = " + str(x) + " and the ternary number is = " + str(ter))
        sys.exit("Yes!")
    x+=1

Useful Information about PCP's: https://en.wikipedia.org/wiki/Post_correspondence_problem

Many thing can be improved to make your code faster and clearer:

Multiple useless calls to str

You keep calling str on ter which is a string already. You can replace every occurence of "str(ter)" by "ter".

The 2 loops with (apparently) different loop indices can be written in a single loop

Unless I am missing something, the 2 loops could be written with a single loop.

Loop like a native

I highly recommend watching Ned Batchelder's excellent talk "Loop like a native". In a nutshell, any time you use range and len together, there is a better way to do it.

In your case, after taking the comment above, the code becomes :

for c in ter:
    if c == '0':
        wordx = wordx + x1
    if c == '1':
        wordx = wordx + x2
    if c == '2':
        wordx = wordx + x3
    if c == '0':
        wordy = wordy + y1
    if c == '1':
        wordy = wordy + y2
    if c == '2':
        wordy = wordy + y3

Similar conditions

You have the same conditions evaluated twice. You could group them together.

elif

You could use elif between your mutually exclusive cases.

+=

You could use a += b instead of a = a+b.

Then your code becomes:

for c in ter:
    if c == '0':
        wordx += x1
        wordy += y1
    elif c == '1':
        wordx += x2
        wordy += y2
    elif c == '2':
        wordx += x3
        wordy += y3

Single value provided to %

Instead of builting a tuple with a single value via "%01d" % (float(ter),), you could directly write "%01d" % float(ter).

Loop like a native again

Instead of writing a while True to loop over integers, you could use itertools.count.

import itertools
for x in itertools.count(1):
    ter = "%01d" % float(ternary(x))

divmod

A builtin called divmod does exactly what your are doing in your ternary function.

Final code

Many things can still be improved but here is my current version of the code:

import sys
import itertools

x1 = "aba"
x2 = "a"
x3 = "bab"
y1 = "a"
y2 = "ab"
y3 = "abaa"

def ternary(n):
    if n == 0:
        return ''
    else:
        e, q = divmod(n, 3)
        return ternary(e) + str(q)

for x in itertools.count(1):
    ter = "%01d" % float(ternary(x))
    #print("ter = " + ter)
    wordx = ""
    wordy = ""
    for c in ter:
        if c == '0':
            wordx += x1
            wordy += y1
        elif c == '1':
            wordx += x2
            wordy += y2
        elif c == '2':
            wordx += x3
            wordy += y3

    if wordx == wordy:
        print("Wordx = " + wordx + " and wordy = " + wordy + " and the decimal number is = " + str(x) + " and the ternary number is = " + ter)
        sys.exit("Yes!")


Replacing a sequence of real numbers with another sequence I have a simple task:

  Given a sequence of real numbers, replace all of its members larger
  than Z by Z and count the number of replacements.

I solved this task using Scala, but my code looks like imperative-style code. I want to make my code more functional. How can I rewrite my program so it becomes functional-style code?

def main(args : Array[String]) {
    println("Enter Z number: ")
    val z = readDouble();

    println("Enter the length of sequence: ")
    val lenght = readInt();

    var seq: List[Double] = List();

    // Filling sequence
    for (i <- 1 to lenght) {
      println("Enter sequence item № " + i)
      val item = readDouble();
      seq = item :: seq;
    }

    // number of replacements
    var substituteCount = 0;
    //replacing all numbers larger Z by Z
    seq = seq.map(x => {if (x > z){substituteCount+= 1; z} else x})

    println("Replacement count: " + substituteCount)
    println("New sequence: " + seq.reverse.mkString(" "))
}

Let's split this up a bit; firstly, let's put the sequence filling code in its own function:

def populateSequence(length: Int): IndexedSeq[Double] = {
  for {
    i <- 1 to length
    _ = println("Enter sequence item ? " + i)
    item = readDouble()
  } yield item
}

This utilizes the a combination of for and yield - in this case, this returns an IndexedSeq[Double], but don't worry, you can treat this like a List (or simply convert it to one with a .toList if you really want to.

Our code structure then looks like this:

def main(args: Array[String]) {
  println("Enter Z number: ")
  val z = readDouble();

  println("Enter the length of sequence: ")
  val length = readInt();

  val seq = populateSequence(length)

Ok, where to from here? Well, in this case, instead of modifying substituteCount as we go, I'd rather just calculate it up front:

val substituteCount = seq count (_ > z)

To get our modified List (or IndexedSequence), we can use another for expression. Again, one thing to remember is that both for and if are expressions in Scala, so we can do this:

val modified = for {
  v <- seq
  item = if (v > z) z else v
} yield item

We can then print everything as usual:

println("Replacement count: " + substituteCount)
println("New sequence: " + modified.mkString(" "))


Getting 6 possible URL parameters I am in the midst of writing a simple job board and I have the URL's filtering and working how I want, everything works - I just want to know if I can improve it, if there are any security concerns, and if it is horribly inefficient.

Take the following URL section - foo.com/in/london/at/google/do/design

I assign each section a variable and work out the location, company, and category.

Other use cases that work:

Switching the order - foo.com/at/google/in/london/do/design
Having less parameters - foo.com/in/london/at/google

My code to figure out all these variables is:

$regex = "[^a-zA-Z0-9,-]";
$a = isset($_GET["a"]) ? preg_replace("/".$regex."/", "", $_GET["a"]) : "";
$aa = isset($_GET["aa"]) ? preg_replace("/".$regex."/", "", $_GET["aa"]) : "";
$b = isset($_GET["b"]) ? preg_replace("/".$regex."/", "", $_GET["b"]) : "";
$bb = isset($_GET["bb"]) ? preg_replace("/".$regex."/", "", $_GET["bb"]) : "";
$c = isset($_GET["c"]) ? preg_replace("/".$regex."/", "", $_GET["c"]) : "";
$cc = isset($_GET["cc"]) ? preg_replace("/".$regex."/", "", $_GET["cc"]) : "";

if ($a == "in" && $aa != null) { $searchLocation = $aa; }
if ($b == "in" && $bb != null) { $searchLocation = $bb; }
if ($c == "in" && $cc != null) { $searchLocation = $cc; }

if ($a == "at" && $aa != null) { $searchCompany = $aa; }
if ($b == "at" && $bb != null) { $searchCompany = $bb; }
if ($c == "at" && $cc != null) { $searchCompany = $cc; }

if ($a == "do" && $aa != null) { $searchCategory = $aa; }
if ($b == "do" && $bb != null) { $searchCategory = $bb; }
if ($c == "do" && $cc != null) { $searchCategory = $cc; }

if ($a == "missinghtml") { $errorUrl = true; }

I'm looking at this thinking there must be a better to do this. Is this section secure? Any thoughts on this are much appreciated. Like I say it works, but can it be better?

Your regex could be improved:

$regex = "[^a-zA-Z0-9,-]";
$a = isset($_GET["a"]) ? preg_replace("/".$regex."/", "", $_GET["a"]) : "";

The character , shouldn't be in a URL, but _ can be.
You don't really need to add / to the beginning and end of your regex every time.
I see a lot of people do a-zA-Z when they could really just do a-z with a case insensitive search.

Becomes:

$regex = "/[^a-z0-9-_]/i"
          ^           ^^
$a = isset($_GET["a"]) ? preg_replace($regex, "", $_GET["a"]) : "";

Using if ($a == "in" && $aa != null) could probably be improved also as if $aa is not null, then it evaluates to true anyway:

if ($a == "in" && $aa)

With if ($a == "missinghtml") { $errorUrl = true; }, you should make it into an else if loop

like:

if ($a == "in" && $aa) { $searchLocation = $aa; }
else if ($a == "missinghtml") { $errorUrl = true; }


Geocoding and Reverse in python using Geopy I had done this program that takes an input from STDIN and processes it to check if it is a Latitude/longitude combo or a place's name. Then it uses geocoding and returns an address corresponding to the latitude longitude or returns a latitude and longitude if the input is an address. I am using Nomatim, I would like to know how else can we do this?I would also like to know how I can make the code better.

from geopy.geocoders import Nominatim
import re
geolocator=Nominatim()
arg=raw_input()
pattern=re.compile(r'^(\-?\d+(\.\d+)?),\s*(\-?\d+(\.\d+)?)$')
if pattern.match(arg):
    loc=geolocator.reverse(arg)
    if loc is not None:
        print(loc.address)
        print(loc.latitude, loc.longitude)
    else:
        print "the location doesnt exist"
else:
    loc=geolocator.geocode(arg)
    if loc is not None:
        print(loc.address)
        print(loc.latitude, loc.longitude)
    else:
        print "This address does not exist"

Duplicated code

You should try to follow the Don't Repeat Yourself principle.
In your case, most of the logic is repeated in 2 branches.

You could write :

if pattern.match(arg):
    loc=geolocator.reverse(arg)
else:
    loc=geolocator.geocode(arg)
if loc is not None:
    print(loc.address)
    print(loc.latitude, loc.longitude)
else:
    print "the location doesnt exist"

Alternatively, this could be written using conditional expressions :

loc = geolocator.reverse(arg) if pattern.match(arg) else geolocator.geocode(arg)


Java code that takes user input of Zipcode and turns it into a bar code This program works 100%. If someone could help me just separate it into two classes that would be really great. I have been working on it for a while and I can't seem to figure out how to make it happen. This is the full program that I have so far.

import java.util.Scanner;
public class BarCodeConverter {

public static void main(String[] args) {
    Scanner input = new Scanner(System.in);
    System.out.print("Enter a zip code: ");
    int zip = input.nextInt();
    input.close();
    int checkDigit = checkDigit(zip);

    String barcode = "|";
    barcode = dBarCode(checkDigit) + barcode;
    for (int i = 0; i < 5; i++) {
        barcode = dBarCode(checkDigit) + barcode;
        zip /= 10;
    }
    barcode = "|" + barcode;
    System.out.println(barcode);

}
 public static int checkDigit(int zip) {
    int remaining = zip;
    int sum = 0;
    while (remaining > 0) {
        sum += remaining % 10;
        remaining /= 10;
    }
    return 10 - (sum % 10);
}

public static String dBarCode(int digit) {

    if (digit == 1) {
        return ":::||";
    }
    if (digit == 2) {
        return "::|:|";
    }
    if (digit == 3) {
        return "::||:";
    }
    if (digit == 4) {
        return ":|::|";
    }
    if (digit == 5) {
        return ":|:|:";
    }
    if (digit == 6) {
        return ":||::";
    }
    if (digit == 7) {
        return "|:::|";
    }
    if (digit == 8) {
        return "|::|:";
    }
    if (digit == 9) {
        return "|:|::";
    }
    return "||:::";

  }

}

My two cents:

I did not know about this system, so the methods don't tell me much. checkDigit calculates some kind of validation number? Perhaps name it something like calculateCheckDigit. Same for dBarCode.

Also, you only ever call checkDigit on zip once, and then keep on calling it on the checkDigit variable, although this one never changes? Could it be a bug?

Some other comments:

Close the scanner as soon as possible; it is also AutoCloseable, so you can use try-with-resources: try-with-resources
dBarCode can be implemented with a switch statement, an array of Strings or possibly a Map.
I'd extract the zip to barcode functionality of the main method, without the input/output, to a separate function

I think it is fine to leave the functions in one class, and leave them static.


Custom exception for string that is too long This is my first time working with Java exceptions. I have made a program that works to specifications, but I am wondering if there is anything I've done which is unconventional or not recommended. The application program does specify that there be line breaks between each message.

My driver is:

import java.util.Scanner;
public class ReadStrings {

    public static void main(String[] args) throws StringTooLongException{
        String str1;
        final int MAX_STRING_LENGTH = 20;
        Scanner input = new Scanner(System.in);
        System.out.println("Enter strings, enter DONE when finished:\n");
        str1 = input.nextLine();
        if(str1.equalsIgnoreCase("done"))
            System.exit(0); //exit on first prompt
        do{
            try
            {
                if(str1.length()>MAX_STRING_LENGTH)
                    throw new StringTooLongException();
                System.out.println("\nEnter a string, enter DONE when finished:\n");
                str1 = input.nextLine();
            }
                catch(StringTooLongException e)
            {
                    System.out.println(e.getMessage());
                    str1 = input.nextLine();
            }
        }while(!str1.equalsIgnoreCase("done"));
        input.close();
    }
}

And my custom exception class is:

public class StringTooLongException extends Exception {
    public StringTooLongException()
    {
        super("\nString has too many characters\n\nPlease try again:\n");
    }
}

Thanks for taking a look. Any advice is appreciated.

if(str1.equalsIgnoreCase("done"))
        System.exit(0); //exit on first startup

Do not use System.exit() (somewhere else then in a catch block in main) It terminates the JVM immediately. This is usually not intended.
just write a return instead.

         if(str1.length()>MAX_STRING_LENGTH)
                throw new StringTooLongException();
            System.out.println("\nEnter a string, enter DONE when finished:\n");
            str1 = input.nextLine();

This will result in an endless loop when the user enters a long string. The check is done (and the exception thrown) before she gets a chance to enter a new one.

You should move the if behind the input request and delete the initial input request (along wit the if there) before the loop.

Since you do nothing else in the loop the checking the string and requesting new one some may argue that you use the Exception as flow control for which exceptions are not supposed to be used.
I'd agree to that argument in this particular case as long as the "happy path" also repeats the loop...


Making an email text-parser generic using PredicateBuilder I have some code that reads an email from a text file, parses the text, removes some lines (the 'cc' and 'to' lines) and then finds the line numbers of various phrases (e.g. "Regards", "Thanks") and then removes all the text for the lines after the first phrase found.

It works fine, but is not generic enough as I will have a variable number of phrases to look for to remove text. I want to use PredicateBuilder to add the OR conditions but I am not sure of the syntax to use.

I have attached a cut down version of the code below. The part with 'regards', 'thanks', 'please do not forward' is that part I want to use PredicateBuilder on.

foreach (List<string> list in groups)
{
    IEnumerable<string> lines3 = list.Where(line => !line.StartsWith("Cc:", StringComparison.OrdinalIgnoreCase));
    lines3 = lines3.Where(line => !line.StartsWith("To:", StringComparison.OrdinalIgnoreCase));

    var result = lines3.Select((r, i) => new { line = r, index = i })
                    .Where(r =>
                        r.line.StartsWith("Regards", StringComparison.OrdinalIgnoreCase)
                        || r.line.StartsWith("Thanks", StringComparison.OrdinalIgnoreCase)
                        || r.line.ToLower().Contains("please do not forward")
                        ))
                        .Select(r => new { LineNo = r.index }).OrderBy(r => r.LineNo);

    foreach (var item in result)
    {
        lineNumber = item.LineNo;
        if (lineNumber > 0)
            emailText += String.Join(System.Environment.NewLine, lines3.Take(lineNumber - 1));
        else
            emailText += String.Join(System.Environment.NewLine, lines3);
        break;
    }
}

First of all, you have one too many parentheses and are missing some variable declarations, so let's fix that up.

var emailText = "";
foreach (List<string> list in groups)
{
    IEnumerable<string> lines3 = list.Where(line => !line.StartsWith("Cc:", StringComparison.OrdinalIgnoreCase));
    lines3 = lines3.Where(line => !line.StartsWith("To:", StringComparison.OrdinalIgnoreCase));

    var result = lines3.Select((r, i) => new { line = r, index = i })
                    .Where(r =>
                        r.line.StartsWith("Regards", StringComparison.OrdinalIgnoreCase)
                        || r.line.StartsWith("Thanks", StringComparison.OrdinalIgnoreCase)
                        || r.line.ToLower().Contains("please do not forward")
                        )
                        .Select(r => new { LineNo = r.index }).OrderBy(r => r.LineNo);

    foreach (var item in result)
    {
        var lineNumber = item.LineNo;
        if (lineNumber > 0)
            emailText += String.Join(System.Environment.NewLine, lines3.Take(lineNumber - 1));
        else
            emailText += String.Join(System.Environment.NewLine, lines3);
        break;
    }
}

Now let's run this on

var groups = new List<List<string>>
{
    new List<string> { "Hello", "To: me", "Message body", "Regards", "me" }
};

From your problem description, I would expect the output to be Hello\nMessage body, but it's just Hello. So I'm going to assume the problem description is correct and the code is wrong.

Here are some other notes:

lines3 is not a good variable name.
If you find yourself concatenating a lot of strings, see if you can use a StringBuilder instead.
OrderBy line number is redundant, as the lines are already ordered by line number.

Let's start with a skeleton of what we want the code to look like:

var result = new StringBuilder();
foreach (List<string> email in emails)
{
    // TODO: Remove "to" and "cc" lines.
    // TODO: Remove everything after first "thanks", "regards", etc.
    result.Append(string.Join(Environment.NewLine, ???));
}

Let's handle the easy part first.

private static bool IsDestination(string line)
{
    return line.StartsWith("Cc:", StringComparison.OrdinalIgnoreCase) ||
           line.StartsWith("To:", StringComparison.OrdinalIgnoreCase);
}

var result = new StringBuilder();
foreach (List<string> email in emails)
{
    var filteredEmail = email.Where(line => !IsDestination(line));
    // TODO: Remove everything after first "thanks", "regards", etc.
    result.Append(string.Join(Environment.NewLine, filteredEmail));
}

Now you want a variable number of phrases to look for, and it seems that some will need to be at the start of the line, while others can be anywhere.

We can write the tests like this

var valedictionTests = new List<Predicate<string>>
{
    line => line.StartsWith("Regards", StringComparison.OrdinalIgnoreCase),
    line => line.StartsWith("Thanks", StringComparison.OrdinalIgnoreCase),
    line => line.IndexOf("Please do not forward", StringComparison.OrdinalIgnoreCase) != -1
};

Then you can add tests like this

valedictionTests.Add(line => line.StartsWith("Best,", StringComparison.OrdinalIgnoreCase));

And pass them in as a parameter

var result = new StringBuilder();
foreach (List<string> email in emails)
{
    var filteredEmail = email.Where(line => !IsDestination(line))
        .TakeWhile(line => !valedictionTests.Any(test => test(line)));
    result.Append(string.Join(Environment.NewLine, filteredEmail));
}


A UNIX timestamp microservice I have made a small timestamp microservice that converts from UNIX time to a natural language date, and vice versa. Please let me know of anything I could do better.

Here is a demo. Here is the time.js file that holds the class that controls converting the time:

let debug = require('debug')('test');
let moment = require('moment');

class TimeConverter {
  constructor(){
    this.naturalDate = null;
    this.unixTime = null;
  }
  //check if string meets a specific format
  checkString(string){
    let format = /\w{3,9}?\s\d{1,2}?,\s\d{4}?/;
    let unixFormat = /^\d[0-9]{0,20}$/;
    let test = format.test(string);
    let test2 = unixFormat.test(string);

    if (test === true){
      this.natToUnix(string);

    } else if (test2 === true) {
      this.unixToNat(string);

    } else {
      this.naturalDate = null;
      this.unixTime = null;
    }

    return this.getResults();

  }
  //convert natural language date to unix
  natToUnix(string){
    //put into iso format for moment.js
    let newDate = new Date(string);
    //change into unix time
    let date = moment(newDate).unix();
    this.unixTime = date;
    this.naturalDate = string;

  }
  //change unix time to natural language date
  unixToNat(string){

    let date = moment.unix(string);
    let formatted = date.format("MMMM DD YYYY");

    this.unixTime = string;
    this.naturalDate = formatted;

  }

  getResults(){
    let results = {unix: this.unixTime, natural: this.naturalDate};
    debug(results);
    return results;
  }

}

let time = new TimeConverter();

module.exports = time;

Here is the server.js file that has some express code. It handles the routes in the application. It grabs the input from the request parameters, and serves the CSS / index.html files:

  let express = require('express');
  let debug = require('debug')('test');

  let app = express();

  let bodyParser = require('body-parser');
  let time = require('./time.js');
  let port = process.env.PORT || 8080;

  app.use(bodyParser.json());
  //serves static files
  app.use('/static', express.static('public'));
  app.use(bodyParser.urlencoded({ extended: true}));
  //route for inputting the time
  app.get(['/:time','/'], function(req,res){
    res.sendFile(__dirname + "/index.html");
    let data = req.params;

    if (data.time != undefined){
        res.json(time.checkString(data.time));
    }

  });

  //tell server to listen on a port
  app.listen(port, function(){
    debug("listening on " + port);
  });

Here is the repo.

Design

It's not great that some functions that manipulate internal state are exposed.
It would be better to make them private,
inaccessible from outside the class.

Alternatively, change them in a way that they will not modify the internal state.

Avoid unnecessary evaluation

Here, test2 doesn't need to be evaluated when test is true:

let format = /\w{3,9}?\s\d{1,2}?,\s\d{4}?/;
let unixFormat = /^\d[0-9]{0,20}$/;
let test = format.test(string);
let test2 = unixFormat.test(string);

if (test === true){
  this.natToUnix(string);

} else if (test2 === true) {
  this.unixToNat(string);

I would rewrite like this:

if (this.isNaturalFormat(input)) {
  return this.getResultWithUnixFormat(this.naturalToUnix(input));
}
if (this.isUnixFormat(input)) {
  return this.getResultWithNaturalFormat(this.unixToNatural(input));
}
return this.getResult();

All the functions I invented here, I suggest to write them ;-)
That is, getResultWithUnixFormat takes as parameter a date in unix format, and returns {unix: ?}, and so on.

Use boolean values directly

Since .test(...) returns a boolean, you could use if (test) instead of this:

let test = format.test(string);
// ...

if (test === true){
  // ...

Naming

test and test2 don't tell much about their purpose:

let format = /\w{3,9}?\s\d{1,2}?,\s\d{4}?/;
let unixFormat = /^\d[0-9]{0,20}$/;
let test = format.test(string);
let test2 = unixFormat.test(string);

I suggest to rename them to isNaturalFormat and isUnixFormat.

Also, I would spell out nat as natural in unixToNat and natToUnix.

Thread-safety

It's true that thread-safety is not a practical concern in JavaScript,
as user code is typically executed on a single thread in browsers and in node.js too.
However, I think it's still a good idea to write thread-safe code when it's easily possible, in order to avoid picking up bad habits that may bite you later in other languages.

The implementation is not thread-safe.
checkstring calls functions that mutate the internal state of the instance (naturalDate and unixTime),
so if this function gets called from concurrent threads,
you may get incorrect results.

You can easily make it thread-safe by eliminating the naturalDate and unixTime fields.
You don't need them.
These can be local variables in checkstring,
and passed as parameters to getResults.

In fact, the best would be to replace getResults with dedicated functions that return {unix: ?, natural: ?} with the appropriate value filled,
as I did in the examples above.


Archiving data in file system for iOS Being my first time archiving data to the file system, I would like to have my code reviewed.  This simple test project works as expected, but I want to see if what I am doing is following best practices.  You open the app, put in text for the textfields, press button and strings are saved and displayed in labels.  When app reopens, the saved strings appear in labels.

Cars.h

@interface Car : NSObject <NSCoding>
@property (nonatomic, copy) NSString *year;
@property (nonatomic, copy) NSString *make;
@property (nonatomic, copy) NSString *model;
@end

Cars.m

NSString *const kYearKey = @"YearKey";
NSString *const kMakeKey = @"MakeKey";
NSString *const kModelKey = @"ModelKey";

@implementation Car

-(void)encodeWithCoder:(NSCoder *)aCoder {
[aCoder encodeObject:self.year forKey:kYearKey];
[aCoder encodeObject:self.make forKey:kMakeKey];
[aCoder encodeObject:self.model forKey:kModelKey];
}
-(instancetype)initWithCoder:(NSCoder *)aDecoder {
self = [super init];
if (self != nil) {
    _year = [aDecoder decodeObjectForKey:kYearKey];
    _make = [aDecoder decodeObjectForKey:kMakeKey];
    _model = [aDecoder decodeObjectForKey:kModelKey];
}
return  self;
}
@end

ViewController.m

@interface ViewController () <UITextFieldDelegate>

@property (weak, nonatomic) IBOutlet UITextField *yearTextField;
@property (weak, nonatomic) IBOutlet UITextField *makeTextField;
@property (weak, nonatomic) IBOutlet UITextField *modelTextField;
@property (weak, nonatomic) IBOutlet UILabel *yearLabel;
@property (weak, nonatomic) IBOutlet UILabel *makeLabel;
@property (weak, nonatomic) IBOutlet UILabel *modelLabel;
@property (strong, nonatomic) NSString *yearString;
@property (strong, nonatomic) NSString *makeString;
@property (strong, nonatomic) NSString *modelString;

- (IBAction)doneButton:(id)sender;

@end

@implementation ViewController

- (void)viewDidLoad {
[super viewDidLoad];
// Do any additional setup after loading the view, typically from a nib.

[self loadCarInfo];
}

-(void)loadCarInfo {

NSString *path = [self archivePath];
Car *returnedCarInfo = [NSKeyedUnarchiver unarchiveObjectWithFile:path];

NSString *year = [[NSString alloc] initWithFormat:@"%@", returnedCarInfo.year];
NSString *make = [[NSString alloc] initWithFormat:@"%@", returnedCarInfo.make];
NSString *model = [[NSString alloc] initWithFormat:@"%@", returnedCarInfo.model];

_yearLabel.text = year;
_makeLabel.text = make;
_modelLabel.text = model;
}

-(BOOL)saveCarInfo {

NSString *path = [self archivePath];

Car *myCar = [[Car alloc] init];
myCar.year = _yearString;
myCar.make = _makeString;
myCar.model = _modelString;

//archive the object to the file & return YES on success
return [NSKeyedArchiver archiveRootObject:myCar toFile:path];

}

- (IBAction)doneButton:(id)sender {

_yearString = [[NSString alloc] initWithString:_yearTextField.text];
_makeString = [[NSString alloc] initWithString:_makeTextField.text];
_modelString = [[NSString alloc] initWithString:_modelTextField.text];

BOOL success = [self saveCarInfo];
if (success) {
    [self loadCarInfo];
}
}

-(NSString *)archivePath {

NSArray *documentDirectories= NSSearchPathForDirectoriesInDomains(NSDocumentDirectory,
                                                                 NSUserDomainMask, YES);
//get the document directory from that list
NSString *documentDirectory = [documentDirectories firstObject];

return [documentDirectory stringByAppendingPathComponent:@"savedText.txt"];
}

-(BOOL)textFieldShouldReturn:(UITextField *)textField {

[textField resignFirstResponder];

return YES;
}

@end

The code looks ok. I would just comment on two things:

Why do you keep individual fields as properties in your view controller when you can just retain a reference to a Car object?
You don't need to connect outlets to all labels unless you're planning to change them in the code (localize or customize them for example)


Very basic "sleep calculator" algorithm in C using cs50 library I've been following along with CS50 online and tried my first shot at making a program from scratch and without guidelines. I hope you may be able to rip it to shreds for me.

//
// this program determines how many hours you slept last night.
// it assumes that:
// you're using a 12 hour clock
// you sleep for a maximum of 24 hours
//

#include <stdio.h>
#include <cs50.h>

int main(void)
{   // get user bedtime
    printf("When did you go to bed last night?\n");
    int bedtime = GetInt();
    // determine AM or PM
    printf("AM [1]\nor\nPM [2]\n");
    int b = GetInt();
    // get user wake up time
    printf("When did you wake up this morning?\n");
    int wakeup = GetInt();
    // determine AM or PM
    printf("AM [1]\nor\nPM [2]\n");
    int w = GetInt();
    // interpret inputs and determine how to calculate hours slept
    {if ((b == 1) && (w == 1))
    {
        int slept = wakeup-bedtime;
        printf("You slept for %i hours last night\n",slept);

    }
    else if (((b == 1) && (w == 2)) || ((b == 2) && (w == 1)))
    {
        int slept = 12-bedtime+wakeup;
        printf("You slept for %i hours last night\n",slept);

    }
    else
    {
        int slept = (12-bedtime)+12+wakeup;
        printf("You slept for %i hours last night\n",slept);
    }
    }
}

A few things:

Naming: b and w aren't particularly descriptive.  Even bampm and wampm are better.
You've got extra {} around your if statement for some reason.
Your algorithm does a lot of tests.  What if you convert to 24-hour time?

Doing so will let you get rid of b and w by changing bedtime and wakeup after you read them.

printf("When did you go to bed last night?\n");
int bedtime = GetInt();
// determine AM or PM
printf("AM [1]\nor\nPM [2]\n");
int ampm = GetInt();
// convert to 24-hour time
if (ampm == 2) bedtime = bedtime + 12;
// get user wake up time
printf("When did you wake up this morning?\n");
int wakeup = GetInt();
// determine AM or PM
printf("AM [1]\nor\nPM [2]\n");
ampm = GetInt();
// convert to 24-hour time
if (ampm == 2) wakeup = wakeup + 12;

Also, the conditionals get easier:

int slept = wakeup - bedtime;
if (slept < 0) { slept = slept + 24 } // math is your friend
printf("You slept for %i hours last night\n",slept);

Once principle of programming is "Don't Repeat Yourself" aka "DRY".  Do you really need 3 copies of the output printf?

Of course not.  You can move it after the end of the if statement.
Finally, I don't know if this was covered in class, but you're not doing any input checking.  What if the user puts in 3 instead of 1 or 2 ?  What if they put in times like -5 or 27 ?


Hackerrank : Hash Tables: Ransom Note I just solved the Hash Tables: Ransom Note problem on Hackerrank using both Java-8 and Java-7.  Given m words in a magazine and the n words in the ransom note, print Yes if a kidnapper can replicate his ransom note exactly (case-sensitive) using whole words from the magazine; otherwise, print No.  1 ≤ m, n ≤ 30000.

I am looking for suggestions regarding the efficiency, coding style and if this is the best way to solve this kind of problem.

Java-7

public class RansomeNotesJava7 {

    public static void main(String[] args) throws Exception{
        Scanner in = new Scanner(new File("input.txt"));
        int m = in.nextInt();
        int n = in.nextInt();
        String magazine[] = new String[m];
        for(int magazine_i=0; magazine_i < m; magazine_i++){
            magazine[magazine_i] = in.next();
        }
        String ransom[] = new String[n];
        for(int ransom_i=0; ransom_i < n; ransom_i++){
            ransom[ransom_i] = in.next();
        }

        if(getRansom(m,n,magazine,ransom))
            System.out.println("Yes");
        else
            System.out.println("No");
    }

    private static boolean getRansom(int m, int n, String[] magazine, String[] ransom) {

        if(m < n)
            return false;

        Map<String, Long> magazineMap = getFrequencyMapFromArray(magazine); //Arrays.stream(magazine).collect(Collectors.groupingBy(Function.identity(), Collectors.counting()));
        Map<String, Long> ransomMap =  getFrequencyMapFromArray(ransom); //Arrays.stream(ransom).collect(Collectors.groupingBy(Function.identity(), Collectors.counting()));

        System.out.println(magazineMap);
        System.out.println(ransomMap);

        for(String key : ransomMap.keySet()){
            if(!magazineMap.containsKey(key))
                return false;

            if(magazineMap.get(key) < ransomMap.get(key))
                return false;
        }

        return true;

    }

    private static Map<String, Long> getFrequencyMapFromArray(String[] arr) {

        Map<String, Long> map = new HashMap<>();

        for(String key : arr){
            if(map.containsKey(key))
                map.put(key, map.get(key)+1);
            else
                map.put(key, new Long("1"));
        }
        return map;
    }
}

Java-8

public class RansomNotes {

    public static void main(String[] args) throws Exception {
        Scanner in = new Scanner(new File("input.txt"));
        int m = in.nextInt();
        int n = in.nextInt();
        String magazine[] = new String[m];
        for (int magazine_i = 0; magazine_i < m; magazine_i++) {
            magazine[magazine_i] = in.next();
        }
        String ransom[] = new String[n];
        for (int ransom_i = 0; ransom_i < n; ransom_i++) {
            ransom[ransom_i] = in.next();
        }

        if (getRansom(m, n, magazine, ransom))
            System.out.println("Yes");
        else
            System.out.println("No");
    }

    private static boolean getRansom(int m, int n, String[] magazine, String[] ransom) {

        if (m < n)
            return false;

        Map<String, Long> magazineMap = getFrequencyMapFromArray(magazine);
        Map<String, Long> ransomMap = getFrequencyMapFromArray(ransom);
        System.out.println(magazineMap);
        System.out.println(ransomMap);

        return (ransomMap.entrySet().stream()
                .filter(i -> (!magazineMap.containsKey(i.getKey()) || magazineMap.get(i.getKey()) < i.getValue()))
                .count() == 0);

    }

    private static Map<String, Long> getFrequencyMapFromArray(String[] arr) {

        return Arrays.stream(arr)
                     .collect(Collectors.groupingBy(Function.identity(), Collectors.counting()));
    }

}

Coding style

Overall looks good. I only got some small remarks:

Put {} after each if and else as well. This is mostly to prevent stupid bugs if you (or someone after you) adds an innocent looking line like this:

 for(String key : ransomMap.keySet()){
    if(!magazineMap.containsKey(key))
        System.out.println("Not enough of: " + key);
        return false;  //<- this false is actually outside the if block now.

    if(magazineMap.get(key) < ransomMap.get(key))
        return false;
}

Suddenly it always returns false, even tho we just wanted to print which key wasn't found. Most IDE's can be set up to always place the braces for you.

A minor thing is also the name for magazineMap. I would rename this to magazineFrequencyMap to make it clear that it's a frequency map (like your great method name says to populate the map). But since that might feel a little long later on you'll have to decide for yourself what feels better.

The last thing is that you never close your Scanner. Since this is in your main method and your program ends after execusion it doesn't really matter much, but since we're talking best practices here I couldn't ignore this.

Efficiency

Why store the number of words in a Long if there's at most 30k of them? An Integer should do:

Map<String, Integer> magazineMap = getFrequencyMapFromArray(magazine);

A major change to gain efficiency is to never build the frequency map for the ransom notes. Instead use the following strategy to solve the problem:

Put all the magazine words into a map like you did now.
Try to take each ransom word out of that bag while reading them from the file.
If it's not in the bag (or count is 0) you can stop here and return false

If you are "lucky" and the first ransom word is not present in your magazines you can stop early. This can save "a lot" of time counting the other ransom words. (Note that "a lot" means a couple of miliseconds probably, so it doesn't really matter that much for this problem).

the best way

I would argue that there is no best way to solve a computer problem. Only a better way given certain criterea.

For the given problem I would argue that my "take ransom words out of the existing bag as we go" is better since it's more memory efficient and fails faster.

If, however, we want to expand this problem later to:

  Given a stack of magazines, which of these magazines can be used to write the ransom note?

Then it makes a lot more sense to read the ransom note once and put them into their own frequency map. Because we can use this map to compare with each of the magazine frequency maps like you did in your solution.


Checking for duplicate values in database with Python I'm working on a Python application where I read and extract data from an HTML file. The data is stored in a list, and the number of items in a list is, on average, 50,000+.
The items from the list are added to a database. I cannot allow duplicate datetime values in one of the columns, and so, I have to check if a specific value exists in the database already, and if it does, keep incrementing it until there's no duplicates.

Here's what my code looks like (I'm using SQLAlchemy):

import re
from bs4 import BeautifulSoup
from datetime import datetime, timedelta
from models import db, Notes

def import_file(filename, user):
    file = open(filename, 'r')
    html = file.read()
    file.close()
    soup = BeautifulSoup(html, "lxml")
    for link in soup.find_all('a'):
        validurl = re.compile(
            r'^(?:[a-z0-9\.\-]*)://'
            r'(?:(?:[A-Z0-9](?:[A-Z0-9-]{0,61}[A-Z0-9])?\.)+(?:[A-Z]{2,6}\.?|[A-Z0-9-]{2,}(?<!-)\.?)|'
            r'\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3}|'
            r'\[?[A-F0-9]*:[A-F0-9:]+\]?)'
            r'(?::\d+)?'
            r'(?:/?|[/?]\S+)$', re.IGNORECASE)
        url = validurl.match(link.get('href'))
        if url:
                url = link.get('href')
                if len(url) <= 2000:
                    if link.get('add_date'):
                        date = datetime.utcfromtimestamp(int(link.get('add_date')))
                        while Notes.query.filter_by(user=user, added_on=date).first():
                            date += timedelta(0, 1)
                        title = link.string
                        import_notes = Notes()
                        import_notes.main_url = url
                        import_notes.title = title
                        import_notes.user = user
                        import_notes.added_on = date
                        db.session.add(import_notes)
                        db.session.commit()

However, this code runs very slowly. I calculated how long it was taking per insert, and it came out at exactly 13 seconds per insert. I ran a profiler, and the filter_by query was taking the majority of the time. I figured it was because the script had to run a query so many times per item, and so I changed my code to use a static list instead:

import re
from bs4 import BeautifulSoup
from datetime import datetime, timedelta
from models import db, Notes

def import_file(filename, user):
    file = open(filename, 'r')
    html = file.read()
    file.close()
    soup = BeautifulSoup(html, "lxml")
    for link in soup.find_all('a'):
        validurl = re.compile(
            r'^(?:[a-z0-9\.\-]*)://'
            r'(?:(?:[A-Z0-9](?:[A-Z0-9-]{0,61}[A-Z0-9])?\.)+(?:[A-Z]{2,6}\.?|[A-Z0-9-]{2,}(?<!-)\.?)|'
            r'\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3}|'
            r'\[?[A-F0-9]*:[A-F0-9:]+\]?)'
            r'(?::\d+)?'
            r'(?:/?|[/?]\S+)$', re.IGNORECASE)
        url = validurl.match(link.get('href'))
        if url:
                url = link.get('href')
                if len(url) <= 2000:
                    if link.get('add_date'):
                        date = datetime.utcfromtimestamp(int(link.get('add_date')))
                        while Notes.query.filter_by(user=user, added_on=date).first():
                            date += timedelta(0, 1)
                        title = link.string
                        import_notes = Notes()
                        import_notes.main_url = url
                        import_notes.title = title
                        import_notes.user = user
                        import_notes.added_on = date
                        db.session.add(import_notes)
                        db.session.commit()

I thought this might make the code run faster, but there was little to no change in runtime. For comparison, if I remove the duplicate check, I get ~300 inserts per second.

I cannot let duplicate datetime values into the database. What can I do to make my code faster?

In python checking membership of an item in a set is extremely fast.

I suggest you use sets.


Extracting information and returning specific data The purpose of this code is to return an address for one of the seven judicial circuits for the cases that is passed into the function.

I am taking a Node ID, which is a 3-digit number the circuit number is the first digit and the county number is the last two digits.

Set oNode = XmlDoc.SelectSingleNode("/Record/CelloXml/Integration/Case/Hearing/Court/NodeID")
Dim Addresses() = (
    "Title /n Street Address /n City, State Zip"
    ,"Title /n Street Address /n City, State Zip"
    ,"Title /n Street Address /n City, State Zip"
    ,"Title /n Street Address /n City, State Zip"
    ,"Title /n Street Address /n City, State Zip"
    ,"Title /n Street Address /n City, State Zip"
    ,"Title /n Street Address /n City, State Zip")

Select Case oNode/100
    Case 1
        ReturnData = Addresses(0)
    Case 2
        ReturnData = Addresses(1)
    Case 3
        ReturnData = Addresses(2)
    Case 4
        ReturnData = Addresses(3)
    Case 5
        ReturnData = Addresses(4)
    Case 6
        ReturnData = Addresses(5)
    Case 7
        ReturnData = Adresses(6)
End Select

Am I using the best coding practices and VBScript that I can be using?

Just a couple suggestions. Since VBScript isn't a strongly typed language, I would do a recommend doing more to make sure that you are making it clear what underlying types you are working on. I prefer an explicit Array call syntax for one, but that's a matter of taste more than anything:

Dim sVariable() = Array("foo","bar")

Also make sure you declare Option Explicit (although I can't tell if it is declared or not here - this appears to be a snippet from a longer piece of code). I would also use (shudder) some sort of variable notation to let you keep track variable types. You apparently used one character Hungarian notation for your object - why not extend this to other types?  Just because VBScript treats everything as a Variant doesn't mean you shouldn't keep track of how you are using them.

Next is to make sure that when you are performing casts, you are doing them explicitly.  The line that jumps out is:

Select Case oNode/100

This is implicitly doing 2 things that are non-obvious - it takes an XmlNode object, calls its default method (.Value), and then casts it to an undefined numeric type to perform division on it. While it's fairly obvious what you are doing in this case, this is a habit that can cause all types of problems in longer scripts and can be hard to figure out when debugging or re-visiting the code in a couple months.

Speaking of debugging, I would also add error handling or trapping of some sort. Obviously trapping errors would be better than the sample below because you can give more meaningful error messages - i.e., invalid NodeID.

Finally, you can simplify this quite a bit by just indexing into the array directly:

On Error Goto Ooops:

Set oNode = XmlDoc.SelectSingleNode("/Record/CelloXml/Integration/Case/Hearing/Court/NodeID")
Dim sAddresses() = Array(
    "Title /n Street Address /n City, State Zip",
    "Title /n Street Address /n City, State Zip",
    "Title /n Street Address /n City, State Zip",
    "Title /n Street Address /n City, State Zip",
    "Title /n Street Address /n City, State Zip",
    "Title /n Street Address /n City, State Zip",
    "Title /n Street Address /n City, State Zip")

Dim iIndex = CInt(oNode.Value) / 100
sReturnData = sAddresses(iIndex - 1)

Ooops:
'Fall-through is OK as long as you check for an error condition here.
If Err.Number <> 0 Then
    sReturnData = vbNullString
    'Do some other useful things.
End If


Producer/Consumer in C# Is this code an adequate implementation of the producer/consumer pattern?

In computer science, the producer/consumer pattern is a classic example of multithreaded synchronisation. The problem describes some threads called consumers and some threads called producers sharing a common fixed-size queue.

In the above code, the queue is called _myQueue and it's size is 1000.

Each producer job is to generate some item, put it in the queue by calling AddItem(), and start again.
The consumers job is to remove items from the queue by calling RemoveItem()

The problem is to make sure no producer tries to add items to the queue when it's full (i.e. containing 1000 items) and no consumer tries to remove items from the queue when it's empty.

Also, we'd like to avoid situation where consumers or producers end up waiting for no reason (deadlocks).

private int _count = 1000;
private Queue<string> _myQueue = new Queue<string>();
private static object _door = new object();

public void AddItem(string someItem)
{
    lock (_door)
    {
        while (_myQueue.Count == _count)
        {
            Monitor.Wait(_door);
        }

        _myQueue.Enqueue(someItem);
        Monitor.Pulse(_door);
    }
}

public string RemoveItem()
{
    string item = null;
    lock (_door)
    {
        while (_myQueue.Count == 0)
        {
            Monitor.Wait(_door);
        }

        item = _myQueue.Dequeue();
        Monitor.Pulse(_door);
     }

    return item;
}

Answering whether or not this implementation is correct would require to know what the specification of the problem is. Now assuming the spec is:

  Create a queue to which multiple threads can add items and multiple threads can remove item. The queue will hold no more than 1000 string references and blocks writers when the queue is full and readers when the queue is empty.

then I'd say yes your implementation will achieve that.

Some remarks:

As mentioned by @svick you should seriously consider using BlockingCollection which was added in .NET 4.0.
You can increase the practical usefulness immediately by doing two simple things

Make your class generic. This way you can create a blocking queue for any type not just string.
Let the user pass in the capacity.

Some future speculation in case you are stuck with .NET 3.5 or earlier and want to stick with your implementation: If you are ever tempted to implement IEnumerable<T> (to show the content in a UI for example) on your class then remember that the following implementation is broken (I have seen this more often than I'd like):

public IEnumerator<T> GetEnumerator()
{
    lock (_door)
    {
        return _myQueue.GetEnumerator();
    }
}

The correct way would be to make a local copy of the queue (inside the lock) and return the enumerator to that. The other option is to iterate over the queue inside the lock and yield the items which effectively locks the queue for the entire duration of the enumeration. However this is probably a bad idea since a caller which is not aware of this can easily lock out all access to the queue for a long period of time.


Billboards InterviewStreet - Code Optimization Heres the link to the original question . Billboards Link

I initially declared n*k array which led to a Out Of Memory Exception.

Then I resorted to using just 2 * k matrix and alternating between 0 and 1 during each iteration. Now the issue is time limit. As of now 8/10 cases are solved using this approach.

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace Billboards
{
class Program
{
    static void Main(string[] args)
    {
        long n, k;
        string line;
        string[] parts = new string[2];
        line = Console.ReadLine();
        parts = line.Split(new char[] { ' ' });
        n = long.Parse(parts[0]);
        k = long.Parse(parts[1]);
        long i, j;
        long[]  c = new long[n];
        for (i = 0; i < n; i++)
        {
            c[i] = long.Parse(Console.ReadLine());
        }
        long[][] dp = new long[2][];
        long max = 0;
        int var1, var2, temp;
        var1 = 0;
        var2 = 1;
        for(i=0;i<=1;i++){
            dp[i] = new long[k + 1];
        }

        for (i = 1; i <= n; i++)
        {
            dp[var2][0] = max;
    max = 0;
            for (j = 1; j <= k; j++)
            {
                dp[var2][j] = Math.Max(dp[var1][j - 1] + c[i-1], dp[var1][j]);
        if(dp[var2][j] > max)
        max = dp[var2][j];
            }
    temp = var1;
    var1 = var2;
    var2 = temp;
        }

        max = Max(dp[var1]);

        Console.WriteLine(max);
    }

    static long Max(long[] arr)
    {
        long max = 0;
        for (long i = 0; i < arr.Length; i++)
        {
            if (arr[i] > max)
            {
                max = arr[i];
            }
        }
        return max;
    }
}
}

Since we are on a code review forum I would suggest you to give more meaningful names to variables, avoid redundant assignments (e.g. initial array assigned to parts variable is not used), and split the logic into smaller methods that perform distinct functionality. Also you can use Max(IEnumerable<long>) extension method instead of writing your own.

But I guess that's not what you want to hear, so in terms of algorithms I suggest you to apply the A* algorithm to this problem instead of full search. If you find it difficult to understand and implement, I would recommend you to pass the Machine Learning course from Coursera or Introduction to Artificial Intelligence from Udacity


Memory text based game Would anyone be so kind as to review my code for Memory?

import java.util.*;

public class Cards {

    public boolean gameInProgress = true;

    public void shuffleCards() {

        Scanner userInput = new Scanner(System.in);
        List cardList = new LinkedList();
        List matchedCards = new LinkedList();

        //this is the list of cards available to pick from
        String[] cards = {"a", "b", "c", "d", "e", "a", "b", "c", "d", "e"};

        //this shuffles the cards each game
        Collections.shuffle(Arrays.asList(cards));

        //this moves the cards array into a LinkedList
        Collections.addAll(cardList, cards);

        //main game loop. stops when all cards are matched
        while (gameInProgress) {

            //Stores the users card picks in variables card1 and card2
            System.out.println("pick a card (0-9)\n");

            int card1 = userInput.nextInt();

            System.out.println("First card picked is the letter " + cardList.get(card1));

            System.out.println("pick a second card.\n");

            int card2 = userInput.nextInt();

while(card1 == card2){
            System.out.println("You cannot pick the same card twice. Pick a different card.");
            card2 = userInput.nextInt();
        }

            System.out.println("second card picked is the letter " + cardList.get(card2));

            //checks if a card has been picked already
            while(matchedCards.contains(cardList.get(card1))) {
                System.out.println("First card already picked. Pick again: ");
                card1 = userInput.nextInt();
            }

            while(matchedCards.contains(cardList.get(card2))){
                System.out.println("Second card already picked. Pick again: ");
                card2 = userInput.nextInt();
            }

                //copies the matched cards to a new linked list
                if (cardList.get(card1) == cardList.get(card2)) {
                    System.out.println("You got a match!\n");

                    matchedCards.add(cardList.get(card1));
                    matchedCards.add(cardList.get(card2));

                    System.out.println("You have collected " + matchedCards.size() + "/10 cards");

                    System.out.println(matchedCards + " are in the matched pile\n");

                    //stops the game once all cards have been matched
                    if (matchedCards.size() == 10) {
                        System.out.println("You Win!!!");
                        gameInProgress = false;
                    }
                }
                else {
                    System.out.println("Not a match\n");

                }
        }
    }
}

    public class Game {

    public static void main(String[] args) {

        Cards card = new Cards();

        card.shuffleCards();

    }
}

Review:

public void shuffleCards()

Should only shuffle cards, because that's what it says it does. Create new methods void playGame() int getFirstPick(), int getSecondPick(), boolean isAlreadyMatched(int card1, int card2), boolean areAllCardsMatched() and possibly others.

Assign these methods to the class that they should belong to.

Game
    void playGame()
    int getFirstPick()
    int getSecondPick()
    boolean verifyFirstPick(int firstPick)
    boolean verifySecondPick(int firstPick, int secondPick)

Cards
    void shuffleCards()
    boolean isAlreadyMatched(int card)
    boolean areAllCardsMatched()

Why?

Because right now you've got one long piece of code that does everything, and it's confusing you and causing bugs. By splitting it up, it'll be easier to identify what you need to do and whether the code is doing it.

If you do it right, you'll end up with code like this:

public void playGame(){
    //printInstructions(); //an idea, perhaps?
    setupGame();
    while(!isGameWon()){
        doTurn();
    }
    printGameWon();
}

That's awfully abstract, Pim.

Okay, lets dive a level deeper, in setupGame:

public void setupGame(){
    cards = new Cards();
    cards.shuffle();
    userInput = new Scanner(System.in);
}

That sets up the playing field and the input, it seems.

What's the next method? isGameWon()...

public boolean isGameWon(){
    return (cards != null && cards.areAllCardsMatched());
}

It's just a check for whether all the cards are matched. (There's a sneaky shortcut here; with cards != null I check whether the game has been initialized yet.)

Okay, what about doTurn() then? That one is a bit bigger.

public void doTurn(){
    int firstPick = -1;
    do {
        firstPick = getFirstPick();
    } while(!verifyFirstPick(firstPick));
    printFirstPick(firstPick);

    int secondPick = -1;
    do {
        secondPick = getSecondPick();
    } while(!verifySecondPick(firstPick, secondPick));
    printSecondPick(secondPick);

    checkMatch(firstPick, secondPick);
}

It handles getting the first picked card and the second picked card, then passes them along to checking for a match.

Let's go look at the checkMatch function.

public void checkMatch(int firstPick, int secondPick){
    if(cards.match(firstPick, secondPick)){
        printMatch();
        printRemainingMatches();
    } else {
        printNoMatch();
    }
}

We STILL haven't seen any real code doing anything. All I have shown you right now that actually does anything is the setupGame method. The rest just structures the game flow.

Function lists right now:

Game
    void setupGame();
    void playGame();
    void doTurn();
    void checkMatch(int firstPick, int secondPick);
    int getFirstPick();
    int getSecondPick();
    boolean verifyFirstPick(int firstPick);
    boolean verifySecondPick(int firstPick, int secondPick);
    boolean isGameWon();

    void printFirstPick(int firstPick);
    void printSecondPick(int secondPick);
    void printMatch();
    void printNoMatch();
    void printRemainingMatches();
    void printGameWon();

Cards
    void shuffleCards();
    boolean match(int card1, int card2);
    boolean isAlreadyMatched(int card);
    boolean areAllCardsMatched();

It'll be your task to implement the ones I haven't shown yet. You'll also have to move some of the variables so they're class members, not just declared in a function (hint: cards and userInput are two of these, I don't know if there are more).

Given the name of the functions, it should be easy to guess what they do.
shuffleCards shuffles the cards. match tests if two cards are a match. And so on...