Optimize Collatz conjecture This was the exercise:

  Exercise PSD

  Think about a number under 25. Write down the number. If the number is
  even, divide in to two en write down the result below. If the number
  isn’t even (the number is odd) multiply the number with 3 and add 1.
  Write down this number. Repeat until the answer is 1. Write the code
  for this problem.

  Example:

13    14
40    7
20    22
10    11
5     34
16    52
8     26
4     13
2     40
1     20
      10
      5
      16
      8
      4
      2
      1

How can I optimize this code? For example, is it possible to write it shorter or better performance?

import java.util.Scanner;

public class Main {

    public static void main(String[] args) {
        Scanner sc = new Scanner(System.in);
        int i = sc.nextInt();
        PSD(i);
    }

    public static void PSD(int x) {
        if(x == 1){
            System.out.println(x);
        }
            else if (x % 2 == 0) {
            System.out.println(x);
                PSD((x / 2));
            } else {
            System.out.println(x);
                PSD((x * 3)+1);
            }
        }
    }

Printing in a Function

A function should really only be doing one thing. Yours calculates and prints. This makes it hard to reuse the function, and it also slows it down. Collect the result inside the function (ideally using a StringBuilder for performance reasons), return the result, and print it elsewhere.

Style

Your indentation is off, which makes your code hard to read. You can fix this easily with any IDE.

Duplication

Your else if and else blocks contain some duplication. You could calculate the value you pass to PSD in there, and call System.out.println and PSD outside, only once.

With these points, your code might look like this:

public static String PSD(int x, StringBuilder result) {
    if (x == 1) {
        result.append(x);
        return result.toString();
    }

    int value;
    if (x % 2 == 0) {
        value = x / 2;
    } else {
        value = (x * 3) + 1;
    }
    result.append(x);
    result.append("\n");
    return PSD(value, result);
}

Recursive Functions

Recursive functions generally perform worse than iterative approaches. An iterative approach might look like this:

public static String PSD(int x) {
    StringBuilder result = new StringBuilder();
    while (x != 1) {
        result.append(x);
        result.append("\n");
        if (x % 2 == 0) {
            x /= 2;
        } else {
            x = (x * 3) + 1;
        }
    }
    result.append(1);
    return result.toString();
}


Replacing data in a .csv file The code works well and does what I intend for it to do. In essence, it opens a file referenced as 'resource'. This is a .csv file. I then searches for the keys in the dictionary and for each key that it encounters, it replaces it with the value - the dictionary being 'rep'. It then exports this to a new .csv file.

The above just seems slightly cumbersome and so I just wonder if there is a slicker / more pythonistic way of achieving the above? I think the non-slick part of my code is the writing of the amended data to a new file but I seek any feedback regarding improving my code.

with open(resource,'rb') as f:
   data = f.read()
   rep = {"Incoming Call" : "Voice (in)",
          "Outgoing Call" : "Voice (out)",
          "Incoming SMS" : "SMS (in)",
          "Outgoing SMS" : "SMS (out)"}
   for i, j in rep.iteritems():
      print i, j
      data = data.replace(i, j)
   with open("output.txt", "w") as outputfile:
      outputfile.write(data)
      outputfile.close()

The process you're using is better than trying to read and write the same file, which is just asking for a headache. However there is another way, which will particularly help if you need to use this on a larger file. You can read the file line by line, writing the replaced lines to your output file. This is less memory intensive as only a line is read at a time.

with open(resource,'rb') as f, open("output.txt", "w") as outputfile:
    for line in f:
        for i, j in rep.iteritems():
            line = line.replace(i, j)
        outputfile.write(line)

Note: I'd move the rep declaration out before you open either file, and I removed the superfluous print call now that it's running over every line.

Additionally, I don't think you need a dictionary for your mapping. It seems to semantically make sense, but offers no actual advantage to you. Just a tuple of tuple pairs would work fine, and I'd rename it too:

replacements = (
                ("Incoming Call", "Voice (in)"),
                ("Outgoing Call", "Voice (out)"),
                ("Incoming SMS", "SMS (in)"),
                ("Outgoing SMS", "SMS (out)"),
               )

Also a minor improvement for your loop, you can use the unpacking operator to simplify things a little:

for pair in replacements:
    line = line.replace(*pair)

* is the unpacking operator, it unpacks a 2-item tuple into 2 arguments for you. I prefer this as I find it more readable and makes for a better name (pair rather than i, j).


How to add exceptions in a class that reads a config file? I am using Boost for reading a config file that is in the .json format. I am using a class to do this and I am calling exit(message) in case of bad values or default value gotten by Boost get() function.

The class is here:

class ConfigFile
{
private:
  PTree m_ptConfig;

  void failedGetValue(const std::string& message)
  {
    std::cout << message << std::endl;
    //TODO throw exception
    exit(EXIT_FAILURE);
  }

public:
  ConfigFile(const std::string& name);
  ~ConfigFile();

  double getVal1()
  {
    std::string path = "path.to.val1";
    double val1 = m_ptConfig.get<double>(path, -1);

    if (val1 < 0)
    {
      std::string msg = "getVal1() failed or val1 < 0";
      failedGetValue(msg);
    }

    return val1;
  }
  double getVal2() {/*the same as before*/};
  // ... more values the same way
};

If I want to throw exceptions like MyConfigFileException instead of calling exit:

class MyConfigFileException : public MyIOException // MyIOException is derived from std::exception
{
private:
  std::string m_msg;

public:
  MyConfigFileException(const std::string msg) : msg(msg) {}
  virtual const char* what() const throw()
  {
    return m_msg.c_str();
  }
};

How would you suggest I do this? Shall I just throw it? Shall I catch where the getValX() function is called or I shall catch it in main()? I am new to exceptions.

Throwing

Yes, just throw it:

if (val1 < 0)
  throw MissingConfigValueException("getVal1() failed or val1 < 0");

Don't print an error message before throwing. Printing is left to whoever catches the exception. For many exceptions (such as missing config values), there may be nothing wrong, so no need to print anything.

When you do print error messages, print to std::cerr — error messages belong on standard error, not standard output.

failedGetValue is not needed, since all it would do is throw an exception.

Catching

Catch exceptions where you can handle them. If the code that calls getVal1 can recover from a missing config value (perhaps by using a default), then it should catch the exception and proceed:

try {
  foo = config.getFoo();
} catch (MissingConfigValueException &e) {
  foo = DEFAULT_FOO;
}
doSomethingWith(foo);

If the caller can't handle the exception, then don't catch it. Just let it propagate up automatically. You don't have to do anything; that's the point of exceptions.

You don't have to catch all exceptions in main, because the default handler should print the exception and exit. You might do this if you want to handle them differently — for example, if you want to abort an operation rather than exiting, or if you want to log exceptions to somewhere other than standard error.

try {
  do(stuff);
} catch (std::exception &e) {
  std::cerr << "Uncaught exception: " << e.what() << std::endl;
}

Comments on other aspects of the program

If getVal2 is just like getVal1 with a different path, then they should be calls to one function, with the varying parts as parameters:

double getVal1() const { return getVal("path.to.val1"); }
double getVal2() const { return getVal("path.to.val2"); }

Or, even simpler, call getVal directly and don't bother with a separate function for each value.

Two problems with the name MyConfigFileException:

Names generally shouldn't contain My. Everything is MyFeature to someone, so it conveys no useful information. The name of a class should say what its instances represent, not who wrote it.
It doesn't say what the problem is. Exception names should say what's wrong, not just which component had a problem. In this case the problem is that a config value is either missing (MissingConfigValue) or invalid (InvalidConfigValue).

It would be nice to distinguish missing values from invalid ones, perhaps using ptree::get_optional. That would let you use the default (when there is one) in place of missing values, but reliably detect invalid values.

MyConfigFileException should derive from std::runtime_error, since it indicates a problem with the environment (specifically the config file), not a bug in the program. Actually, in a small program, I wouldn't bother with a new exception class — I'd just throw a runtime_error with an appropriate error message.

PTree does all this for you

Actually, you needn't check for missing values or throw an exception yourself, because ptree already does it.

The one-argument form of ptree::get throws an exception when the value is missing, and the two-argument form takes a default which is returned instead. This is exactly what you want for config values that have a default: get<int>("foo.bar", 5) returns the configured value of foo.bar, or 5 if there isn't one. When there is no default, get<int>("foo.bar") returns the configured value, or throws an exception.

ptree is intended for storing things like config files, and it's designed to make this convenient. Its authors have done your work for you!


File-write operations Below is the code for user recommendations using mahout.

DataModel dm = new FileDataModel(new File(inputFile));
UserSimilarity sim = new LogLikelihoodSimilarity(dm);
UserNeighborhood neighborhood = new NearestNUserNeighborhood(100, sim,
        dm);
GenericUserBasedRecommender recommender = new GenericUserBasedRecommender(
        dm, neighborhood, sim);

After the recommendations are generated, I am trying to write it to a file like this:

FileWriter writer = new FileWriter(outputFile);
for (LongPrimitiveIterator userIterator = dm.getItemIDs(); userIterator.hasNext();) {
long user = (long) userIterator.next();
List<RecommendedItem> recs = recommender.recommend(user, numOfRec );
    for (RecommendedItem item : recs) {
        writer.write(user + "," + item.getItemID() + ","
                + item.getValue()+"\n");
    }
}
writer.close();

This code to write to file is taking lot of time. How can I speed up the write operations?

I tried with BufferedWriter, but was unable to gain speed-up.

You can gain some efficiency by wrapping the writer in a BufferedWriter.

If you are using java 7 or better you should use try-with-resources to auto close the writer, otherwise you should use a try-finally:

try(BufferedWriter writer = new BufferedWriter(new FileWriter(outputFile))){
    //the for loops
}

with try-finally:

BufferedWriter writer = new BufferedWriter(new FileWriter(outputFile));
try{
    //the for loops
}finally{
    writer.close();
}

This ensures that the writer is closed should an exception occur.

Second appending Strings for output is not the most performant thing that should be done instead pass each string separately:

writer.write(user);
writer.write(",");
writer.write(item.getItemID());
writer.write(",");
writer.write(item.getValue()
writer.newLine();//only available in BufferedWriter

As a more general suggestion there is a better method than reading the entire file doing some processing and then writing the output. Instead you can read only as far as you need to process a part of the data and then write the result out again. Whether you can depends on what you are actually doing with the data. This is only possible if you have a 1-pass transform.


Bellman Ford algorithm implementation Please review my code and help me to improve it.

#include <iostream>
#include <vector>
#include <limits>
#include <map>

class Graph
{
    int vertex_count;

    enum { INF = std::numeric_limits<int>::max() };

    struct Vertex
    {
        int id;
        int distance;
        Vertex * parent;

        Vertex(int _id) : id(std::move(_id)),
                          distance(INF),
                          parent(nullptr)
                          {}
    };
    std::vector<Vertex> vertices;
    //std::pair<int, int> edge;
    std::map<std::pair<int, int>, int> edge_weight;

  public:
    Graph(int);
    void add_edge(int, int, int); //source, destination, weight
    bool bellman_ford(int); //source
    void distance_from_source();

  private:
    void initialize_single_source(int); //source
    void relax(int, int, int); //source, destination, weight
};

Graph::Graph(int v)
{
    vertex_count = v;
    for (int i = 0; i < vertex_count; i++)
    {
        vertices.push_back(Vertex(i));
    }
}

void Graph::initialize_single_source(int src)
{
    vertices[src].distance = 0;
}

void Graph::add_edge(int src, int dest, int weight)
{
    edge_weight.insert(std::make_pair( std::make_pair(src, dest), weight ));
}

void Graph::relax(int src, int dest, int weight)
{
    auto wt =  edge_weight.find(std::make_pair(src, dest));
    if (vertices[dest].distance > (vertices[src].distance + wt->second))
    {
        vertices[dest].distance = (vertices[src].distance + wt->second);
        vertices[dest].parent = &vertices[src];
    }
}

bool Graph::bellman_ford(int src)
{
    initialize_single_source(src);
    for (int i = 0; i < vertex_count - 1; i++)
    {
        for (auto it = edge_weight.begin(); it != edge_weight.end(); it++)
        {
            relax(it->first.first, it->first.second, it->second);
        }
    }

    for (auto it = edge_weight.begin(); it != edge_weight.end(); it++)
    {
        if (vertices[it->first.second].distance > (vertices[it->first.first].distance + it->second))
           return false;
    }
    return true;
}

void Graph::distance_from_source()
{
    std::cout << "Vertex\t\tDistance from Source\n";
    for (int i = 0; i < vertex_count; i++)
    {
        std::cout << vertices[i].id <<"\t\t" << vertices[i].distance <<"\n";
    }
}

int main()
{
    Graph grp(5);
    grp.add_edge(0, 1, 6);
    grp.add_edge(0, 2, 7);
    grp.add_edge(1, 3, 5);
    grp.add_edge(1, 4, -4);
    grp.add_edge(1, 2, 8);
    grp.add_edge(2, 3, -3);
    grp.add_edge(2, 4, 9);
    grp.add_edge(3, 1, -2);
    grp.add_edge(4, 0, 2);
    grp.add_edge(4, 3, 7);

    bool res = grp.bellman_ford(0);
    if (res == true)
       std::cout << "Graph contains no negative cycle \n";
    else
       std::cout << "Graph conatins negative cycle \n";

    grp.distance_from_source();
}

Vertex class

The INF constant is used only as the initializer for Vertex::distance, so I'd inline the value directly, like this:

struct Vertex
{
    const int id;
    int distance = std::numeric_limits<int>::max();
    Vertex *parent = nullptr;

    Vertex(int id)
        : id(id)
    {}
};

I've inlined the initializers that don't depend on arguments, and I've removed the std::move cast that has no benefit for a primitive type.  Since that constructor is now the same as default, we can omit it.

Graph class

The vertex_count member can (and should) be declared const; that also requires it to be initialized (not assigned) in the constructor.  That said, there's no need for it, as we can always use vertices.size().

The constructor argument isn't obvious from the class definition - name it and if necessary also add a comment.

Naming: distance_from_source() sounds like it can be used to obtain the results, but it actually prints to standard output.  I'd rename it to something like (using #include <iosfwd>):

std::ostream& print_distances(std::ostream&) const;

Constructor

We should tell the vector its final size, so it can reserve capacity and avoid multiple allocations.

We can use emplace_back() to simplify the population of the vector:

Graph::Graph(int v)
    : vertex_count(v)
{
    vertices.reserve(vertex_count);
    for (int i = 0;  i < vertex_count;  ++i) {
        vertices.emplace_back(i);
    }
}

Adding edges

The add_edge method doesn't check that src and dest are within range, and it does nothing if an edge already exists.  We can easily detect these conditions:

void Graph::add_edge(int src, int dest, int weight)
{
    if (src == dest) {
        throw std::logic_error("src==dest");
    }
    if (src < 0 || vertex_count <= src) {
        throw std::out_of_range("src");
    }
    if (dest < 0 || vertex_count <= dest) {
        throw std::out_of_range("dest");
    }

    auto const inserted = edge_weight.insert({ {src, dest}, weight });
    if (!inserted.second) {
        throw std::logic_error("existing edge");
    }
}

I recommend changing the index type from int to an unsigned type (e.g. std::size_t) as negative values are not valid.

Also, the use of std::pair rather than a specific structure makes it hard to follow code full of first and second all meaning different things.

Relaxing weights

It's inefficient to have to re-find() an edge.  However, we're passing weight to avoid that, so just re-use it:

void Graph::relax(int src, int dest, int weight)
{
    auto & existing = vertices[dest].distance;
    auto const proposed = vertices[src].distance + weight;
    if (proposed < existing) {
        existing = proposed;
    }
}

I've dropped the update to parent since we never use it.

Bellman-Ford computation

If we want to recalculate with a different origin node, we'll need to reset initial distances to maximum, as well as setting the origin to zero.

Modified program

#include <iosfwd>
#include <vector>
#include <map>

class Graph
{
    using index_type = std::size_t;
    using distance_type = int;

    static const distance_type max_distance;

    struct Vertex
    {
        const index_type id;
        distance_type distance = max_distance;
    };

    struct Edge
    {
        index_type from;
        index_type to;
        bool operator<(const Edge& other) const
        {
            return std::tie(from, to) < std::tie(other.from, other.to);
        }
    };

    std::vector<Vertex> vertices = {};
    std::map<Edge,distance_type> edge_weight = {};

public:
    Graph(index_type size);
    void add_edge(index_type from, index_type to, distance_type weight);
    bool bellman_ford(index_type origin); // true if no negative cycles
    std::ostream& print_distances(std::ostream&) const;

private:
    void relax(index_type from, index_type to, distance_type weight);
};

// Implementation

#include <limits>
#include <ostream>

const Graph::distance_type Graph::max_distance
    = std::numeric_limits<distance_type>::max();

Graph::Graph(index_type size)
{
    vertices.reserve(size);
    for (index_type i = 0;  i < size;  ++i) {
        vertices.push_back(Vertex{i});
    }
}

void Graph::add_edge(index_type src, index_type dest, distance_type weight)
{
    if (src == dest) {
        throw std::logic_error("src==dest");
    }
    if (vertices.size() <= src) {
        throw std::out_of_range("src");
    }
    if (vertices.size() <= dest) {
        throw std::out_of_range("dest");
    }

    auto const inserted = edge_weight.insert({ Edge{src, dest}, weight });
    if (!inserted.second) {
        throw std::logic_error("existing edge");
    }
}

void Graph::relax(index_type src, index_type dest, distance_type weight)
{
    auto& existing = vertices[dest].distance;
    auto const proposed = vertices[src].distance + weight;
    if (proposed < existing) {
        existing = proposed;
    }
}

bool Graph::bellman_ford(index_type src)
{
    // initialise distances
    for (auto& vertex: vertices) {
        vertex.distance = max_distance;
    }
    vertices[src].distance = 0;

    // relax edges
    for (index_type i = 1;  i < vertices.size();  ++i) {
        for (auto edge: edge_weight) {
            relax(edge.first.from, edge.first.to, edge.second);
        }
    }

    // check for negative-weight cycles
    for (auto edge: edge_weight) {
        auto const& from_vertex = vertices[edge.first.from];
        auto const& to_vertex = vertices[edge.first.to];
        if (to_vertex.distance > from_vertex.distance + edge.second) {
            return false;
        }
    }

    // success!
    return true;
}

std::ostream& Graph::print_distances(std::ostream& os) const
{
    os << "Vertex\t\tDistance from Source\n";
    for (auto vertex: vertices) {
        os << vertex.id << "\t\t" << vertex.distance << "\n";
    }
    return os;
}

// Test program

#include <iostream>

int main()
{
    Graph grp(5);
    grp.add_edge(0, 1, 6);
    grp.add_edge(0, 2, 7);
    grp.add_edge(1, 3, 5);
    grp.add_edge(1, 4, -4);
    grp.add_edge(1, 2, 8);
    grp.add_edge(2, 3, -3);
    grp.add_edge(2, 4, 9);
    grp.add_edge(3, 1, -2);
    grp.add_edge(4, 0, 2);
    grp.add_edge(4, 3, 7);

    if (grp.bellman_ford(0))
        std::cout << "Graph contains no negative cycle \n";
    else
        std::cout << "Graph conatins negative cycle \n";

    grp.print_distances(std::cout);
}


Cleaner way for coding a repetitive application of a function I have this function definition which takes an r and applies the function f n times:

r => (1 to n).foldLeft(r)((rx, _) => f(rx))

So for n=3 this is equivalent to f(f(f(r)))

I don't like this solution, because it defines a Range from 1 to n which really isn't get used at all, which becomes obvious in the unused parameter _ in the fold left. This in turn forces me to give a separate name to rx which feels wrong to me.

Any idea how I can streamLine this code?

What's wrong with straight recursion?

def ntimes[A](n:Int, f:A=>A, a:A):A = if (n==0) a else ntimes(n-1, f, f(a))

Another clean option is

Iterator.iterate(a)(f).drop(n).next


Euler 2 : Simple Fibonacci I'm just starting to experiment with F# (from a C# background).  I think I'm starting to get into the right way of thinking, but this code still seems pretty awkward.  Is there a better (more terse) way to accoAny comments appreciated.  (BTW, this is supposed to sum all even fibonacci numbers up to 4,000,000)

let rec fib x y max acc =
    let z = x + y
    if z < max then
        if z%2=0 then
            fib y z max (z + acc)
        else
            fib y z max acc
    else
        acc

let result = fib 1 2 4000000 2

printfn "%d" result

I think the solution reads better if you separate concerns a bit more instead of rolling it all into one mega function.

For instance, with these helper functions (that each do one—and only one—thing):

let fib =
  Seq.unfold
    (fun (cur, next) -> Some(cur, (next, cur + next)))
    (0I, 1I)

let isEven n = n % 2I = 0I

you can express the solution at a high level (almost at the level you would explain it in words!):

fib                                        //given the set of fibonacci numbers
|> Seq.filter isEven                       //take those that are even
|> Seq.takeWhile (fun n -> n <= 4000000I)  //not exceeding 4,000,000
|> Seq.sum                                 //and sum them


Improving function that parses string and returns first integer present This exercise is from jshero.com.
Write a function parseFirstInt that takes a string and returns the first integer present in the string. If the string does not contain an integer, you should get NaN.
Here is my working solution. I'm posting in hopes of receiving pointers on how I could have written the code more effectively.
function parseFirstInt(str) {
    if (Number.isNaN(str) === true) {
        return NaN;
    } else {
        let searchFrom = str.search(/[-+]?[0-9]+/g);
        let searched = str.substr(searchFrom);
        return parseInt(searched, 10);
    };
}

Example: parseFirstInt('No. 10') should return 10 and parseFirstInt('Babylon') should return NaN.
parseFirstInt('No. -3) should return -3.
Thank you!

I believe you have either misunderstood the task, or misunderstood Number.isNaN. In any case the if doesn't do anything usefull. Number.isNaN returns true only when the value is exactly NaN, which is not a valid input for your function anyway.
Then you are not checking if search() isn't matching anything (and thus searchFrom is -1). It doesn't really matter, because substr(-1) simply returns the last character, but it would make the code better to understand.
Use const instead of let when the value doesn't change.
Personnally I'd use .match() instead of .search(), because it returns the matched string directly (in an array):
function parseFirstInt(str) {
  // Check if the paramter is a object with the method `match`
  if (!str.match) {
    return NaN;
  }
  const match = str.match(/[-+]?[0-9]+/);
  if (match && match[0]) {
    return parseInt(match[0], 10);
  }
  return NaN;
}


Ajax Object - 12030 Loop Fix? Here are 3 methods which handle my ajax calls.

I loop until pass b.c. I've had problems with the Ajax Object working as expected.  This is noted here

/**
 *    Ajax
 */

var Ajax = ( function ()
{
    var Ajax = function (element)
    {
        this.object = this.create();
    };
    Ajax.prototype.create = function()
    {
        var request;
        try
        {
            request = new window.XMLHttpRequest();
        }
        catch( error )
        {
            try
            {
                request = new window.ActiveXObject( "Msxml2.XMLHTTP" );
            }
            catch( error )
            {
                try
                {
                    request = new window.ActiveXObject( "Microsoft.XMLHTTP" );
                }
                catch( error )
                {
                    request = false;
                }
            }
        }
        return request;
    };
    Ajax.prototype.use = function( param, ajax_func )
    {
        var GATEWAY = 'class.ControlEntry.php';
        var self = this;
        this.object.open( "POST", GATEWAY, true );
        this.object.setRequestHeader( "Content-type", "application/x-www-form-urlencoded" );
        this.object.setRequestHeader( "Content-length", param.length );
        this.object.setRequestHeader( "Connection", "close" );
        this.object.onreadystatechange = function()
        {
            if( this.readyState === 4 )
            {
                if( this.status === 200 )
                {
                    ajax_func( this.responseText );
                    return true;
                }
                else
                {
                    self.invoke( param, ajax_func );
                    return false;
                }
            }
        };
        this.object.send( param );
        return true;
    };
    Ajax.prototype.invoke = function( param, ajax_func )
    {
        var state = false,
            count = 1;
        while( state === false && count <= 5 )
        {
            if( count !== 1 )
            {
                alert( 'Ajax Object Use Failed | Try Again ');
            }
            state = this.use( param, ajax_func );
            count++;
        }
        return state;
    };
    return Ajax;
} () );

This is the real bad practice.

The Ajax is Asynchronous, you never know how the response will back.
Javascript is the single thread language, so that means in one time period just can run one function. in your repeatUseAjax function, you try to loop send ajax. the result will be you will send 5 times ajax call every time when the ajax response failure.And you will never know when the response suceess. what about first failure other success? what about always failure?loop forever?


Babysitting a server reboot I'm looking to improve a loop within a script that performs scheduled server restarts.

Currently, I'm using a DO While loop with an exit condition within an IF statement once a counter reaches a limit.

I'm looking to change this into a more simpler loop using a Labeled While loop containing a For loop that breaks when the nested If loop catches the counter variable reaching a limit.  I've read it's best to avoid using Break but I just cannot come up with anything.

#current loop
$max = 60;$i = 0
 DO
 {
  IF($i -gt $max){
  [PsCustomObject]@{
  "Server" =  $server
  "Status" = "FailedToReboot!"
  "LastRebootTime" = "$LastReboot"
  "CurrentRebootTime" = "FailedToReboot!"
   }#end custom object

Send-MailMessage <Details>

exit}#exit script and log failed to reboot.

$i++

Start-Sleep -Seconds 10

}#end DO

While (Test-path "\\$server\c$")#After this point, server went offline, continue script.

#new test loop
:WaitingForReboot while (test-path \\$server\C$){
 for($i = 0; $i -le 7){
 $i++
 "Waiting for $server to reboot, waited " + $(10 * $i) + " of 50 seconds"

  IF($i -eq 5){"Exit wait loop";break WaitingForReboot}Else{
    #do nothing in Else, until $i hits 5.
    }#else block
Start-Sleep -Seconds 10
 }#for loop
}#while loop

As you have read it is usually preferred to exit a loop via its condition(s) as supposed to force exiting via commands like break, exit and continue. Those commands do have their place but your loop and logic could do without them. You are only executing the code inside the if once. Why bother keeping it inside the loop? Lets update the while condition and move the if clause outside.

$maximumAttempts = 60
$attemptIndex = 0

do {
    # Wait between checks.
    $attemptIndex++
    Start-Sleep -Seconds 10
} While (Test-path "\\$server\c$" -and $attemptIndex -le $maximumAttempts)

# Check if the loop ran for its maximumAttempts
if($attemptIndex -ge $maximumAttempts){

  [PsCustomObject]@{
      "Server" =  $server
      "Status" = "FailedToReboot!"
      "LastRebootTime" = "$LastReboot"
      "CurrentRebootTime" = "FailedToReboot!"
   }

   Send-MailMessage <Details>
}

You will also see that I have used more meaningful variable names. I have also used standardized indentation. Doing that removes the need for comments like }#while loop. The indentation makes it clear where the brackets start and stop and make reading the code easier.


Linux cat command in C I wrote my own implementation of the linux cat command in C for my computer laboratory class. We were asked to replicate its functionality with no options passed as arguments or with just the -b, -n and -s options.

I'm posting the code here because, although it works fine, i feel a little bit insecure about. Firstly because i think i "over-coded" it a bit, specially on the option handling part, and also, because i'm still not very confortable working with pointers and so i am not very sure if my approach does not generate memory leaks or malfunctions.

Here it is:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
#include <ctype.h>
#define BUF_SIZE 1024

int readStdin(int index, int bflag, int nflag){
  char buffer[BUF_SIZE];
  while(fgets(buffer, BUF_SIZE, stdin)){ //reads from the standard input and prints the input
    if (nflag){
      printf("     %d  %s", index, buffer);
      index++;
    }
    if (bflag){
      if (*buffer == '\n'){
        printf ("%s", buffer);
      }
      else{
        printf ("     %d  %s", index, buffer);
        index++;
      }
    }
    else {
      printf("%s", buffer);
    }
  }
  return index; //returns the incremented index to perpetuate its use
}

int readFile(char* filename, FILE* fp, int index, int bflag, int nflag){
  char ch;
  char s[BUF_SIZE];
  if (fp==NULL){ //in case the file doesn't exist
    printf("%s: No such file or directory\n", filename);
    exit(1);
  }

  if (bflag){
    while ((fgets(s,BUF_SIZE,fp))){
      if (strcmp(s,"\n") == 0){
        printf (" %s", s);
      }
      else {
        printf ("     %d  %s", index, s);
        index++;
      }
    }
    fclose(fp);
  }
  if (nflag){
    while ((fgets(s,BUF_SIZE,fp))){
      printf ("     %d  %s", index, s);
      index++;
    }
    fclose(fp);
  }
  else{
    while ((ch=fgetc(fp)) != EOF){ //printing loop
      putchar(ch);
    }
    fclose(fp);
  }
  return index;
}

void readArgs(int argc, char* argv[]){
  FILE* fp;
  int index = 1; //line index. to be used in case -b or -n is passed as an argument
  int option; //option passed as argument
  int bflag = 0; //-b option deactivated by default
  int nflag = 0; //-n option deactivated by default
  opterr = 0; //deactivates getopt's default error messages

  //checks if there are options passed as argument and updates their flags
  while ((option = getopt(argc, argv, "bn")) != -1){
    switch (option){
      case 'b':
              bflag = 1;
              break;
      case 'n':
              nflag = 1;
              break;
      case '?': //in case there was some problem
              exit(1);
    }
  }

  if (bflag == 1 && nflag == 1){ //if -b and -n are passed as argument, b overrides n
    nflag = 0;
  }

  for (int i=optind; i<argc; i++){
    if (*argv[i] == '-'){ //in case of '-' in argv[i], reads from stdin and prints
      index = readStdin(index, bflag, nflag);
      clearerr(stdin);
    }
    else { //prints the contents of the file in *argv[i]
      fp = fopen(argv[i], "r");
      index = readFile(argv[i], fp, index, bflag, nflag);
    }
  }
}

int main(int argc, char* argv[]){
  if (argc<2){ //if there are no arguments
    readStdin(1,0,0);
    return 0;
  }
  readArgs(argc, argv); //otherwise
  return 0;
}

I didn't implement the -s option because i cant think of a way to handle it at the same time as the other options. That's why i think that the way i am handling the options part is a bit inefficient.

Any suggestions?

Use Standard Constants When You Can

The stdio.h header file contains a definition of BUFSIZ. This is a good constant to use for input and output buffers.

In the stdlib.h header file are 2 constants that are useful when calling exit or returning from main, these are EXIT_FAILURE and EXIT_SUCCESS. These constants are universal, even if the underlying operating system expects different numbers for success or failure this constants will provide the correct value.

DRY Code
There is a programming principle called the Don't Repeat Yourself principle. The principle is this, if you find yourself repeating code, it might be better to create a function or sub-program and call that function where the code is repeated. This reduces the amount of code in a module and makes it easier to read. What is more beneficial than the readability is the fact that you only have to write and debug that code once. Another benefit of puting repeating code into a function is that if the code needs to be changed, there is only one place to change it.
.

Code that repeats a lot in this program:

    printf("     %d  %s", index, buffer);
    index++;

While this is only one line of code, making a function out of it reduces the possibility of errors such as missing spaces or too many spaces.

All of the output could be in their own functions:

void formattedLineOut(int *index, char buffer[])
{
    printf("     %d  %s", *index, buffer);
    *index++;
}

The function formattedLineOut is receiving a reference to index rather than
the value. This way the function doesn't have to have a return value for
index.

void bprint(int *index, char buffer[])
{
    if (strcmp(s, "\n") == 0) {
        printf(" %s", s);
    }
    else {
        formattedLineOut(index, s);
    }
}

void outputLine(int *index, char buffer[], int nflag, int bflag)
{
    if (nflag)
    {
        formattedLineOut(index, buffer);
    }
    else if (bflag)
    {
        bprint(index, buffer);
    }
    else
    {
        printf("%s", buffer);
    }
}

int readFile(char* filename, FILE* fp, int index, int bflag, int nflag) {
    char ch;
    char s[BUF_SIZE];
    if (fp == NULL) { //in case the file doesn't exist
        printf("%s: No such file or directory\n", filename);
        exit(1);
    }

    while ((fgets(s, BUF_SIZE, fp))) {
        outputLine(&index, s, bflag, nflag)
    }

    return index;
}

Match fclose() With fopen()

The function readFile has multiple calls to fclose(). This can lead to bugs,
it might be better to call fclose() after readFile returns in the calling
function. This will also allow readFile to handle stdin, since stdin is a file pointer.


Dynamic menu depending on a set mode I am creating a dynamic menu whose items appear depending on a set 'mode' (that is passed via AJAX). Off this it creates the menu, disabling and hiding icons that are not associated with that mode.

The problem is, there are a lot of if conditions in my implementation. Can anyone show me a cleaner way of doing what I am trying to achieve?

public function gridMenu()
  {
    $mode = Validate::sanitize($_POST['mode']);

    $modes = array(
        'products' => array('edit', 'delete', 'archive')
    );

    $output = '
';
    for($i = 1; $i < 7; $i++) {
      $img = '';
      $output .= '<div class="hex hex' . $i;
      if($i == 1)
      {
        if(in_array('edit', $modes[$mode]))
        {
          $output .= ' hex-selectable';
          $img = '';
        } else {
          $output .= ' hex-disabled';
        }
      }
      if($i == 2)
      {
        if(in_array('zzz', $modes[$mode]))
        {
          $output .= ' hex-selectable';
        } else {
          $output .= ' hex-disabled';
        }
      }
      if($i == 3)
      {
        if(in_array('delete', $modes[$mode]))
        {
          $output .= ' hex-selectable';
          $img = '';
        } else {
          $output .= ' hex-disabled';
        }
      }
      if($i == 4)
      {
        if(in_array('xxx', $modes[$mode]))
        {
          $output .= ' hex-selectable';
        } else {
          $output .= ' hex-disabled';
        }
      }
      if($i == 5)
      {
        if(in_array('archive', $modes[$mode]))
        {
          $output .= ' hex-selectable';
          $img = '';
        } else {
          $output .= ' hex-disabled';
        }
      }
      if($i == 6)
      {
        if(in_array('zzz', $modes[$mode]))
        {
          $output .= ' hex-selectable';
        } else {
          $output .= ' hex-disabled';
        }
      }

      $output .= '">';
      $output .= $img;
      $output .= '
';
    }
    $output .= '

';
    $output .= '
';
    echo $output;
  }

Your code has multiple bad practices.

There is no speration of concern. Your code is handling display, logic and controlling. All in onde function.

If I look at the function, it tells me it needs nothing (no arguments need to be passed in). So the only thing it should be using are variables that are accessible inside the object scope. BUT it turns out you are using a 'Validate' class and variables from the global scope ($_POST). I only know this because I read the entire code. No really nice towards other developers or the future you demaning that one should read the entire code before knowing how to use it and what is going on. And then ofcourse the biggest problem. al those if's

So, some problems and lets fix those problems.

You sanitize a variable that is handed in by the client ($_POST). You then create an array of possible 'modes' out of the blue. And after that you construct some html with images and a lot of business logic in it. Your function really has to be all knowing!

So, let's sepperate the concerns.

the $mode is clearly something the function needs to do it's magic. So this sould be a paramter that is passed in:

public function gridMenu(String $mode);

Note that the $mode needs to be a string. (obviously)

Another thing this function absolutely needs is the $modes variable. But, this variable doesn't change with every call of the gridMenu() function. It is the same, and should be, during runtime. But it could change over time or even change depending on what user is logged in. So we place this in the constructor of the class:

class Grid {

  private $modes;

  public function __construt($modes) {
    $this->$modes = $modes;
  }

  public function gridMenu(String $mode) { }
}

Now, gridMenu doesnt really say much. I would have to guess what it does: save a menu? does it create one? does it return one? So lets change that name to something more understandable:

class Grid {

  private $modes;

  public function __construt($modes) {
    $this->$modes = $modes;
  }

  public function renderMenu(String $mode) { }
}

Other possibilities are echoMenu, printMenu... Note that i left the word grid out because we are allready in the grid class. Obviously we wouldn't print a non-grid menu out here...

Good, so now we can remove that Validate::sanitize to the outside of the code and that declaration of $modes is no longer hard-coded into the class. This gives us a lot more flexibility,...

Now let's look at what our function does. It creates html. wheut? So it doesn't provide functionality but simply prints some html? So it's is actually a template disguised as a function... the sneaky bastard. We really have to keep an eye on those templates mate. They tend to creep into functions all the time all over the code.

This makes them harder to maintain and makes you write tons of Repeat-Yourself code. For instance if we would no want it to return the data in XML, or in a list instead of divs. Or even worse, have the delete button placed somewhere else, or or.... OOh boy, beter start all over again.

Needless to say, we are doing something really wrong here and we have read all this text for nothing :( Well atleast you know about sepration of concern. And I give you an oher link: SOLID

But what has this todo with you question? Ah it's very easy. What you actually need is a template that handles the display of of those '.hex' divs (what is a hex? and why is action.delete/archive a hex?). You also need some business logic that calculates what 'actions' you can do. and then we need some controller to bring it all together.

Your template would look something like this:

  <div class="hex hex1 hex-<?php echo $mode->hasAction('edit') ? 'enabled' : 'disabled' ; ?>">
    <?php echo $mode->hasAction('edit') ? '': ''; ?>

    //zzz

  <div class="hex hex1 hex-<?php echo $mode->hasAction('delete') ? 'enabled' : 'disabled' ; ?>">
    <?php echo $mode->hasAction('edit') ? '': ''; ?>

    //xxx

  <div class="hex hex1 hex-<?php echo $mode->hasAction('archive') ? 'enabled' : 'disabled' ; ?>">
    <?php echo $mode->hasAction('edit') ? '': ''; ?>

    //zzz

Your business logic 'Mode' and 'ModeFactory' class would look something like this:

class ModeFactory {

  private $modes;

  public function __construt(array $modes) {
    $this->$modes = $modes;
  }

  public function getMode(string $mode) {
    if ( !isset($this->modes[$mode]) ) {
      throw new ModeNotFoundException();
    }
    return new Mode($this->modes[$mode]);
  }
}

class Mode {
  private $actions = $actions;
  public function __construct(array $actions) {
    $this->actions = $actions;
  }
  public function hasAction(string $action) {
    return in_array($this->actions[$action]);
  }
}

class ModeNotFoundException extends Exception {}

NOTE: I used the Factory pattern here, is this the best? not, but it is a pattern that suited my needs and I feld like using it.

Then to bring it all togeter: the controller:

include 'Validate.php'; #the Validate class
include 'Mode.php'; #Modefactory and Mode

//note that now only the controller needs to know
// how the client communicates with this program
$currentMode = Validate::sanitize($_POST['mode']);

//because we pass in the modes+Actions in the factory,
// we can easily change everything.
//For instance we could fetch these mode+actions from a database
$modeFactory = new ModeFactory(array(
  'products'  =>  array('edit', 'delete', 'archive')
));

//we need this variable in the template so lets create it
$mode = $modeFactory->getMode($currentMode);

include 'myHexTemplate.php';

//all done

To sum up: the massive amount of if's wasn't the real problem. The real problem was the seperation of concern and those sneaky template functions. By solving those issues we now not only have better to read code. It is also very flexible, very easy to test etc etc.

Say we want to test a new Mode, we simply create our Mode:

$mode new Mode(array('myTestAction'));

Is this code perfect? no, the names used are horrible. What is a hex? what is a mode? what is an action? These are all generic words and make it very hard actualyl properly reuse this code. But appart from that, I think you get the important lesson here.

Good luck and fire away at the comments. @everyone else: feel free to correct spelling, I honestly don't feel like reading this entire post again for spelling mistakes.


Object Oriented Design of Card Deck I am designing classes for a game with card deck. Please review my code.

Each Player gets Hand of cards
The Deck can be shuffled and cards are dealt one at a time from the deck and added to the players hands.

Specifically, I am confused with the good method signature for the method dealCard in the Deck class. I can either pass the player method to the method like - void dealCard(Player player) or get the card and add to hand of player -  Card dealCard(). Which of these implementation is good as per the practices of OOPS? How do I think in these kind of scenarios? Any advice can be helpful

    package main;
        //Each card has a value and a suite.
    public class Card {
        int value;
        SUITE suite;

        public Card(int value, SUITE suite) {
            this.value = value;
            this.suite = suite;
        }

        public int getValue() {
            return value;
        }

        public SUITE getSuite() {
            return suite;
        }

        public void setValue(int value) {
            this.value = value;
        }

        public void setSuite(SUITE suite) {
            this.suite = suite;
        }

        @Override
        public String toString() {
            return "Card{" +
                    "value=" + value +
                    ", suite=" + suite +
                    '}';
        }
    }

package main;
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import java.util.Random;

public class Deck {
    List<Card> cardDeck;

    public Deck() {
        this.cardDeck = new ArrayList<Card>();
        for(int value = 1 ; value <= 13 ; value++){
            for(SUITE suite : SUITE.values()){
             cardDeck.add(new Card(value,suite));
            }
        }
    }

    @Override
    public String toString() {
        return "Deck{" +
                "cardDeck=" + cardDeck +
                '}';
    }

    public void shuffle(){
        Random rand = new Random();
        //Generate two random numbers between 0 to 51
        for(int i = 0 ; i < 20 ; i ++){
            int firstCard = rand.nextInt(52);
            int secondCard = rand.nextInt(52);
            Collections.swap(cardDeck,firstCard,secondCard);
        }
    }

    public void dealCard(Player player){
       //Get next card and add to hand of the player
        Card removedCard = cardDeck.remove(0);
        player.getHand().add(removedCard);
    }

    public Card dealCard(){
        Card removedCard = cardDeck.remove(0);
        return removedCard;
    }

    //Size of the deck for testing purpose
    public int getSizeOfDeck(){
        return cardDeck.size();
    }
}

package main;
import java.util.ArrayList;
import java.util.List;

public class Player {
    List<Card> hand;

    public Player() {
        this.hand = new ArrayList<Card>();
    }

    public List<Card> getHand() {
        return hand;
    }

    @Override
    public String toString() {
        return "Player{" +
                "hand=" + hand +
                '}';
    }
}

package main;

public enum SUITE {
    HEART,
    SPADE,
    CLUB,
    DIAMOND
}

package main;
public class Main {
    public static void main(String args[]) {
        Deck deck = new Deck();
        System.out.println(deck);
        System.out.println("Size of deck is: "+deck.getSizeOfDeck());
        deck.shuffle();
        System.out.println("Deck after shuffling is "+deck);

        Player player1 = new Player();
        Player player2 = new Player();

        deck.dealCard(player1);
        System.out.println("Size of deck after dealing a card to player is "+deck.getSizeOfDeck());
        deck.dealCard(player2);
        System.out.println("Size of deck after dealing a card to player is "+deck.getSizeOfDeck());

        System.out.println("Hand of player 1 is "+player1.getHand());
        System.out.println("Hand of player 2 is "+player2.getHand());

        player1.getHand().add(deck.dealCard());
    }
}

So much to say, in so little space. This could be answered in so many ways, mainly due to why you need to model the cards. Without a proper context, the requirements will change and can thusly change the entire model. So lets pretend you're aiming to build one (or several) card game(s).

Separation of Concerns

There are many important OOP principles, but I'm mainly going to address the Separation of Concerns (or Concern related to Computer Science, as it often is what you need to consider first: Why do you have the classes/objects, and what is their intended purpose?

Somewhat simplified one could say that: An object/class shouldn't need to concern itself with stuff outside of itself. This could allow for the internal of any class to be replaced, as long as the public API (the public methods to call it) aren't changed. I'm going to try to exemplify this related to the various classes you've provided.

Card

In other words, a Card shouldn't need to concern itself who might use it, and how they might use it. And the basic method of handling a card should remain somewhat constant.

More specific, it seems kind of strange that a Card is able to change its value or suit. What kind of a gaming card is able to do that? If it is the King of Diamonds, then it usually stays that way for ever.

To me, it would be more natural for the cards to have a method defining how the output could look like. For example to choose alternative textual variations: "value: 13, suit: Spade", "King of Spade", "K♠" or even "". The last two examples from: Wikipedia's Playing Cards in Unicode.

Possibly the parent Card class should be rather empty, and allow for subclasses to specify the criteria of that card. Like ordinary playing cards, or a Uno deck, or Pokemon deck, and so on.

Deck

The Deck consists of Cards, but other than basic handling of cards, it shouldn't know too much about the cards. And it shouldn't really know anything about a Player or a Game using the Deck.

And this answers your question on dealCard(). It shouldn't know anything about the player, and basically it should only remove a Card from the deck, and return it to whomever who might use it as they please.

Other interesting functions for the Deck could be re-insert a card into the deck, possibly verifying that the card is legal in this kind of deck. Imaging solitaire games like Spider (using only the suite of Spade), or games using double decks.

Player, or maybe Hand?

I'm not sure what I would put in a Player class, but in the context of a card game, I think I would rather have a Hand class to describe my current set of cards.

In this context, it would be natural to add/remove cards to the hand. And to display it in various alternatives. It could possibly have some generic methods for describing the order of a hand compared to another hand.

For subclassing one could also imagine that the hand could calculate the overall value of the hand, for example the Poker value, or whether it contained sets or series, I could imagine have methods for calculating the value of my hand, for example the Poker Hand value, or whether I had any series or sets. And the subclasses could specify which hands were the most valuable related to each other.

Code review

In general your code looks nice, with proper indentation and brace usage. I would prefer a little more consistency in commenting the methods/classes at a given place related to the definition. You could/should consider using Javadoc, or a given documentation standard for describing your classes.
A shuffle() method which only shuffles 20 cards, by swapping 2 each time, seems like a really lousy way to shuffle a deck. I'm sure you can do better than this. Maybe loop through the deck once, and switch the card placement to somewhere else in the deck?
Better presentation alternatives would be nice, I think. See comment on Card class above.
A minor detail on the SUIT enum. I would order it as SPADE, HEART, DIAMOND and CLUB, as that is the common ordering of value of cards in a lot of card games. Not entirely thrilled about the Uppercasing, but not sure what's the common method in Java to denote globals like these.

PS! I've used Wikipedia for reference, there does exist better description, but for consistency I chose to use these here.

PPS! Another interesting principle are the SOLID principle, but it might be difficult to wrap your head around if you're very new to programming.


Simple dispatcher for an embedded system This code is something that I have used for about 3-4 simple embedded systems projects.

The idea is something like this:

This is non-OS based implementation, everything is done in an outer infinite loop. No dynamic memory involved or advised.
It's an interrupt-driven system, polling for peripherals is not used anywhere.
Peripherals like Timer, UART, I2C, etc loads a particular value based on the event, in a Queue-like variable.
Non-trivial tasks like blinking an LED is done in a common Idle task.
Dispatcher loads the function handlers based on the event added in the queue.
(UPDATE) All read/write activities on CPU registers are managed in the respective Interrupt Service Routine. Adding the event in queue is possibly the last thing the code does in the ISR.

Enum for possible events:

typedef enum {

    TASK_EMPTY = 0,
    TASK_INITIALIZE = 1,
    TASK_IDLE,
    TASK_SOMETHING_1,
    TASK_SOMETHING_2,
    TASK_SOMETHING_3,
    TASK_SOMETHING_4,
    TASK_SOMETHING_N,

    MAX_TASKS,
    TASK_INVALID = -1,

} eTasks_t ;

A constant array which contains the event mapped to the handler:

const stTask_t GlobalTasksList[MAX_TASKS] =
{
    { TASK_EMPTY,              Placeholder             },
    { TASK_INITIALIZE,         SystemInitialization    },
    { TASK_IDLE,               Idle                    },
    { TASK_SOMETHING_1,        Handler_1               },
    { TASK_SOMETHING_2,        Handler_2               },
    { TASK_SOMETHING_3,        Handler_3               },
    { TASK_SOMETHING_4,        Handler_4               },
    { TASK_SOMETHING_N,        Handler_N               },
};

I talked about a Queue:

eTasks_t Queue[MAX_TASKS];
eTasks_t qTasksToAdd[MAX_TASKS * 2];

There is a structure which binds an event and it's handler together:

typedef struct stTask {

    eTasks_t TaskID;
    bool (*handler) (void);   // Would return true for success, and false for failure

} stTask_t;

stTask_t Task;

A Dispatcher function will take care of queued events:

void Dispatcher(void)
{
    while(1)
    {
        // Assign the task handler
        Task.handler = GlobalTasksList[Task.TaskID].handler;

        //Invoke handler
        if(FAILURE == Task.handler())
        {
            //Something went wrong with handler, Error!
            // Error handling

        }

        //Does the task needs a retry, or is a recurring task?
        // Reload if yes, else discard and load the next
        if(bTaskRetry == FALSE)
        {
            Queue_DiscardTask0();
        }

        //Assign the 0th task from Queue
        Task.TaskID = Queue[0];
    }
}

Let's look at an example of adding the event in the Queue from an ISR:

#pragma vector=PORT2_VECTOR
__interrupt void PORT2_Interrupt(void)
{
    // Clear interrupt
    CLR_BIT(P2IFG, 2);  //P2IFG is a CPU register which indicates the Port pin that has generated an interrupt.
    // Check if the queue is overflowing
    if(qIndexTasksToAdd < MAX_TASKS-1)
    {
        // Add event
        qTasksToAdd[qIndexTasksToAdd++] = TASK_SOMETHING_1;
    }
}

As of now, this strategy works fine without any questionable latency.
I have used the same skeleton for systems that could have as many as 20 events lined-up, and it works as per requirement to say the least.

Another advantage of this implementation that I felt is in regards with the efficiency this has when I am debugging an issue. Everything is routed through the handler/dispatcher, so I precisely know where to have a debug point/print setup.

The concern that I have with this implementation is the fixed memory footprint is has. For example, this won't be memory efficient for a LED blinking project.

Any thoughts where I could improve on this?

.... have as many as 20 events lined-up, and it works as per requirement
  Any thoughts where I could improve on this?

Stress test

The queue approach is non-deterministic and would benefit in knowing how much extra CPU is left.

Presently the processor is ample and the cumulative task list is low.  There is spare CPU power.  Yet let us stress test this design to find out how close and how code handles falling behind.

Add code to detect when Dispatcher() is starving.  Did every task get serviced at least once every 0.1s say?

Append a do-nothing task that consumes a x% of CPU time, call it Administratium.    Did the queue ever fail to add because it was full?  What was the maximum queue usage?  Was task latency too great?

Armed with this adjustable idle task, preliminary testing can gauge the degree of margin in the design.


Inherited code checking Struts method name I am working in an inherited code (as in "everybody related to it left before I arrived at the job and left no documentation") and I have noticed an strange pattern.

Almost(*) each Struts (Struts 1.3.8) method that is invoked begins with a code such as

public ActionForward myMethod(ActionMapping mapping, ActionForm form,
  HttpServletRequest request, HttpServletResponse response)
    throws IOException, SerletException {

  String method = request.getParameter("method");
  if (method != null && "myMethod".equals(method)) {
    // Perform the actual work
    ...

I mean, I would be pretty confident that Struts would redirect each request to its associated method and I should not need to double-check it in the logic (also, if Struts fails to properly redirect, then the application would fail because the desired operation would not be executed).

I have asked Google for reasons for this code (existing or old bugs, etc.) but found none; anyone with experience in Struts can provide a reason that justifies the existence of such a code and so much (apparently) wasted effort? I want to know if I am free to delete such code to simplify the methods (which are messy enough without these "improvements").

(*) There are a few instances in which the code does not appear, but it smells just of the programmer forgetting it.

I don't believe there's any good use-case for this code.  I know it makes for a boring answer, but sometimes the correct thing really is simple and boring.  As you say, the Struts framework should always redirect to the appropriate method and fail entirely otherwise.

Even the scant lines you posted have some bad/useless programming practices, so my guess is that the developer thought he was being thorough and clever by tacking on needless framework.

if (method != null && "myMethod".equals(method)) {
    // Perform the actual work

He already checks for method != null and then checks for "myMethod".equals(method).  The only reason to use the second construction for a String comparison is to avoid a NullPointerException... which he's already done by checking against null.  So either he doesn't know that Java short-circuits conditions, or he has no idea what the purpose is behind the second construction.

Also, it's generally a bad idea to have huge wrappers around entire method bodies.  It would be a better design to have something like this (if it were needed at all):

if(method == null || !method.equals("myMethod") {
    return;
}

//do stuff

... Or you could write the conditional as if(!"myMethod".equals(method)), but I honestly really hate the flipped comparison rather than explicitly checking for null.  I suppose it's a matter of taste, though.

Anyway, bottom line: no, there's no use for it, and there's evidence that the original programmer didn't entirely know what he was doing, anyway.


HackerEarth Girlfriend's Demand challenge, solved two ways I solved the Girlfriend's Demand challenge on HackerEarth.  In summary:

  Input: First line of the input contains a string S. Next line contains an integer Q, the number of queries.  Each of the next Q lines contains a test case consisting of 2 integers, a and b.

  Output: If S is repeated infinitely, would string positions a and b contain the same character (using 1-based indexing)?  For each query, print “Yes” or “No”.

  Constraints:

  1 ≤ |S| ≤ 105
  1 ≤ Q ≤ 105
  1 ≤ a, b ≤ 1018

  Sample Input:

vgxgp
3
2 4
2 5
7 14

  Sample Output:

Yes
No
Yes

My first solution:

public static void main(String args[] ) throws Exception {
      BufferedReader br = new BufferedReader(new InputStreamReader(System.in));
        String item=br.readLine();

        StringBuffer sb=new StringBuffer();
        int len = item.length();
        int inputs = Integer.parseInt(br.readLine());

        while(inputs-->0)
        {
            String strarr[] =null;

                strarr = br.readLine().split(" ");

            long a=Long.parseLong(strarr[0]);
            long b=Long.parseLong(strarr[1]);
            a=a-1;
            b=b-1;
            a=a%len;
            b=b%len;

            if(item.charAt((int)a)==item.charAt((int)b))
                sb.append("Yes\n");
            else
                sb.append("No\n");

        }

        System.out.println(sb);

    }

While this gave me an output in almost 5.2954 seconds for various inputs, when I tried the same inputs on this version of the code that I wrote:

public static void main(String args[] ) throws Exception {
      BufferedReader reader= new BufferedReader(new InputStreamReader(System.in));
        String demanded=reader.readLine();
        int length = demanded.length();
        int T=Integer.parseInt(reader.readLine());
        for(int i = 0;i < T; i++){
            String[] pairedAAndB=reader.readLine().split(" ");
                long a = ((Long.parseLong(pairedAAndB[0]) - 1) % length);
                long b = ((Long.parseLong(pairedAAndB[1]) -1) %length);
            System.out.println((demanded.charAt((int)a)==demanded.charAt((int)b)) ? "Yes" : "No");

    }

}

It took around 15.7819 seconds. What is the best way to convert long to int optimally? What I'm not able to understand is why the second code snippet is taking so much time when the second version is almost similar. There must be something about the second version that I'm not able to understand.

Slowness caused by System.out.println
The difference between the first and second programs isn't the parsing or long/int conversion.  The difference is that in the first program, you use a StringBuffer to build your output into one big string (which is good).  In the second program, you call System.out.println() for each of your 100000 lines of output.  This takes a significant amount of time.
The current code
I don't see any problem with your first program.  It runs about as well as can be expected.  There are some strange indentations in your code and a few places where I would have added some spaces, but otherwise I don't see other problems.  I played around with it and got it to run about 8% faster by changing the parsing, but it wasn't really significant.  For your reference, here is the modified code:
public static void main(String args[] ) throws Exception {
    BufferedReader br        = new BufferedReader(
                                    new InputStreamReader(System.in));
    String         item      = br.readLine();
    int            len       = item.length();
    int            numInputs = Integer.parseInt(br.readLine());
    StringBuffer   output    = new StringBuffer(numInputs * 4);

    while (numInputs-- > 0) {
        String line  = br.readLine();
        int    space = line.indexOf(' ');
        long   a     = Long.parseLong(line.substring(0, space-1));
        long   b     = Long.parseLong(line.substring(space+1));

        a = (a-1) % len;
        b = (b-1) % len;

        if (item.charAt((int)a) == item.charAt((int)b))
            output.append("Yes\n");
        else
            output.append("No\n");
    }

    System.out.println(output);
}


Static typed Ids (integers) in C# Within our application I need to work a lot with simple IDs (just integers).

They are used to make queries against a database. For performance reasons I don't want to materialize them to Entities before the query.

Example:

// not really *that* static typed
public void DoQuery(int userId, int groupId)
{
}

Because I want to make our application more static typed (preventing errors like accidentally switching parameters around), I'm looking for a way to make this all more static typed, like:

public struct UserId
{
     int _UserId;

     public UserId(int userId)
     {
          _UserId = userId;
     }

    public static implicit operator int (UserId userId)
    {
        return userId._Id;
    }
}

and then:

// will throw compilation errors when I try to shoehorn a UserId into a GroupId
public void DoQuery(UserId userId, GroupId groupId)
{
}

Do I need to override equals operators and hascode, or would this be enough?
Are there any important performance penalties (during a highload)? For i.e. garbage collection.
What could be any more negative consequences from this?

Do I need to override equals operators and hascode, or would this be enough?

Structs automatically implement Equals() and GetHashCode(), but that implementation is slow. If you're actually going to use them (even indirectly, e.g. by using some LINQ methods), and you care about performance, you should override them.

  Are there any important performance penalties (during a highload)? For i.e. garbage collection.

There shouldn't be. GC doesn't get involved (unless you're going to box the values, but that's the same for integers).

I'm not sure if the runtime can optimize code using UserId as well as it could for an integer, so for performance critical code, you should make sure it's okay by profiling.

  What could be any more negative consequences from this?

I think this is not a usual pattern in C#, so one negative consequence might be some confusion by your coworkers.


Correctly using promises with node-mysql I have been trying to do a simple query and return the results with node-mysql however I had several problems. Originally I wrote it where there was no promise, but if I tried to access user after just doing a query if returned undefined
The solution I came up with is the following, is this a good way of doing it? I need to wait until the results come back so I can then send a signal to the frontend to say the player is loaded and can play. Do I need to create a new promise for each query? I have a few tables I need to get data from (That can't be done with joins)
class Player {
    constructor(user_id, socket, connection) {
        this.socket = socket;
        this.connection = connection;

        this.loadUser(user_id)
            .then(() => {
                console.log("loaded user", this.user);
            });
    }

    async loadUser(user_id) {
        let _self = this;
        return new Promise((resolve, reject) => {
            _self.connection.query(`SELECT * FROM users WHERE id = ${user_id}`, (err, res) => {
                this.user = res[0];
                resolve()
            });
        })
    }
}
```

Remove the async method call from the constructor, and make the async method return the instance (this):
class Player {
  constructor(user_id, socket, connection) {
    this.socket = socket;
    this.connection = connection;
  }

  async loadUser() {
    let _self = this;
    return new Promise((resolve, reject) => {
      _self.connection.query(`SELECT * FROM users WHERE id = ${this.user_id}`, (err, res) => {
        this.user = res[0];
        resolve(this)
      });
    })
  }
}

Then you can instantiate a new Player and wait for it to load the necessary data from the database, all in one line, like this:
const player = await new Player(user_id, socket, connection).loadUser();

Or like this:
new Player(user_id, socket, connection).loadUser().then(player => {

});


The C++ analog of norm in C I noticed that the C++ version of <complex> has norm.

However, I am programming in C, and there is no cnorm.  So I made one to define before my main function:

#include <complex.h>

double cnorm(double _Complex z)
{
    return cpow(creal(z),2) + cpow(cimag(z),2);
}

How would you improve this definition? Is the name ok?  or will it break when it is eventually added to <complex.h>.  Is the return type appropriate?  Would it be more consistent to use double _Complex?  And is the implementation efficient (for rapid numerical evaluation)?

I most cases the norm for complex numbers means the magnitude/absolute value, so if you want Gauss' definition you might think about a different name.

If you want it really fast and and with an error of at most one ulp it gets quite complicated. You might download a version of Cygwin's new-libc (e.g. from the git here: https://github.com/mirror/newlib-cygwin) and take a look at newlib/libm/math/e_hypot.c to get the gist of it.

What you have now is quite slow and imprecise, for two reasons:

the return of creal()/cimag() is a double, no need for the complex function cpow().
you have two squarings and one addition; all three together have a good chance of unnecessarily adding error.

There is the function cabs() which computes the Euclidean norm sqrt(a²+b²). I would propose to use that function and simply square the outcome if you do not want to go the long and complicated way. If cabs() is implemented well (error less than 1 ulp) you should get a good error value for your own function, too.


Prevent Variable Overflow from InputBox user error I was writing some code that required the user to input a number, which was then stored in a variable.  While testing the code I input a number larger than the variable type would allow and I got an variable overflow error.  So I wrote this code to prevent that from happening. It works, but I was hoping there was a more concise way to do it, or even a function already part of VBA that I could use.

Sub PreventOverflow()

    Dim IsThisANumber As String
    Dim Number As Integer
    Dim PreventOverflow As Long
    Dim Cancel As VbMsgBoxResult

    Number = 0
    PreventOverflow = 0

    IsThisANumber = InputBox("Enter a number less than 32768")

    Do While Number = 0

        Do While PreventOverflow = 0
            If IsNumeric(IsThisANumber) = True Then
                If Len(IsThisANumber) < 6 Then
                    PreventOverflow = IsThisANumber
                Else
                    IsThisANumber = InputBox("Your number was not less than 32768" & vbNewLine & "Please enter a number less than 32768")
                    PreventOverflow = 0
                End If
            ElseIf IsThisANumber <> "" Then
                IsThisANumber = InputBox("Your did not enter a number" & vbNewLine & "Plese enter a number less than 32768")
                PreventOverflow = 0
            Else
                Cancel = MsgBox("You did not enter a number.  Do you want to continue?", vbYesNo)
                If Cancel = vbNo Then
                    Exit Sub
                Else
                    IsThisANumber = InputBox("Enter a number less than 32768")
                    PreventOverflow = 0
                End If
            End If
        Loop

        If PreventOverflow < 1 Or PreventOverflow > 32767 Then
            IsThisANumber = InputBox("Your number was not less than 32768" & vbNewLine & "Please enter a number less than 32768")
            PreventOverflow = 0
        Else
            Number = PreventOverflow
        End If
    Loop

End Sub

I think you over engineered this function a bit, which is easy to do. Simply store your value in a variant to check if it will fit in an integer.

 Private Function IsInteger(ByVal prompt As String) As Integer
    Dim longHolder As Variant
    longHolder = InputBox(prompt)
    If IsNumeric(longHolder) Then
        If Int(longHolder) = CDbl(longHolder) Then
            If Abs(longHolder) < 32768 Then
                IsInteger = longHolder
            Else
                MsgBox "Too Big"
                IsInteger = 0
            End If
        Else
            MsgBox "Decimals!"
            IsInteger = 0
        End If
    Else
        MsgBox "Not a number"
        IsInteger = 0
    End If
End Function

You would use it like this

Sub test()
    Dim x As Integer
    x = IsInteger("give me an integer")
End Sub

That being said, integers are obsolete. According to msdn VBA silently converts all integers to long. So, just use Long to begin with.


Single element from iterator When I was working in Rust this week, I missed kotlin's fun <T> Sequence<T>.single(): T. Basically, calling this function means that you are asserting that the sequence contains only one element and that you want that one element back. The most common use case is you've filtered a sequence down to one element. It's also clearer than using first() for the same task as it is clear that there is only supposed to be one thing and that if there is more than one you need to crash because your assumptions don't hold anymore.

I wanted it in Rust, so I implemented it and put it on crates.io. Before I go 1.0.0, however, I'd like to ask for review, mostly focusing on usability of the crate (thus design) and style of the code.

//! This crate exposes a `Single` trait for extracting the element from a
//! single-element iterator (or `panic!`ing if that precondition is false).

use std::{fmt, error, result};

type Result<T> = result::Result<T, self::Error>;

/// Trait to extract the element from a single-element iterator.
pub trait Single {
    /// The item type of the wrapped iterator.
    type Item;

    /// Get the single element from a single-element iterator.
    ///
    /// Note that many iterators return references to the elements,
    /// so this method will as well if the backing iterator does.
    ///
    /// # Examples
    ///
    /// ```
    /// # use single::{ self, Single };
    /// # use std::iter;
    /// assert_eq!(iter::empty::<i32>().single(), Err(single::Error::NoElements));
    /// assert_eq!(iter::once(0).single(), Ok(0));
    /// assert_eq!(iter::repeat(0).single(), Err(single::Error::MultipleElements));
    /// ```
    fn single(self) -> Result<Self::Item>;
}

/// An error in the execution of [`single::Single::single`](trait.Single.html#tymethod.single).
#[derive(Clone, Copy, PartialEq, Eq)]
pub enum Error {
    /// Asked empty iterator for single element.
    NoElements,
    /// Asked iterator with multiple elements for single element.
    MultipleElements,
}

impl fmt::Debug for Error {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        fmt::Display::fmt(self, f)
    }
}

impl fmt::Display for Error {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        match *self {
            Error::NoElements => write!(
                f,
                "SingleError::NoElements: {}",
                error::Error::description(self),
            ),
            Error::MultipleElements => write!(
                f,
                "SingleError::MultipleElements: {}",
                error::Error::description(self),
            ),
        }
    }
}

impl error::Error for Error {
    fn description(&self) -> &str {
        match *self {
            Error::NoElements => "Asked empty iterator for single element",
            Error::MultipleElements => "Asked iterator with multiple elements for single element",
        }
    }
    fn cause(&self) -> Option<&error::Error> { None }
}

impl<I> Single for I where I: Iterator {
    type Item = <Self as Iterator>::Item;

    fn single(mut self) -> Result<Self::Item> {
        match self.next() {
            None => Err(Error::NoElements),
            Some(element) => if self.next().is_none() {
                Ok(element)
            } else {
                Err(Error::MultipleElements)
            }
        }
    }
}

#[cfg(test)]
mod test {
    use std::iter;
    use super::Single;

    #[test]
    #[should_panic(expected = "Asked empty iterator for single element")]
    fn panic_empty() { let _: i32 = iter::empty().single().unwrap(); }

    #[test]
    #[should_panic(expected = "Asked iterator with multiple elements for single element")]
    fn panic_multiple() { let _ = iter::repeat(0).single().unwrap(); }
}

Future growth by optimization(?) with std::iter::ExactSizeIterator and/or std::iter::TrustedLen may be possible in the future, but would require benchmarking to see if it is worthwhile.

"`panic!`ing" would normally be written as "panicking". Strangely, it doesn't appear your code actually... panics though? Seems your docs are misleading or outdated.
I dislike the common repetition in documentation for an X that starts with an equivalent of "The X is". Try removing those and improving the information density of the docs. We can already see it's an X from the doc tool itself.
Since the crate is so narrowly focused, I'd recommend moving at least one (if not all) the examples from the method to the crate level. This allows users to immediately see what it does and how to use it.
There's no reason to use self:: in the custom Result.
There's never a time where you'd implement this trait for anything but an iterator, so your trait should require Iterator as a supertrait. This has the benefit of unifying the Item types.
The note about "many iterators return references to the elements", seems out-of-place; that's just how iterators work. If someone doesn't understand that, it seems like they should re-read The Rust Programming Language. For similar reasons, I wouldn't explain how to use a trait or how to add dependencies to the Cargo.toml in my crate docs.
Avoid referencing your crate name in the doc prose. A user can choose to import it with whatever name they like.
Consider a nested match in the iterator implementation.
The majority of your code appears to be dealing with the error type. Perhaps a crate like quick-error would help tip the balance back towards the interesting code.
It's strange to have Display and Debug be the same value. Recall that Debug is for programmers; we'd expect to see the variant name. Display is for humans that see the error.
I'd delegate your Display implementation to Error::description as adding the names of the variants doesn't help the humans reading it.
I'm not a fan of multi-line expressions that aren't blocks in a match (like your write! or if). Would promote them to blocks.

//! Provides the `Single` trait for extracting the element from a
//! single-element iterator.

use std::{fmt, error, result};

type Result<T> = result::Result<T, Error>;

/// Extract the element from a single-element iterator.
pub trait Single: Iterator {
    /// Get the single element from a single-element iterator.
    ///
    /// # Examples
    ///
    /// ```
    /// # use single::{ self, Single };
    /// # use std::iter;
    /// assert_eq!(iter::empty::<i32>().single(), Err(single::Error::NoElements));
    /// assert_eq!(iter::once(0).single(), Ok(0));
    /// assert_eq!(iter::repeat(0).single(), Err(single::Error::MultipleElements));
    /// ```
    fn single(self) -> Result<Self::Item>;
}

/// An error in the execution of [`Single::single`](trait.Single.html#tymethod.single).
#[derive(Clone, Copy, PartialEq, Eq)]
pub enum Error {
    /// Asked empty iterator for single element.
    NoElements,
    /// Asked iterator with multiple elements for single element.
    MultipleElements,
}

impl fmt::Debug for Error {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        fmt::Display::fmt(self, f)
    }
}

impl fmt::Display for Error {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        match *self {
            Error::NoElements => {
                write!(
                    f, "SingleError::NoElements: {}",
                    error::Error::description(self),
                )
            }
            Error::MultipleElements => {
                write!(
                    f, "SingleError::MultipleElements: {}",
                    error::Error::description(self),
                )
            }
        }
    }
}

impl error::Error for Error {
    fn description(&self) -> &str {
        match *self {
            Error::NoElements =>
                "Asked empty iterator for single element",
            Error::MultipleElements =>
                "Asked iterator with multiple elements for single element",
        }
    }
    fn cause(&self) -> Option<&error::Error> { None }
}

impl<I> Single for I where I: Iterator {
    fn single(mut self) -> Result<Self::Item> {
        match self.next() {
            None => Err(Error::NoElements),
            Some(element) => {
                match self.next() {
                    None => Ok(element),
                    Some(_) => Err(Error::MultipleElements),
                }
            }
        }
    }
}

#[cfg(test)]
mod test {
    use std::iter;
    use super::Single;

    #[test]
    #[should_panic(expected = "Asked empty iterator for single element")]
    fn panic_empty() { let _: i32 = iter::empty().single().unwrap(); }

    #[test]
    #[should_panic(expected = "Asked iterator with multiple elements for single element")]
    fn panic_multiple() { let _ = iter::repeat(0).single().unwrap(); }
}


Determine added, removed and kept items between two lists My code is determining the difference between two iterables in terms of items that have been addded, removed and kept. The items are assumed to be sortable and there are no duplicates.

def diff(before, after):
    """Return diff between iterables of sortable items `before` and `after` as
    tuple of three lists of items that have been added, removed and kept.
    """
    before_iter = iter(sorted(before))
    after_iter = iter(sorted(after))
    add_items = []
    rem_items = []
    keep_items = []
    try:
        new = old = None
        while True:
            if new == old:
                if new is not None:
                    keep_items.append(new)
                new = old = None
                new = next(after_iter)
                old = next(before_iter)
            elif new < old:
                add_items.append(new)
                new = None
                new = next(after_iter)
            elif old < new:
                rem_items.append(old)
                old = None
                old = next(before_iter)
    except StopIteration:
        if new is not None:
            add_items.append(new)
        if old is not None:
            rem_items.append(old)
        add_items += list(after_iter)
        rem_items += list(before_iter)
    return add_items, rem_items, keep_items

if __name__ == '__main__':
    assert diff([], []) == ([], [], [])
    assert diff([1, 2, 3], [4, 3, 2]) == ([4], [1], [2, 3])
    assert diff(['foo', 'bar'], ['baz', 'bar']) == (['baz'], ['foo'], ['bar'])

Main concerns:

The way I'm working with iterators seems a bit awkward and appears to unnecessarily bloat the code. I feel there must be a more elegant pattern, maybe without iterators at all?
The code should perform well on large lists. That's also why I'm not using multiple set operations instead. (add_items = set(after) - set(before), etc.) Or can I solve this efficiently with set operations? (I'm expecting both very small and very large differences between the iterables.)

Update: Unfortunately, I failed to include a key requirement into the question which therefore couldn't be considered in the answers. But I'll state it here, just for the record:

The algorithm needs to perform well when comparison between the items is expensive. Since an approach using set operations incorporates more comparisons than the algorithm above, it performs worse in my use case.

First, let's look at the naive implementation using sets:

def diff_set(before, after):
    b, a = set(before), set(after)
    return list(a - b), list(b - a), list(a & b)

This is (so much more) readable and understandable than your code. The question is, is this significantly slower than your code?

Here are timings of your three example cases:

             Input                     diff             diff_set         diff_counter
 ------------------------------ ------------------- ------------------- -------------------
  [], []                         1.72 µs ± 29.9 ns   1.21 µs ± 5.34 ns   9.91 µs ± 91.4 ns
  [1,2,3], [4,3,2]               3.16 µs ± 183 ns    1.57 µs ± 28.4 ns   12 µs ± 180 ns
  ['foo','bar'], ['baz','bar']   2.77 µs ± 83.4 ns   1.56 µs ± 37.7 ns   11.5 µs ± 240 ns

The set implementation is faster in all three cases. But what about the scaling behavior? First lets set up some random test data of increasing length. Note that I scaled the number of available values up with the size of the lists, so that set(before), set(after) does not become trivial and so that there are both added and removed values. The only way in which this might be unrealistic is that both before and after are always of the same size.

import random
random.seed(42)
random_data = [([random.randrange(2*n) for _ in range(n)], [random.randrange(2*n) for _ in range(n)]) for n in range(1, 10000, 100)]

Here also the set implementation wins:

In other words: always try the naive approach first and then test to see if the more complicated approach actually improves anything.

For reference, here the implementation of diff_counter I used:

from collections import Counter

def diff_counter(before, after):
    c = Counter(after)
    c.subtract(before)
    return list(+c), list(-c), [elem for elem, count in c.items() if count == 0]


Backing up a file to prevent its corruption during processing My program is iterating over list of files and processing each of them using some library. It's possible that file might be corrupted in the process so I want to backup it to prevent potential corruption.

I implemented it this way:

private static void ComprocessFiles(List<string> files)
{
    FileInfo file = null;
    foreach (string filepath in files)
    {
        bool backedUp = false;
        bool keepBackup = false;
        try
        {
            file = new FileInfo(filepath);
            if (!file.Exists) continue;
            if (file.IsReadOnly) continue;

            BackupHelper.Backup(file);
            backedUp = true;
            ProcessFile(file);

            BackupHelper.RestoreTimestamps(file);
            file.Refresh();
        }
        catch (Exception ex)
        {
            try
            {
                Logger.LogSkippedFile(ex.GetType().Name, ex.Message, filepath);
                if (backedUp) BackupHelper.RestoreFile(file);
            }
            catch (Exception ex)
            {
                keepBackup = true;
            }
            // Listed exceptions are not critical.
            if (!(ex is IOException || ex is UnauthorizedAccessException || ex is System.Security.SecurityException))
            {
                throw;
            }
        }
        finally
        {
            if (!IsCorrupted(file)) keepBackup = true;
            if (!keepBackup) BackupHelper.DeleteBackup(file);
        }
    }
}

BackupHelper class:

static class BackupHelper
{
    static private DateTime[] timestamps = new DateTime[3];

    static internal void Backup(FileInfo originalFile)
    {
        timestamps[0] = originalFile.CreationTime;
        timestamps[1] = originalFile.LastWriteTime;
        timestamps[2] = originalFile.LastAccessTime;

        File.Copy(originalFile.FullName, originalFile.FullName + "_backup", true);
    }

    static internal void Backup(string originalFile)
    {
        Backup(new FileInfo(originalFile));
    }

    static internal void RestoreFile(FileInfo originalFile)
    {
        File.Copy(originalFile.FullName + "_backup", originalFile.FullName, true);
    }

    static internal void RestoreFile(string originalFile)
    {
        RestoreFile(new FileInfo(originalFile));
    }

    static internal void RestoreTimestamps(FileInfo originalFile)
    {
        originalFile.CreationTime   = timestamps[0];
        originalFile.LastWriteTime  = timestamps[1];
        originalFile.LastAccessTime = timestamps[2];
    }

    static internal void RestoreTimestamps(string originalFile)
    {
        RestoreTimestamps(new FileInfo(originalFile));
    }

    static internal void DeleteBackup(FileInfo originalFile)
    {
        timestamps = new DateTime[3];
        File.Delete(originalFile + "_backup");
    }

    static internal void DeleteBackup(string originalFile)
    {
        DeleteBackup(new FileInfo(originalFile));
    }
}

I tried my best as I plan to publish source code online but still feel that there is room for improvement.

I guess methods ProcessFile() and IsCorrupted() are not important for that question as well as Logger class.

Important note - StackOverflowException and OutOfMemoryException can't be thrown by this code as it don't do many method calls and don't use much memory.

How great is the risk that your files will be corrupted? Is there a particular reason why you think this might happen, and if so, is that cause something you can address?

Are you concerned that it is your act of processing the file that might corrupt it? If so, you may want to do the opposite of what you're doing. Don't create a backup, but instead leave the original alone and create a working copy which you discard when you're done. If something goes wrong then at least your original file is still there, untouched.

This may sound facetious but it's not: If you don't trust your file system then it's not a good place for your application's data. How do you know that even after backing up, the file you're reading won't become corrupt as your're reading it? How do you know it wasn't already corrupt when it was backed up? How do you know that the backups won't be corrupted?

The best approach depends not only on how much you trust (or distrust) your storage but also how great the risk is if your data gets lost. If your server gets backed up regularly then that might be all the protection you need. Perhaps you could create a hash or a checksum for each file and store it with the file. When you process the file, validate that it matches the hash. That gives you about as much certainty as you could possibly want.


Compare two lists to see if one is a rotation of the other 2 Second attempt to solve this rotation problem, implemented a double ended queue as was suggested in a comment of my first attempt. So did I finally do the problem some justice or is there perhaps a better solution? Also anything I can and should clean up? I wanted to thank everyone from my previous post for their help and suggestions!

'''
Attempt 2
Problem statement: Given 2 integer arrays, determine if the 2nd array is a rotated version of the 1st array.
Ex. Original Array A={1,2,3,5,6,7,8} Rotated Array B={5,6,7,8,1,2,3}

@author: Anonymous3.1415
'''

from collections import deque

def is_rotated(lst1, lst2):
    '''is lst2 a rotation of lst1 '''

    if len(lst1) != len(lst2):
        return False
    if lst1 == [] and lst2 == []:
        return True

    d_lst1 = deque(lst1)
    d_lst2 = deque(lst2)

    #rotate all possible rotations to find match
    for n in range(len(d_lst1)):
        d_lst2.rotate(n)
        if d_lst2 == d_lst1:
            return True
        d_lst2.rotate(-n)
    return False

# rotation
lst1, lst2 = [1,2,3,4,6,4,7], [6,4,7,1,2,3,4]
assert is_rotated(lst1, lst2)

# rotation with repeated numbers
lst1, lst2 = [1,2,3,4,6,4,7,1], [6,4,7,1,1,2,3,4]
assert is_rotated(lst1, lst2)

# different set
lst1, lst2 = [1,2,3,4,6,4,6], [6,4,7,1,2,3,4]
assert not is_rotated(lst1, lst2)
lst1, lst2 = [1,2,3,4,6,4,7], [6,4,6,1,2,3,4]
assert not is_rotated(lst1, lst2)

# equal
lst2 = lst1
assert is_rotated(lst1, lst2)

# empty
lst1, lst2 = [], []
assert is_rotated(lst1, lst2)

# 1 empty, 1 not empty
lst1, lst2 = [], [1]
assert not is_rotated(lst1, lst2)
lst1, lst2 = [1], []
assert not is_rotated(lst1, lst2)

If you compare one of the lists with all the rotations of the other list, this will take \$\Theta(n^2)\$ in the worst case.

For example, if we make test cases like this:

from timeit import timeit
def test(n):
    l1 = [0] * n
    l2 = [0] * (n - 1) + [1]
    return timeit(lambda:is_rotated(l1, l2), number=1)

then the quadratic runtime can be clearly seen in the timings:

>>> test(10**3)
0.009426131844520569
>>> test(10**4)
0.5701698779594153
>>> test(10**5)
57.70295810396783

There are a couple of ways to solve this problem in linear time:

Search for one list in the concatenation of the other list with itself, using a search algorithm that has linear time in the worst case, for example Knuth–Morris–Pratt.

Here's an illustration of this approach using strings instead of lists:

>>> a, b = 'basketwork', 'workbasket'
>>> a in b + b
True

Find the lexicographically minimal rotation of each of the lists to be compared (for example using Booth's algorithm) and see if the results are the same.