Website for allowing data evaluation from submission I'm making a really basic website where users essentially just submit data into some forms and they will be able to review their data in another page. I feel like I'm going to be repeating myself far too much.
This is how I'm planning to display the users records, the trouble I'm having is I have about six more tables of data that is slightly different. So I can't just hard-code a template because the number of columns and the names of the columns is slightly different in each table. I know I could just make six different HTML files but that just seems silly. I feel like I need to somehow loop over the attributes of the models and then somehow display them in the template?
Is there a better way of displaying database data in a template?
class (db.Model):
    __abstract__ = True
    employee_name =  db.Column(db.String(50), nullable=False)
    date_submitted = db.Column(db.DateTime, nullable=False, default=datetime.utcnow)
    comment =  db.Column(db.String(100), nullable=True)

class FridgeFreezerTempTable(Base):
    id = db.Column(db.Integer, primary_key=True)
    unit_name = db.Column(db.String(50), nullable=False)
    unit_type = db.Column(db.String(50), nullable=False)
    temperature = db.Column(db.String(10), nullable=False)
    company_id = db.Column(db.Integer, db.ForeignKey('user.id'),nullable=False)

@app.route("/unit_temp_records/<int:page_number>")
def unit_records(page_number):
    """
    records = pagiantion object
    page number = the current page

    """
    records = FridgeFreezerTempTable.query.filter_by(user=current_user).paginate(per_page=5,
             page=page_number, error_out=True)
    return render_template("unit_temp_records.html", user_records=records, table=table)

{% extends "records.html"%}

{%block recordcontent%}

            #
            Unit Name
            Unit Type
            Temperature
            Employee Name
            Comment
            Date Submitted

        {% for record in user_records.items %}

                {{ loop.index }}
                {{record.unit_name}}
                {{record.unit_type}}
                {{record.temperature}}

                {{record.employee_name}}
                {{record.comment}}
                {{record.date_submitted}}

        {% endfor %}

    {% for page in user_records.iter_pages() %}
    {% if page %}
    {{ page }}
    {% else %}
    .
    {% endif %}
    {% endfor %}

{% endblock%}
```

Perhaps the templates can be generated by a function.  Pass in a list of the attribute names, and it returns a template string.  Or pass in the table name, and query the column names to make the template.  Then use render_template_string() instead of render_template().

This code is rough and untested, but outlines what I'm thinking:

def column_headers(attrs):
    headers = (f'{attr.replace("_"," ").titlecase()}	'
               for attr in attrs)
    return '\n'.join(headers)

def columns(attrs):
    column_refs = (f'{{record.{attr}}}	' for attr in attrs)
    return '\n'.join(column_refs)

def make_template(attrs):
    template_string = ["""
        {% extends "records.html"%}
        {%block recordcontent%}

                """,
                column_headers(attrs),
                """

                {% for record in user_records.items %}

                    {{ loop.index }}
                    """,
                    columns(attrs),
                    """

                {% endfor %}

        {% endblock%}
        """]
        return '\n'.join(template_string)

FridgeFreezerTemplate = make_template("""
                                      unit_type
                                      temperature
                                      employee_name
                                      comment
                                      date_submitted
                                      """.strip().split())

@app.route("/unit_temp_records/<int:page_number>")
def unit_records(page_number):
    """
    records = pagiantion object
    page number = the current page

    """
    records = FridgeFreezerTempTable.query.filter_by(user=current_user).paginate(per_page=5,
             page=page_number, error_out=True)
    return render_template_string(FridgeFreezerTemplate, user_records=records, table=table)


Improving a shortcode replacement routine PHP and regex I am using wordpress to allow a client to enter a curriculum vitae and have it output the correct and consistent html formatting:

So I have a plain text box custom field and am having them enter in the content in the following way:

[section "Solo Exhibitions"]
[year 2011]

Inner Space
January 11

Peter Pan
February 11

[section "Group Shows"]
[year 2009]

Group Show
January 09

Big Group Show
February 09

So on and so forth. I wrote the following to parse that text field and output the correct HTML formatting, but I think it is pretty brute force-- not very much finesse.

$cv = get('cv_text');
$cv = strip_tags($cv);

// Remove empty line breaks
$cv = preg_replace("/(^[\r\n]*|[\r\n]+)[\s\t]*[\r\n]+/", "", $cv);

// Explode line breaks
$cv = explode("\n", $cv);

$i = 0;

foreach($cv as $line) {
    if( preg_match('/\[section \"(.*)\"\]/', $line) ) {
        echo preg_replace('/\[section \"(.*)\"\]/', '
$1
', $line);
    } elseif( preg_match('/\[year (\d*)\]/', $line) ) {
        echo preg_replace('/\[year (\d*)\]/', '
$1
', $line);
    } else {
        // Count every two lines and wrap in a
 tag
        if($i === 1) {
            echo ''.$line.'
';
            $i = 0;
            continue;
        } else {
            echo '
'.$line.'

            ';
            $i++;
        }
    }
}

Can anyone recommend a way to improve my present code or present a better approach?

Thanks!

Why are you having them write that all in one box? And with custom tags no less!

I'd just create a form with each of these fields in it (section, year, event), with a JS "add" button below the event to add additional events to the DOM. Then if you want to give them the option of adding multiple sections at a time, then you can add another "add" button below the section that will duplicate an unmodified version of the original form so that they can do it all over again.

This is much better than having your client learn your custom tags for your program. And if you were to continue this route you'd end up recreating BB code, which there are plenty enough out there that you'd be better off downloading one instead. Not to mention that the above method is also easier on you. No need to create a custom parser at that point, just extract the proper keys from the POST data. I'm not going to put the code here, simply because a simple google search will find what you're looking for easier and faster than I could write it all here.

As for improvement to your code, I'd steer clear of Regex, but that's just me. I wasn't born on whatever planet that language came from.


How to clean the indexes, and ideally not create an additional array So I answered this question on SO and probably did someone's homework along the way.
In the original question the OP has the answer variable and wants it split to real and imaginary values with the original coefficient (q in the original post) as an array for each q from 1 to 10, so you get an array of arrays looking like:
[[1,-0.39002422, 1.84237253, -0.39002422, -1.84237253, -0.10997578, 0.51949688,-0.10997578, -0.51949688j],
[2, ...]
...
[10, ...]]

There are two things I'm unsatisfied with in the answer I gave that are the reason I'm asking for a CR.

The indexing: Isn't particularly readable, and I don't know how to make it more readable. For example [q-1, 2*_+1]. Would it have been better to create a variable j = q-1 because q-1 sees frequent use? Isn't that just moving the ugly to a different place?
The idea for the solution itself. The way I see it the original problem can be solved by splitting the answer array to a new array, but that feels somehow wrong. I can see several ways to create the new array, but I don't see a way to do it without creating the new array. Am I wrong and this is an OK solution or not, and why?

Thanks.
import numpy as np
export = np.empty([10,9])
for q in range(1,11):
    a0=1
    a1=3*q^2
    a2=4*q
    a3=np.sqrt(q)
    a4=q
    coeff=[a0, a1, a2, a3, a4]
    answer = np.roots(coeff)
    export[q-1, 0] = q
    for _ in range(len(answer)):
        export[q-1, 2*_+1] = answer[_].real
        export[q-1, 2*(_+1)] = answer[_].imag
with open("/tmp/latest_foo.csv", "ab") as output_file:
    np.savetxt(output_file, export, delimiter=",")

Here is a review of the solution.

^ is xor in Python. It is not for computation of exponentials.

When running code outside a method / class, it is a good practice to put the code inside a main guard. See here for more explanation.
 if __name__ == "__main__":
    ...

When you are providing quick answers on a forum, the guard might not always be necessary. However, if you are writing reusable code for yourself, it is better to adopt the practice.

By convention, _ is used to represent a don't-care variable, which won't be used after its value is assigned, e.g. v = [[] for _ in range(4)]. It is undesirable to refer to it in an expression like answer[_].

The output is a pure text file. It is unnecessary to open it in binary mode (b). The original question does not suggest an append mode (a) either.

The q-1 index would no longer work if q is changed to a different group of values. Therefore, it is better to use enumerate in this case:
for i, q in enumerate(RANGE_Q):
   ...
   export[i, 0] = q
   ...

A better approach is to use zip:
for q, output_row in zip(RANGE_Q, output_arr):
    ...
    output_row[0] = q
    ...

The assignments to the output numpy array can be improved, as shown in my solution below.

Here is my solution.
import numpy as np

if __name__ == "__main__":
    # Define constants
    RANGE_Q = range(1, 11)               # Range of q values
    POLY_DEG = 4                         # Degree of polynomial
    OUTPUT_PATH = "/tmp/latest_foo.csv"  # Output csv path

    # Compute roots
    roots_arr = np.array([np.roots([1, 3*q*q, 4*q, np.sqrt(q), q]) for q in RANGE_Q])

    # Construct output array and assign values
    output_arr = np.empty(shape=(len(RANGE_Q), POLY_DEG * 2 + 1))
    output_arr[:, 0] = RANGE_Q
    output_arr[:, 1::2] = roots_arr.real
    output_arr[:, 2::2] = roots_arr.imag

    # Save results to output file
    np.savetxt(OUTPUT_PATH, output_arr, fmt="%.4g", delimiter=',')


Count power of big numbers and then apply modulo on this numbers I have this example code in which I have very big numbers as you see. The problem is that when I use this algorithm for some small numbers, it works well, but when I use it for big numbers like in this example, this algorithm is so slow.

Can you tell me why this algorithm is so slow for that big numbers? How can I make it faster?

n = 11698030645065745910098695770921
e = 9569991810443215702618212520777
d = 7874909554574080825236064017913
m = 104228568138
m = int(m)
e = int(e)
n = int(n)
def preparation(m, n):
    block_lenght= len(str(n)) - 1
    m = [str(m)[i:i+block_lenght] for i in range(0, len(str(m)), block_lenght)]
    return m

def encrypt(m, n, e):
    m = preparation(m, n)

    power = [int(i) ** e for i in m]

    modulo = [i%n for i in power]

    total_sum = sum(modulo)

    return total_sum

m = encrypt(m, n, e)
print("m = ", m)

Your spelling of length is unconventional, and will cause errors.

Calculating the full result will generate a large number, slowing computation.  It's better to reduce the partial results as you compute the power, and keep the partial result mod n - you'll probably want to use binary exponentiation instead of the ** operator so that you can apply the modulus to the intermediate results.

I don't see what m = int(m) and similar lines are intended to achieve - probably worth a comment there.

How is the final (partial) block handled?  That seems to be missing altogether.


Seq[Either[A, B]] => Either[A, Seq[B]] Function I wrote a function of type Seq[Either[String,Int]] => Either[String, Seq[Int]]

scala> xs
res11: List[Either[String,Int]] = List(Right(1), Right(55))

scala> acc
res12: Either[String,Seq[Int]] = Right(List())

scala> xs.foldLeft(acc){ (acc, elem) => {
     |     acc.right.flatMap(list => elem.right.map(_ +: list) )
     | }}
res10: Either[String,Seq[Int]] = Right(List(55, 1))

Please review my foldLeft solution. Perhaps there's a higher order function that can more elegantly do this work?

A few points:

Your implementation reverses the order of elements if it succeeds (i.e. if they're all rights). I'm assuming this isn't intentional?
You're taking a list but returning a Seq. This may be what you want, but I find it a little confusing.
The inner curly brackets are unnecessary (and I personally find them kind of noisy).
I'd strongly suggest not shadowing the external acc with the accumulator function argument.

So I'd probably write it like this:

val xs: List[Either[String, Int]] = List(Right(1), Right(55))

xs.foldRight(Right(Nil): Either[String, List[Int]]) { (elem, acc) =>
  acc.right.flatMap(list => elem.right.map(_ :: list))
}

Or:

xs.foldRight(Right(Nil): Either[String, List[Int]]) { (elem, acc) =>
  for {
    t <- acc.right
    h <- elem.right
  } yield h :: t
}

But that's only if I couldn't use cats or Scalaz, both of which provide this operation as sequenceU:

scala> import cats._, cats.syntax.traverse._, cats.std.all._
import cats._
import cats.syntax.traverse._
import cats.std.all._

scala> val xs: List[Either[String, Int]] = List(Right(1), Right(55))
xs: List[Either[String,Int]] = List(Right(1), Right(55))

scala> xs.sequenceU
res0: scala.util.Either[String,List[Int]] = Right(List(1, 55))

Which is much nicer.


Java Properties Wrapper I've decided to make a wrapper class for the java.util.Properties class since, in its current state, it only allows for storing and reading String values.

Is there any way I can improve the following code?

public final class Prefs {

    private static final Properties properties = new Properties();
    private static OutputStream out;
    public static final File test = new File("settings.properties");

    public static void main(String[] args) throws IOException {
        try {
            set("hello", "5");

            int hello = get("hello", 1337);
            System.out.println(hello == 5);

            set("hello", "34");
            hello = get("hello", 1337);
            System.out.println(hello == 34);

            remove("hello");
            hello = get("hello", 1337);
            System.out.println(hello == 1337);
        } finally {
            save();
        }
    }

    static {
        try {
            if (!test.exists()) {
                test.createNewFile();
            }
            properties.load(new FileInputStream(test));
            out = new FileOutputStream(test);
        } catch (IOException e) {
            e.printStackTrace();
        }
    }

    public static <T> T get(String key, T defaultValue) {
        if (!properties.containsKey(key)) {
            return defaultValue;
        }
        String value = properties.getProperty(key);
        if (defaultValue instanceof Long) {
            return (T) new Long(value);
        } else if (defaultValue instanceof Integer) {
            return (T) new Integer(value);
        } else if (defaultValue instanceof Short) {
            return (T) new Short(value);
        } else if (defaultValue instanceof Byte) {
            return (T) new Byte(value);
        } else if (defaultValue instanceof Double) {
            return (T) new Double(value);
        } else if (defaultValue instanceof Float) {
            return (T) new Float(value);
        } else if (defaultValue instanceof Boolean) {
            return (T) new Boolean(value);
        }
        return (T) value;
    }

    public static void set(String key, String value) {
        properties.setProperty(key, "" + value);
    }

    public static void remove(String key) {
        properties.remove(key);
    }

    private static final ReentrantLock lock = new ReentrantLock();

    public static void save() {
        lock.lock();
        try {
            properties.store(out, "");
            out.flush();
        } catch (Throwable e) {
            e.printStackTrace();
        } finally {
            lock.unlock();
        }
    }

}

Is the try-finally code block really required in your main() method?
Your get() fails if the default value is not any of those primitive wrapper classes. What if I want a BigDecimal instance? "1.0" can't be casted to that.
On a related note, you are relying on auto-boxing, which may lead to unexpected consequences:

// code compiles, but throws NullPointerException here
int npe = getValue("invalid-key", (Integer) null);

Maybe this is just a quick prototyping, but if you do decide to make this a full-fledged library class, you should not be having the main() method, the static initialization block, or your static fields. The fields should become class fields where Prefs needs to be instantiated. Testing should go into its own class.
On another related note, you don't necessarily need to create temporary files for testing. There is a Properties.load(Reader) method that will work with a StringReader, which can be used in turn to wrap your test Strings/lines.
I don't think there's a benefit to instantiating a FileOutputStream object immediately after you have load()-ed the file. Things may happen between the loading and storing of properties, so regardless of whether you have this object or not, the storing process may still fail afterwards. Also, it seems like you do not need to explicitly flush() after calling store() as that is already done according to the Javadoc:

  After the entries have been written, the output stream is flushed. The output stream remains open after this method returns.


Echo implemented in Java I implemented a simple version of echo(1) command utility. The program works as described in the man page: it writes to the standard output all command line arguments, separated by a whitespace and end with a newline. It can process the option -n that avoid to print the newline.

About my implementation, it is not complete, because it doesn't interpret common backslash-escaped characters (for example \n, \c, and so forth). i used a StringBuilder object to build the output string, because I'm not sure that the standard output is buffered. I also make some checks so the program can work without specifying any arguments.

You can compile the program with javac JEcho and run it with java JEcho <...>.

JEcho.java

/**
 * JEcho writes any command line argument to the standard output; each argument
 * is separated by a single whitespace and end with a newline (you can
 * specify '-n' to suppress the newline).
 *
 * This program doesn't interpret common backslash-escaped characters (for
 * exampe '\n' or '\c').
 */
public class JEcho {
    public static void main(String[] args) {
        boolean printNewline = true;
        int posArgs = 0;

        if (args.length > 0 && args[0].equals("-n")) {
                printNewline = false;
                posArgs = 1;
        }

        StringBuilder outputBuilder = new StringBuilder();

        for (; posArgs < args.length; posArgs++) {
            outputBuilder.append(args[posArgs]);
            outputBuilder.append(" "); // Separator.
        }

        // Remove the trailing whitespace at the end.
        int outputLength = outputBuilder.length();
        if (outputLength > 0)
            outputBuilder.deleteCharAt(outputBuilder.length() - 1);

        String output = outputBuilder.toString();

        if (printNewline)
            System.out.println(output);
        else
            System.out.print(output);
    }
}

If you're using Java 8, you can use StringJoiner.

/**
 * JEcho writes any command line argument to the standard output; each argument
 * is separated by a single whitespace and end with a newline (you can
 * specify '-n' to suppress the newline).
 *
 * This program doesn't interpret common backslash-escaped characters (for
 * exampe '\n' or '\c').
 */
public class JEcho {
    public static void main(String[] args) {
        boolean printNewline = true;
        int posArgs = 0;

        if (args.length > 0 && args[0].equals("-n")) {
                printNewline = false;
                posArgs = 1;
        }

        StringJoiner outputBuilder = new StringJoiner(" ");

        for (; posArgs < args.length; posArgs++) {
            outputBuilder.add(args[posArgs]);
        }

        String output = outputBuilder.toString();

        if (printNewline)
            System.out.println(output);
        else
            System.out.print(output);
    }
}


Class to verify if a rectification is upgradable Today I encountered something that I have been wondering about in the past a few times before. How can I refactor a method that has this format:

private boolean isRectificationUpgradable(Rectification rectification) {

    final boolean validType = rectification.getType() == RectificationType.VH
            || rectification.getType() == RectificationType.COL;

    if (!validType) {
        return false;
    }

    if (rectification.getStatus() != RectificationStatus.IN_PROGRESS && rectification.getStatus() != RectificationStatus.OPEN) {
        return false;
    }

    if (rectification.getVehicle() == null) {
        return false;
    }

    final List<CarPassCertificate> certificates = rectification.getVehicle().getCertificates();

    if (CollectionUtils.isEmpty(certificates)) {
        return false;
    }

    CarPassCertificate validCertificate = getValidCertificate(rectification);

    if (validCertificate == null) {
        return false;
    }

    return true;
}

When a rectification is created it has the status "open". An employee indicates that he started to handle it by putting it in the status "in progress".

The upgradable in this context means that sometimes the creator chose a certain type for the rectification, when in fact he could have picked a more accurate other type. In this case someone in the application can "upgrade" this rectification to the more specific type.

In code, this "upgraded" type is not modeled as another class as it is not really an upgrade, it always stays the same Rectification class and some shared additional fields are set to make it a bit more specific.

I think it is pretty readable but as more validations are added it can become very long. Any ideas?

This looks as if it is part of the upgrade code itself.  I think you have too low a level of abstraction in here and you are also mixing different concerns.  Also, your workflow just seems fragile.

Cars and certificates

You check that there is a car, then you check that it has any certificates, then you pass the rectification to getValidCertificate.  Either you only need to pass the certificate list or you don't need to do the "has car/certificates" check yourself, since getValidCertificates clearly has to do that itself to be able to return a CarPassCertificate (or null).

(Since starting this question, we've discussed this issue in comments and I see you've agreed that only getValidCertificates needs to do the check.  Nice.)

Workflow

How can a rectification be both open and in progress and yet not even have a car?  Even if it is legitimate to have one open with no car, I can't see how it is valid for an in-progress task.  I really think your type hierarchy may not properly model your workflow.

If a rectification may exist for a period without a car (let's think of it as a rectification request) but

Rectifications can only be actioned once a car is added
Cars are not removed from Rectifications once added

then you should have two different classes to represent them. The first type should not have a car field at all.  The second should not be creatable without being passed a non-null car object. Depending on the rest of your code, the second class could be a subclass of the first (with the addition of a car field amongst other things) or share an interface with the first (preferred option of those two) or the request could be an entirely separate class, replaced with a car-containing rectification by a factory.

Either way, isRectificationUpgradable only need specify the second type as input. And immediately a whole category of errors is eliminated.  There is no need to check for the existence of a car when it is guaranteed to be present.

If you do it this way, you never have to check for the presence of a real car.  Any method that depends on the existence of a car simply has to specify the car-owning type.

I would not be surprised if other stages in the lifecycle of rectifications can be treated this way.  If you create a common rectification interface but

only the OpenRectification class/interface (which has no employee field) has a toAssigned method which requires an employee object and returns an assignedRectification object
only the assignedRectification type has changeAssignedEmployee method and toInProgress methods
only thetoInProgress type has methods which can be used to (for example) log work done

then many more categories of error evaporate.


Partitioning an array of objects into four arrays by one attribute I have an array of API objects, below is an example of one object:

{
    description: "Get a list of most recent social alerts"
    example_input: ""
    example_output: "status"
    id: 19
    method: "GET"
    resource: "/api/alerts"
}

With my code below, I'm able to loop through the big Array, then filter out items into 1 of 4 arrays (for GET, POST, PUT and DELETE)

RestFactory.getREST().then(function(res) {
    filterArrays(res.data.rest_methods);
});

function filterArrays(data) {
    for (var i=0; i<data.length; i++) {
        if (data[i].method === 'GET') {
            getArray.push(data[i]);
        }
        else if (data[i].method === 'POST') {
            postArray.push(data[i]);
        }
        else if (data[i].method === 'PUT') {
            putArray.push(data[i]);
        }
        else if (data[i].method === 'DELETE') {
            deleteArray.push(data[i]);
        }
    }
}

I feel that this is not the most efficient way to accomplish this, how should this be refactored? Would you recommend lodash in this scenario?

This can easily be done with array.reduce

let resultsByMethod = data.reduce((results, result) => {
  if(!results[result.method]) results[result.method] = [];
  results[result.method].push(result);
  return results;
}, {});

The end result is something like:

{
  GET: [...],
  POST: [...],
  PUT: [...],
  DELETE: [...]
}

One disadvantage in your approach is when it encounters a method that's note one of the 4 you're comparing.


sorting a numerical list using basic scheme I would like some feedback on the appropriate way of ordering a numerical list using the most elementary scheme functions (sorry, i realise that elementary is not well defined).

By way of background: I've been learning a bit of scheme on my ipad, where I have only a basic implementation -- this is a nice challenge as much as a problem, as i have to figure out simple things like sorting a list for myself.

In doing this, i learnt that (append (cdr list) (car list)) is not what you might expect -- which is how i had originally wanted to implement this simple sort.

To hack around it, i discovered list-tail and reverse -- see the else clause below.

(define ra-sort
  (lambda (numList)
    (cond
      ((null? numList) '())
      ((= (car numList) (apply max numList))
       (cons (car numList) (ra-sort (cdr numList))))
      (else (ra-sort (append (list-tail numList (- (length numList) 1))
                             (reverse (list-tail (reverse numList) 1)))))
      )))

(define a '(99 1 6 12 101 2 67 -3))

> (ra-sort a)
(101 99 67 12 6 2 1 -3)

Now this seems to work, but all this repeated calling of reverse seems inefficient -- how should it be done?

Or am i missing something? Is it possible to make things work with the output of (append (cdr a) (car a))?

what does the notation even mean in (1 6 12 101 2 67 -3 . 99)? and how is it useful?

So to answer your specific question, yes you can greatly simplify your algorithm in a couple places:

(list-tail numList (- (length numList) 1))

So you're taking the last n - 1 elements of a list of length n? That's exactly:

(cdr numList)

As for this part:

(reverse (list-tail (reverse numList) 1))

So the list-tail will return a list of size 1, so you can drop the outer-most reverse entirely.  Secondly, what are you getting here? Just the first element right? Which is to say...

(car numList)

So putting that together, we end up with this (+/- missing parens)

(define ra-sort
  (lambda (numList)
    (cond
      ((null? numList) '())
      ((= (car numList) (apply max numList))
          (cons (car numList) (ra-sort (cdr numList))))
          (else (ra-sort (append (cdr numList) (list (car numList))))
      )))

Ok so now think about this algorithm. At each iteration, we're checking if the head is in the right place, if it is we go to the next element, else we rotate the whole array. Checking if the first element is the max takes O(n) time. We expect to have to do this O(n) times per element, for every element... so we end up with O(n^3).

A simple insertion sort, which is what I think you're trying to implement, should be only O(n^2). So try to redo your algorithm so that instead of just checking if the first elem is the maximum one, you actually find it. So your inner clause would be something like:

(cons maxElem                               # largest element
    (ra-sort (remove-elem numList maxElem)) # + rest of list
)

That'll get you to where you want to be. At least for this sorting algorithm.


Deleting millions of rows from a MSSQL server table This SQL query took 38 minutes to delete just 10K of rows. How can I optimize it?

Index already exists for CREATEDATE from table2.

declare @tmpcount int
declare @counter int

SET @counter = 0
SET @tmpcount = 1
WHILE @counter <> @tmpcount
BEGIN
    SET ROWCOUNT 10000

    SET @counter = @counter + 1

    DELETE table1
    FROM table1
    JOIN table2
        ON table2.DOC_NUMBER = table1.DOC_NUMBER
        AND table2.DOC_YEAR = table1.DOC_YEAR
    WHERE YEAR(CREATEDDATE) BETWEEN 2007 and 2009

    IF @@ROWCOUNT = 0
        BREAK
END

The performance problem is due to the non-sargable expression YEAR(CREATEDATE).  Applying a function to a column in a WHERE clause prevents efficient use of the index on the column.  Below is an alternative technique, which uses an inclusive start and exclusive end for the datetime range.

Note the use of SET ROWCOUNT is deprecated for DELETE, INSERT and UPDATE statements.  Use TOP instead.

DELETE TOP(10000) table1
FROM dbo.table1
JOIN dbo.table2
    ON table2.DOC_NUMBER = table1.DOC_NUMBER
    AND table2.DOC_YEAR = table1.DOC_YEAR
WHERE
    CREATEDDATE >= '20070101'
    AND CREATEDATE < '20100101';

I'm not sure the purpose of @tmpcount in your script other than perhaps controlling the number of iterations for testing.


Generating product variants in Ruby Based on the sample variants, I need to get all combinations of all variants. In the example I have 3x3x2=18 variants.
## SAMPLE VARIANTS
sizes = ['small', 'medium', 'large']
colors = ['red', 'green', 'blue']
materials = ['cotton', 'linen']

## ITERATE TO ALL VARIANTS
titles = []
sizes.each do |size|
  colors.each do |color|
    materials.each do |material|
      ## PUT THE VARIANT IN THE NEW ARRAY
      titles.push("#{size} - #{color} - #{material}")
    end
  end
end

puts titles.inspect

Is my nested each loop preferred or there is some better implementation for this?

Frozen string literals
Immutable data structures and purely functional code are always preferred, unless mutability and side-effects are required for clarity or performance. In Ruby, strings are always mutable, but there is a magic comment you can add to your files (also available as a command-line option for the Ruby engine), which will automatically make all literal strings immutable:
# frozen_string_literal: true

It is generally preferred to add this comment to all your files.
Word array "percent" literals
Ruby has special array literals for arrays of single-word strings that can make your code easier to read by reducing the amount of "syntax fluff" around the actual content.
The literal starts with the sigils %w or %W (think "word" or "witespace-separated"). %w behaves like a single-quoted string, i.e. does not perform interpolation and supports no escape characters other than \' and \\. %W behaves like a double-quoted string.
So, the beginning of your script could look like this:
# frozen_string_literal: true

## SAMPLE VARIANTS
sizes = %w[small medium large]
colors = %w[red green blue]
materials = %w[cotton linen]

As with all percent literals, you can freely choose the delimiter you want to use such that the delimiter does not occur inside the literal. For example, you can use | as the delimiter, ,, @ anything you want:
sizes = %w@small medium large@
colors = %w@red green blue@
materials = %w@cotton linen@

The first character after the w determines the delimiter. Delimiters come in two variants: paired and unpaired. With an unpaired delimiter, the same character ends the literal, as in the second example. With a paired delimiter, the corresponding closing delimiter ends the literal, e.g. when you start with <, you close with >, etc. See the first example.
Linting
You should run some sort of linter or static analyzer on your code. Rubocop is a popular one, but there are others.
Rubocop was able to detect all of the style improvements I pointed out above, and also was able to autocorrect all of the ones I listed.
I have set up my editor such that it automatically runs Rubocop with auto-fix as soon as I hit "save".
Here's what the result of the auto-fix looks like:
# frozen_string_literal: true

## SAMPLE VARIANTS
sizes = %w[small medium large]
colors = %w[red green blue]
materials = %w[cotton linen]

## ITERATE TO ALL VARIANTS
titles = []
sizes.each do |size|
  colors.each do |color|
    materials.each do |material|
      ## PUT THE VARIANT IN THE NEW ARRAY
      titles.push("#{size} - #{color} - #{material}")
    end
  end
end

puts titles.inspect

puts foo.inspect
Kernel#p is the preferred debugging method. It does the same thing, but is more idiomatic, and is specifically designed for quick debugging (hence the one-character name).
So, the last line can simply be
p titles

Also, Kernel#puts returns nil, but Kernel#p returns its argument(s), so you can quickly chuck it into a long chain of expressions without changing the result.
Vertical whitespace
Your code could use some vertical whitespace to give the code more room to breathe. I would suggest at least separating the initialization at the beginning of the loop:
titles = []

sizes.each do |size|
  colors.each do |color|
    materials.each do |material|
      ## PUT THE VARIANT IN THE NEW ARRAY
      titles.push("#{size} - #{color} - #{material}")
    end
  end
end

The "shovel" operator <<
Array#push is not idiomatic. More precisely, it is only idiomatic if you are using the array as a stack, then you would use Array#push and Array#pop, since those are the standard names for the stack operations.
The idiomatic way of appending something to something else is the shovel operator, in this case Array#<<, so that should be
titles << "#{size} - #{color} - #{material}"

Iterators
In Ruby, it is idiomatic to use high-level iterators. In your code, you are already using iterators instead of loops, so that is good. However, each is really the lowest-level of all the iterators. It is essentially equivalent to a FOREACH-OF loop. It has no higher-level semantics, and it relies on mutation and side-effects.
Whenever you have the pattern of "Initialize a result, loop over a collection appending to the result, return result", that is a fold. There are two implementations of fold in the Ruby core library, inject and each_with_object. inject is the more functional one, each_with_object is the more imperative one. So, for now, we will use each_with_object here, since the code is still pretty imperative, and that makes the relationship between the old and new code more clear.
As a general transformation,
accumulator = some_initial_value

collection.each do |element|
  accumulator = do_something_with(accumulator, element)
end

becomes
accumulator = collection.inject(some_initial_value) do |accumulator, element|
  do_something_with(accumulator, element)
end

or
collection.each_with_object(some_initial_value) do |element, accumulator|
  do_something_with(accumulator, element)
end

In your case, it would look like this:
titles = []

sizes.each do |size|
  colors.each do |color|
    materials.each do |material|
      ## PUT THE VARIANT IN THE NEW ARRAY
      titles << "#{size} - #{color} - #{material}"
    end
  end
end

becomes
titles = []

sizes.each_with_object(titles) do |size, titles|
  colors.each_with_object(titles) do |color, titles|
    materials.each_with_object(titles) do |material, titles|
      ## PUT THE VARIANT IN THE NEW ARRAY
      titles << "#{size} - #{color} - #{material}"
    end
  end
end

Granted, this doesn't buy us much, actually the opposite. It starts to look slightly different though, when we move to a purely functional version without side-effects and mutation using Enumerable#inject:
titles = sizes.inject([]) do |acc, size|
  colors.inject(acc) do |acc, color|
    materials.inject(acc) do |acc, material|
      ## PUT THE VARIANT IN THE NEW ARRAY
      acc + ["#{size} - #{color} - #{material}"]
    end
  end
end

Linter, revisited
Rubocop actually complains about my use of shadowing the outer acc with the inner acc.
I disagree. You should not be afraid to disable or re-configure rules in your linter to fit your style.
However, note that programming is a team sport. If you are modifying code, adopt the existing style. If you are part of a team, adopt the team's style. If you write open source code, adopt the project's style. If you start your own project, adopt the community's style (do not create your own style for your project, until your project is big and successful enough to have its own independent community).
Excursion: Generality of fold (inject / each_with_object)
When I wrote above that you can rewrite this iteration with inject or each_with_object, that was actually a tautological statement. I didn't even have to read the code to make this statement.
It turns out that fold is "general". Every iteration over a collection can be expressed using fold. This means, if we were to delete every method from Enumerable, except inject, then we could re-implement the entire Enumerable module again, using nothing but inject. As long as we have inject, we can do anything.
Iterators, take 2
So, what we did until now was replace the low-level iterator with a higher-level iterator.
However, we are still not done. What we are now doing is we take each three elements from our three collections, concatenating them, and putting it into a new collection. So, really, what we are doing is transforming each element (or triple of elements), or "mapping" each element to a new element.
This is called map and is also available in Ruby as Enumerable#map.
So, finally, our code looks like this:
titles = sizes.map do |size|
  colors.map do |color|
    materials.map do | material|
      "#{size} - #{color} - #{material}"
    end
  end
end

This result is actually not quite right: we get a triply-nested array, because we have a triply-nested Enumerable#map.
We could Array#flatten the result, but there is a better way: Enumerable#flat_map:
titles = sizes.flat_map do |size|
  colors.flat_map do |color|
    materials.map do | material|
      "#{size} - #{color} - #{material}"
    end
  end
end

What we did here, was to replace the general high-level iterator fold (which can do anything) with a more restricted, more specialized high-level iterator map. By using a more specialized iterator, we are able to better convey our semantics to the reader. Instead of thinking "Okay, so here we have an accumulator, and an element, and we do something with the element, and then append it to the accumulator … ah, I see, we are transforming each element", the reader just sees map and instantly knows that map transforms the elements.
Array methods
There is not much we can improve in the code by using iterators. However, there are many more methods in both the Enumerable mixin and the Array class.
So, let's take a step back and think about what we are actually doing here: we are constructing the Cartesian Product of the three arrays. And perhaps not surprisingly, there already is a method that computes a product of arrays, creatively named Array#product:
titles = sizes.product(colors, materials).map do |size, color, material|
  "#{size} - #{color} - #{material}"
end

Array#join
As a last improvement, let's look at what the block is doing: it is "joining" the three variants together. And again, there is already a method which does that: Array#join:
titles = sizes.product(colors, materials).map do |variant|
  variant.join(' - ')
end

Final result
So, in the end, the entire thing looks something like this:
# frozen_string_literal: true

## SAMPLE VARIANTS
sizes = %w[small medium large]
colors = %w[red green blue]
materials = %w[cotton linen]

titles = sizes.product(colors, materials).map do |variant|
  variant.join(' - ')
end

p titles

Which I think is some nice-looking, easy to read, easy to understand code.


Convert string to double and check for errors The following program converts a string to a double, and checks for errors.  It uses the strtod() function to do the conversion, and follows the example given for the strtol() function from the man page to do the error checking.

As an example, the program is executed as follows:

$ ./a.out 123.45

I would really appreciate any comments regarding the code, specifically whether there are any issues to be address or whether the code can be made more efficient or improved in any way.

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

int main(int argc, char *argv[])
{
    char *endptr, *str;
    double val;

    if (argc < 2) {
        fprintf(stderr, "Usage: %s str\n", argv[0]);
        exit(EXIT_FAILURE);
    }

    str = argv[1];

    errno = 0;    /* To distinguish success/failure after call */
    val = strtod(str, &endptr);

    /* Check for various possible errors */

    if ((errno == ERANGE && (val == DBL_MAX || val == -DBL_MAX))
            || (errno != 0 && val == 0.0)) {
        perror("strtod");
        exit(EXIT_FAILURE);
    }

    if (endptr == str) {
        fprintf(stderr, "No digits were found\n");
        exit(EXIT_FAILURE);
    }

    /* If we got here, strtod() successfully parsed a number */

    printf("strtod() returned %f\n", val);

    if (*endptr != '\0')        /* Not necessarily an error... */
        printf("Further characters after number: %s\n", endptr);

    exit(EXIT_SUCCESS);
}

Good use of errno, but can be improved.

The below is decent code for detecting overflow, yet would benefit with improvements.

On overflow, the return value is HUGE_VAL, not necessarily DBL_MAX.  HUGE_VAL may be a large finite or an infinity.

if ((errno == ERANGE && (val == DBL_MAX || val == -DBL_MAX)) || ...) {
    perror("strtod");
    exit(EXIT_FAILURE);
}
// replace with
if ((errno == ERANGE && (val == HUGE_VAL || val == -HUGE_VAL)) || ...) {

Underflow may also raise ERANGE.  Look at the code that attempts to discern that and review the spec.

if (...  || (errno != 0 && val == 0.0)) {
    perror("strtod");
    exit(EXIT_FAILURE);
}

  If the result underflows ..., the functions return a value whose magnitude is no greater than the smallest normalized positive number in the return type; whether errno acquires the value ERANGE is implementation-defined.

OP's code incorrectly assumes 0.0 on underflow.

With so  many options with 1) if errno is set and 2) what value is return, portable code nneds to fix a mess.

Portable code would use the following, yet that gives up useful conversions by some of values in the sub-normal range DBL_TRUE_MIN to DBL_MIN.

if (fabs(val) < DBL_MIN) {
  if (val > 0.0) val = DBL_MIN;
  else if (val < 0.0) val = -DBL_MIN;
  // val is now, DBL_MIN, -DBL_MIN, 0.0, or -0.0

  // pedantic code would also reset errno = 0 if it changed due to `strtod()`
}

A somewhat less portable handing, yet retains sub-normal values - just accept any wee value.

if (fabs(val) < DBL_MIN) {
  // reset errno = 0 if it changed due to `strtod()`
}

No conversion

Good test for no conversion, yet some strtod() set errno in the no conversion case.  To simplify errno processing in the previous block, I'd recommend to do the endptr == str before looking at errno.

Use exponential notation for output

"%f" may print all values of magnitude less than 0.0000005 as 0.000000.  This is not informative.  With large values like DBL_MAX, perhaps hundreds of digits are printed of which past the first 20 or so are rarely of interest.

Instead:

#include <float.h>

printf("strtod() returned %.*g\n", DBL_DECIMAL_DIG, DBL_val);
// or
printf("strtod() returned %.*e\n", DBL_DECIMAL_DIG - 1, val);

Fancier code would assess the length of the argv[1] string for exponential notation and number of digits and then print out in a like-wise fashion.

Tolerate trailing white-space

OP code's alerts with input like "123  ", but not with "  123".  Suggest silence on trailing white-space.

// add
while (issapce((unsigned char) *endptr)) {
  endptr++;
}

if (*endptr != '\0')        /* Not necessarily an error... */
    printf("Further characters after number: %s\n", endptr);

Such non-numeric output may be best done on stderr rather than stdout.

Suggested alternative.

Useful as a basis for some ideas.

// Return status for my_strtod().
// Higher values are more problematic.
typedef enum {
  my_strtod_OK,
  my_strtod_Underflow,
  my_strtod_Overflow,
  my_strtod_ExtraJunk,
  my_strtod_NoConvertableText,
  my_strtod_N
} my_strtod_T;

// Convert a pointer to a string to double and saves its value in *dest.
// Return conversion status.
//
// `errno` is temporarily cleared by this function.  Its value on return:
//   1) Should strtod(*s, ...) set errno, then that is its value.
//   2) Otherwise errno is restored to its original value.
my_strtod_T my_strtod(double *dest, const char *s) {
  char *endptr;

  int errno_original = errno;
  errno = 0;
  *dest = strtod(s, &endptr);
  int errno_my_strtod = errno;
  if (errno == 0) {
    errno = errno_original;
  }

  if (s == endptr) {
    return my_strtod_NoConvertableText;
  }

  while (isspace((unsigned char ) *endptr)) {
    endptr++;
  }
  if (*endptr) {
    return my_strtod_ExtraJunk;
  }

  if (errno_my_strtod == ERANGE && fabs(*dest) == HUGE_VAL) {
    return my_strtod_Overflow;
  }

  if (errno_my_strtod == ERANGE && fabs(*dest) <= DBL_MIN) {
    return my_strtod_Underflow;
  }

  // Note: at this point, errno may be set
  return my_strtod_OK;
}

Selected C specs concerning errno:

/*
 * errno ...  has type int ... the value of which is set to a positive error
 * number by several library functions.  C11dr §7.5 2
 *
 * errno ... is never set to zero by any library function. The value of errno
 * may be set to nonzero by a library function call whether or not there is an
 * error, provided the use of errno is not documented in the description of
 * the function ... C11dr §7.5 3
 *
 * Of course, a library function can save the value of errno on entry and
 * then set it to zero, as long as the original value is restored if errno’s
 * value is still zero just before the return.  C11dr footnote 202
 */


Removing and counting words that appear more than once The assignment is to remove the repeating string from a given string and get the count of repeated string.

The code I have tried is below :

public static void main(String[] args) {

        String longString = "Energy in the form of seismic waves is released " +
                            "after an earthquake and seismometers measure its amplitude to calculate the " +
                            "quake's intensity. For a long time, earthquakes were measured on the Richter scale." +
                            " Since the scale was based on conditions in California and was not reliable in measuring large earthquakes," +
                            " it was replaced by a 'moment magnitude' scale in the 1970s";

        // replace the special characters
        String freeString = longString.replaceAll("[^\\w\\s-]", "");
        //the new string is
        System.out.println("freeString ::  " + freeString);
        //Calculate the length of the string
        String[] countOfString = freeString.split(" ");

        System.out.println("countOfString  ::  " + countOfString.length);

        Set<String> uniqueString = new HashSet<String>();
        Map<String, Integer> countMap = new HashMap<String, Integer>();
        for (int i = 0; i < countOfString.length; i++) {
            String tempString = countOfString[i];
            if(!uniqueString.contains(tempString)){
                uniqueString.add(tempString);
            }else{
                if(!countMap.containsKey(tempString)){
                    countMap.put(tempString, 1);
                }else{
                    Integer count = countMap.get(tempString);
                    count++;
                    countMap.put(tempString, count);
                }

                System.out.println("found the repeating string " + tempString);
            }
        }

        //remove the repeating string set from the uniqueString set
        uniqueString.removeAll(countMap.keySet());
        System.out.println("the repeating string " + countMap);
        System.out.println("the set has the uniuqe strings " + uniqueString);
        System.out.println("the set has the uniuqe strings of length " + uniqueString.size());

    }

Is there any best way/approach other than this?
Will this code work for very large string (e.g. parsing the text of a book)
Will it have any performance issue?

Your algorithm seems fine. It doesn't reveal any special tricks to me, but I also don't see any unnecessary steps. I think I'd have written it myself that way. The only part that I might have done differently is the regex; I'm not sure that one is fully correct.

So on an algorithmic level you're doing perfectly fine. However, the implementation suffers from spelling issues.

Comments

First, you have some comments.

    // replace the special characters
    String freeString = longString.replaceAll("[^\\w\\s-]", "");
    //the new string is
    System.out.println("freeString ::  " + freeString);
    //Calculate the length of the string
    String[] countOfString = freeString.split(" ");

Comments are nice, they help understand what's going on... but so does the code. Of course replaceAll replaces things, we already know that the new string is the freeString, and that String.split calculates the length of the string is actually not what it does.

You should replace your comments with comments with non-obvious things about what the code does (What does your regex actually do?), or comments about why the code does.

Variable Naming

Variable naming should be based on what the variable stores - usually its "semantic type". A "syntactic type" would be a String, Integer and so on. The "semantic type" is what it means - "inputString", "validatedString", "splittedString", "stringsToLoopThrough"... some of these are better than others. A semantic type doesn't have to include the syntactic type - you can have variable names like "index", "key", "value", without containing any notion of syntactic type. Usually the syntactic type is inferred, and at other times, it doesn't matter what the type is, just that it's the index/key/value/whatever.

Based on that, I think you should name countOfString words, uniqueString uniqueWords, countMap wordCountMap, and countString cleanedLongString.

countOfString seems to indicate a number. But it's a string array instead. A string array containing the words of the longString. So words seems like a fitting name. It certainly seems logical that if you split a longString on a space, you will get words.

uniqueString to uniqueWords is because of two reasons. First, by changing to Words, we signal that this variable is related to the words variable. Second, by pluralizing the variable name from String to Strings (or Word -> Words), we say this is a "collection" of sorts. Something which holds multiple values.

countMap to wordCountMap is to signal the relationship with words.

countString to cleanedLongString has two reasons, first to again note a relationship with another variable, this time with longString. Second, I add the cleaned because you have cleaned the special characters out of the original string.

The names aren't ideal yet - longString seems like a bad name, considering it doesn't have to be a longString, and it doesn't signal that it needs to be cleaned first.


Filtering undefined elements out of an array I have some code that filters an array with type (Suggestion|undefined)[] into an array with type Suggestion[], but I feel it does so very inelegantly:

const optionsProvider: AutocompletionProvider = () => {
    const results: (Suggestion|undefined)[] = flagDescriptions.map(descriptions => {
        let shortFlagWithArgument = descriptions[0].match(/^ *-(\w) (\w*)$/);
        let shortFlagWithoutArgument = descriptions[0].match(/^ *-(\w) *(.*)$/);
        if (shortFlagWithArgument) {
            const flag = shortFlagWithArgument[1];
            const argument = shortFlagWithArgument[2];
            const description = combineManPageLines(descriptions.slice(1));

            return new Suggestion({
                value: `-${flag}`,
                style: styles.option,
                description,
                displayValue: `-${flag} ${argument}`,
            });
        } else if (shortFlagWithoutArgument) {
            const flag = shortFlagWithoutArgument[1];
            const description = combineManPageLines([shortFlagWithoutArgument[2], ...descriptions.slice(1)]);

            return new Suggestion({
                value: `-${flag}`,
                style: styles.option,
                description,
            });
        }
    });
    const filteredResults: Suggestion[] = [];
    results.forEach(result => {
        if (result) {
            filteredResults.push(result);
        }
    });
    return filteredResults;
};

In straight JS, this would just be array.filter(element => element !== undefined) but the best working solution I could come up with in TypeScript was to push the defined elements into a separate array within a forEach.

For context:

type AutocompletionProvider = (context: AutocompletionContext) => Suggestion[] | Promise<Suggestion[]>;

Original PR: https://github.com/shockone/black-screen/pull/623/files

To be using strict null checks, you must be on "typescript": "^2.0.0". Thus, my example will be using this version of typescript with "compilerOptions":{"strictNullChecks": true}.

It does seem that the filter solution is the most elegant solution to the titular question. Sadly, it seems that the typescript static analysis is unable to track this behavior:

const source: (string|undefined)[] = ["one", "two", "three", undefined, "five"]
const filtered: string[] = source.filter(element => element !== undefined)
//  [ts]
//    Type '(string | undefined)[]' is not assignable to type 'string[]'.
//      Type 'string | undefined' is not assignable to type 'string'.
//        Type 'undefined' is not assignable to type 'string'.

This seems to be the case because of the declaration of the filter method:

/** Returns the elements of an array that meet the condition specified in a callback function. */
(method) Array<string | undefined>.filter(callbackfn: (value: string | undefined, index: number, array: (string | undefined)[]) => any, thisArg?: any): (string | undefined)[]

In this case, our own type logic beats out the compiler's. It's these cases that is the reason that Type Assertion is part of the language spec. Thus:

const source: (string|undefined)[] = ["one", "two", "three", undefined, "five"]
const filtered: string[] = source.filter(element => element !== undefined) as string[]

This transpiles exactly as you would expect -- in this case, transpiling is as simple as just removing the type information. If you need to convince the language to convert two unrelated types, you need to step through any to tell the compiler to back off: object as any as string.

Now, looking at the rest of the code:

shortFlagWithArgument and shortFlagWithoutArgument can be consts, as you do not mutate their values.

Personally, I would add an explicit final return to the flagDescriptions.map operation. This makes explicit why the undefined needs to be in the results array in the first place.

The regex patterns seem good candidates for refactoring into a reusable constant. Depending on your project ecosystem, however, it may be better just to leave them inlined.


Best method to further execute the DRY principle and/or raise performance in my script? (client login/signup system) Just for practice purposes, I written a basic client sided login/signup system that allows you to make an account, and then log into the account from my webpage. What it does is append to an object known as database for every new account made, and when you sign in, it loops through the database and see if your credentials match any account information.

There's a couple of noticable problems with my script. One being that I believe that I didn't follow the DRY principle to the fullest. I felt like I repeated myself a couple of times, and wondering if that can be avoided. The second problem is that looping through the "database" object may not be the most efficient solution, especially since the longer the table, the longer it may take. There may be a solution that is more efficient in performance that I am unaware of.

Other than that I would just like general feedback, tips, and red flags (if any) in my code. I am trying to improve myself as a JavaScript developer, and that requires feedback of my script from my peers.

TL;DR: I am a new programmer, so sorry if this is a bad question or if I have terrible code. I am still learning.

const database = []

function usp(parent) {
  return [parent.username.value, parent.password.value]
}

window.onload = () => {
  const create = document.forms.createAcount;
  const log = document.forms.logIN;

  create.addEventListener('click', (e) => {
    if(e.target == create.submit){
      const information = usp(create);
      database.push({user : information[0], pass : information[1]});
      console.log(database)
    }
  });

  log.addEventListener('click', (e) => {
    if (e.target == log.submit) {
      const information = usp(log);
      for (let x of database) {
        if (x.user == information[0] && x.pass == information[1]) {
          alert(`You successfully logged into your account! Welcome to my website ${information[0]}`)
        }
      }
    }
  })
}
#createAcount {
  background-color: skyblue;
}
#logIN {
  background-color: lightgreen;
}
<!DOCTYPE html>

      <title>SignUp!</title>
      <link rel = "stylesheet" href = 'style.css'>

Sign Up

      <form name = "createAcount" id = "createAcount">

Create a Username

        <input type = 'text' placeholder="Falkyraizu"  name = 'username' maxlength=20>

Create a password

        <input type = 'password' placeholder="password" name = 'password' maxlength=20>

Done

        <input class = 'reset' type = 'button' value = "Reset" name = 'reset'>
        <input class = 'submit' type = 'button' value = "Submit" name = 'submit'>
      </form>

Log In

      <form name = "logIN" id = "logIN">

What is your userName

        <input type = 'text' placeholder="Falkyraizu" name = "username" maxlength=20>

What is your password

        <input type = 'password' placeholder="password" name = "password" maxlength=20>

Done

        <input class = 'reset' type = 'button' value = "Reset" name = 'reset'>
        <input class = 'submit' type = 'button' value = "Submit" name = 'submit'>
      </form>

Side Note: If you notice something else that can be fixed such as my HTML, you can also include it in your response, however I would still like my main problems fixed (read above if you missed it). Thanks for the help!

This code appears to be fairly clean and reduced well to me, I don't see much in the way of any DRY violations, but if you're looking for any places for tightening up your code:

If you can use the newer javascript goodies, then use array destructuring assignment and object shorthand assignment

Given

function usp(parent) {
  return [parent.username.value, parent.password.value];
}

Then

const [user, pass] = usp(create);
database.push({ user, pass });

and use '==' with caution as it isn't strictly equal, it will attempt a type coercion, i.e. '5' == 5 is true, where as '===' won't, i.e. '5' === 5 is false. As a rule, you will almost always want to use '===', only reach for '==' when necessary.

Comparison operators

const [user, pass] = usp(log);
for (const x of database) {
  if (x.user === user && x.pass === pass) {
    ...
  }
}

Looping through any data structure has an O(n) complexity, there's nothing really you can do here, but this is completely fine as any increases in data size is a linear increase in processing.


Updating necessary data using just one function My objective is to update necessary data using one function instead of having different functions to update different fields. So, I've created one and I think this is not really elegant, efficient or secure. I would seriously like a review if possible.

Key.

    $data is an array.
    $data[0] = column to update.
    $data[1] = new data.
    $data[2] = username of the user.

Code:

public function handleUserDataUpdate($data) {
    if($userMapper->validate($data) === true) {
        $userMapper->update($data);
    } else {
        $errors['count'] = count($errors);
        return $errors;
    }
}

UserMapper Class Methods:

public function validate($data) {
    switch ($data[0]) {
        case 'rank':
            if(empty($data[1])) {
                $errors[] = "The rank field cannot be empty!";
            }
            if(count($errors) > 0) {
                return $errors;
            } else {
                return true;
            }
            break;
        case 'display_name':
            if(empty($data[1]) || strlen($data[1]) < 3 || strlen($data[1] > 20)) {
                $errors[] = "The display name should contain at least 3 to 20 characters";
            }
            if(count($errors) > 0) {
                return $errors;
            } else {
                return true;
            }
            break;
    }
}
public function update($data) {
    $sql = "UPDATE users SET " . $data[0] . "=? WHERE username=?";
    $query = $this->db->prepare($this->sql);
    $query->bind_param('ss', $data[1], $data[2]);
    $query->execute();
    $query->close();
}

You are vulnerable to SQL Injections. Even when you are using Prepared statements, doing It wrong will kill you.

$sql = "UPDATE users SET " . $data[0] . "=? WHERE username=?";

This makes you exposed to SQL injections because your function is not private. You should not allow "user" access to "critical" functions. make that function private and you should be good to go.

As soon as the user is allowed to set $data[0] you're screwed.


Enhanced concatenating function I would like to submit my enhanced concatenating function for your review. Originally, its method signature is:

char *concat (const *char1, const *char2);

but after knowing that the calling code must supply all the arguments when it calls the function, I modified the method signature to:

char *enhconcat (const char *str1, int start_index1, int end_index1, const char *str2, int start_index2, int end_index2);

supplying the necessary positional arguments, so I think the code is more secured and adaptable to future changes.

Please suggest ways to improve this code and make it more secure.

/* enhconcat.c: enhanced concat - for more secured concatenating */

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

char *
enhconcat (const char *str1, int start_index1, int end_index1,
           const char *str2, int start_index2, int end_index2);

int main (void) {

     const char *part1 = "Cuius est solum, ";
     const char *part2 = "eius est usque ad coelum et ad inferos.";
     printf ("%s\n", enhconcat (part1, 0, strlen (part1), part2, 0, strlen (part2)));
     return (EXIT_SUCCESS);
}

char *
enhconcat (const char *str1, int start_index1, int end_index1,
           const char *str2, int start_index2, int end_index2)
{
   int m = end_index1 - start_index1;
   int n = end_index2 - start_index2;
   char *concatstr;
   concatstr = malloc (m + n + 1);
   if (!concatstr) {
      printf ("Unable to allocate memory.\n");
      exit (EXIT_FAILURE);
   }
   char *chrptr;
   chrptr = concatstr; // store address of concatstr
   for (int i = start_index1; i < m; i++) {
      *chrptr = str1[i];
      chrptr++;
   }
   for (int j = start_index2; j < n; j++) {
      *chrptr = str2[j];
      chrptr++;
   }
   *chrptr = '\0';
   return concatstr;
} /* end of enhconcat() */

Chris, you have misunderstood the text quoted about the caller supplying all of the arguments. Your original prototype was okay. I imagine the text you read meant that if a function declares parameters a, b and c, then the caller must supply arguments (ie values of some sort) for all of those parameters. If there are 3 it must supply 3,, and of the correct types.

Given the prototype you imposed upon yourself, your implementation was not bad, but you should learn to use the standard library functions.  Your loops for copying characters are better implemented using memcpy.

Also when you declare a variable, if you can it is often better to assign a value to it immediately, not on the next line. In C, this doesn't really matter, but in C++ doing this (assigning, or constructing immediately) is more efficient.

Here is my version of your function with its original prototype.  Note that instead of exiting on malloc failure, I print a message and return NULL. The caller must handle the failure.  Note also that allocated memory must be freed.  In a simple program like this, it is not important, as it will be freed on exit from the program. But remember that you have to do it normally.

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

static char *
enhconcat (const char *str1, const char *str2)
{
    size_t len1 = strlen(str1);
    size_t len2 = strlen(str2);

    char *concatstr = malloc(len1 + len2 + 1);
    if (!concatstr) {
        perror("malloc");
        return NULL;
    }
    memcpy(concatstr, str1, len1);
    memcpy(concatstr + len1, str2, len2);
    concatstr[len1+len2] = '\0';
    return concatstr;
}

int main (void)
{
     const char *part1 = "Cuius est solum, ";
     const char *part2 = "eius est usque ad coelum et ad inferos.";

     char *cat = enhconcat(part1, part2);
     if (cat != NULL) {
         printf ("%s\n", cat);
         free(cat);
     }
     return EXIT_SUCCESS;
}


Challenge: solution for this deep-iteration JavaScript function dilemma? Trying to implement a deep-iteration function with some constraints, I've noticed the faster solution is iterating it twice! Any attempt to avoid this resulted in slower code. So, the challenge: can you redesign the function below removing the code duplicate and doubled iteration, without making it slower?

Constraints:

It must deeply iterate through a tree, providing, for a callback function, the node value and it's path.
It must work for arrays and objects.
Arrays must be iterated by ascending numerical keys first and non-numerical keys last.
It must allow for early interruption by using "return", and must return that value.

Code:

function iter(obj,fn,iter_pos){
    var ret, iter_pos = iter_pos || [];
    for (var i=0; i<obj.length; ++i)
        if (ret = typeof(obj[i]) == "object" ? iter(obj[i],fn,iter_pos.concat(i)) : fn(iter_pos.concat(i),obj[i]))
            return ret;
    for (var key in obj)
        if (isNaN(Number(key)))
            if (ret = typeof(obj[key]) == "object" ? iter(obj[key],fn,iter_pos.concat(key)) : fn(iter_pos.concat(key),obj[key]))
                return ret;
};

Test:

var tree = [1,2,[3,3,3],4];
tree.x = 5;
tree.y = {x:6,y:7,z:8};
tree.z = 9;
console.log(
    iter(tree,function(path,val){
        console.log("Value ",val," at path: ",path);
        if (val==7)
            return "Returned early!";
    })
);

Output:

Value  1  at path:  [ 0 ]
Value  2  at path:  [ 1 ]
Value  3  at path:  [ 2, 0 ]
Value  3  at path:  [ 2, 1 ]
Value  3  at path:  [ 2, 2 ]
Value  4  at path:  [ 3 ]
Value  5  at path:  [ 'x' ]
Value  6  at path:  [ 'y', 'x' ]
Value  7  at path:  [ 'y', 'y' ]
Returned early!

Let me start by asking how would you like to iterate over your object? Because what you are doing is not the same as calling a function for every property of an object.

function iter(obj,fn,iter_pos){
    var ret, iter_pos = iter_pos || [];
    for (var i=0; i<obj.length; ++i)

The above for loop may access elements in the array that are not even defined. For example, take this array:

var a = [0, 1];
a[9] = 9;
console.log(a.length);  // -> 10
console.log(a.hasOwnProperty(0)); // -> true
console.log(a.hasOwnProperty(1)); // -> true
console.log(a.hasOwnProperty(2)); // -> false !!
console.log(a.hasOwnProperty(9)); // -> true

The above array has a length of 10, but it only has three properties (it has 10 elements, but not all are defined). If you do an a.forEach(), you'll iterate over the array's properties, only those that are defined.

If you do a for (i=0; i<a.length; i++) loop over the array, you'll get properties that are not defined. To avoid those, you should check a.hasOwnProperty(). Or just use the native forEach, if you can, that will iterate over the array correctly.

Note that checking against undefined would not be sufficient either, array elements can be present but undefined:

a[5] = undefined;
console.log(a.hasOwnProperty(5)); // -> true !!

        if (ret = typeof(obj[i]) == "object" ? iter(obj[i],fn,iter_pos.concat(i)) : fn(iter_pos.concat(i),obj[i]))
            return ret;
    for (var key in obj)
        if (isNaN(Number(key)))

Here you're trying to do the opposite of forEach: enumerating properties that are not elements of the array.

But this code will miss properties that still evaluate as numbers! Have a look at the following array:

var a = [0, 1, 2, 3, 4];
a[100] = 99;
a[1.5] = 'this is not an array element';
a[-1] = 'this one is not an array element either';
a['20'] = 'this one *is* an array element!';

Also, watch out for rounding:

a[30.00000000000001] = 'this is *not* an array element';
a[30.000000000000001] = 'this *is*, however, because it rounds to 30'

But also for the string representations:

a[30.000000000000001] = 'array element';
a['30.000000000000001'] = 'not array element';

Especially the ones that evaluate the same:

a[1e1] = 'element number ten';
a['1e1'] = 'a completely different element';

'-1', '1.5', '1e1', '30.000000000000001', etc. can all be properties of an array, but they will all evaluate false for isNaN(Number(key)).

            if (ret = typeof(obj[key]) == "object" ? iter(obj[key],fn,iter_pos.concat(key)) : fn(iter_pos.concat(key),obj[key]))
                return ret;
};

You can easily fix your script by changing the line

        if (isNaN(Number(key)))

to

        if (!key.match(/^\d+$/))

, but unfortunately that makes your code about 18% slower.

TL; DR

I've removed the double loop from your code:

function iter(obj, fn, iter_pos){
    var ret, iter_pos = iter_pos || [];
    for (var key in obj)
        if (ret = typeof(obj[key]) == "object" ? iter(obj[key], fn, iter_pos.concat(key)) : fn(iter_pos.concat(key), obj[key]))
            return ret;
};

and it does not seem to be any slower, or at least not according to jsperf.com/iter.

I tried it in a number of browsers I could find.

UPDATE 1:

Here is a version that uses two loops, but is much faster. It will fix the ordering, but only in cases where the array indexes come before other array properties.

__toString = Object.prototype.toString;

function iter(obj, fn, iter_pos) {
    var ret, i = 0,
        iter_pos = iter_pos || [];
    if (__toString.call(obj) === '[object Array]') {
        obj.forEach(function (item) {
            if (!ret) {
                ret = typeof (obj[i]) === "object" ? iter(obj[i], fn, iter_pos.concat(i)) : fn(iter_pos.concat(i), obj[i]);
                i++;
            }
        });
        if (ret)
            return ret;
    }
    for (var key in obj)
        if (--i < 0)
            if (ret = typeof (obj[key]) === "object" ? iter(obj[key], fn, iter_pos.concat(key)) : fn(iter_pos.concat(key), obj[key])) return ret;
};

UPDATE 2:

This is your original version, with two loops, but using forEach and Object.prototype.toString. It seems to output the same result, but is about 2000× faster.

__toString = Object.prototype.toString;

function iter(obj, fn, iter_pos) {
    var ret, iter_pos = iter_pos || [];
    if (__toString.call(obj) === '[object Array]') {
        obj.forEach(function (item) {
            if (!ret)
                ret = typeof (obj[i]) === "object" ? iter(obj[i], fn, iter_pos.concat(i)) : fn(iter_pos.concat(i), obj[i]);
        });
        if (ret)
            return ret;
    }
    for (var key in obj)
        if (!("" + key).match(/^\d+$/))
            if (ret = typeof (obj[key]) === "object" ? iter(obj[key], fn, iter_pos.concat(key)) : fn(iter_pos.concat(key), obj[key]))
                return ret;
};

UPDATE 3:

This version is about as fast as the other one, but uses only Object.prototype.toString and does not use typeof.

__toString = Object.prototype.toString;

function iter(obj, fn, iter_pos) {
    var ret, obj_type, iter_pos = iter_pos || [];
    if (__toString.call(obj) === '[object Array]') {
        obj.forEach(function (item) {
            if (!ret)
                ret = ((obj_type = __toString.call(item)) === "[object Object]") || (obj_type === "[object Array]") ? iter(item, fn, iter_pos.concat(i)) : fn(iter_pos.concat(i), item);
        });
        if (ret)
            return ret;
    }
    for (var key in obj)
        if (!("" + key).match(/^\d+$/))
            if (ret = ((obj_type = __toString.call(item)) === "[object Object]") || (obj_type === "[object Array]") ? iter(obj[key], fn, iter_pos.concat(i)) : fn(iter_pos.concat(i), obj[key]));
                return ret
};


Coding style and best practice regarding exception-handling I need comments on my JPA code. Am I doing it correctly? I need some advice on best practices in Java.

I have this method below:

public Comics add(Comics comics) {
    EntityManager em = EMF.get().createEntityManager();
    EntityTransaction tx = null;
    try{
        tx = em.getTransaction();
        tx.begin();

        em.persist(comics);

        tx.commit();
    }catch(Exception ex){
        ex.printStackTrace();
        if(tx != null && tx.isActive())
            tx.rollback();
    } finally{
        em.close();
    }

    return comics;
}

When I called the method, I just have:

ComicsAccess access = new ComicsAccess();
access.add( comics );

I got no error on the above code, what I want are some suggestions on my coding style. Is the above good code if used in production?

The way you are handling the exception will result in the method returning normally whether or not the method has succeeded.  This is bad API design.  Better design would be to either return some special value to indicate failure or (better) to allow the exception to propagate, or throw a different one.

Catching Exception is usually a bad idea.  The problem is that you may end up catching all sorts of unexpected exceptions.

Unconditionally writing stacktraces to standard output is bad practice.  Use a logging system; e.g. java.util.logging, log4j, etcetera.

Here is an alternative version that avoids these problems:

public Comics add(Comics comics) throws ... {
    EntityManager em = EMF.get().createEntityManager();
    EntityTransaction tx = null;
    try {
        tx = em.getTransaction();
        tx.begin();
        em.persist(comics);
        tx.commit();
    } finally {
        try {
            if (tx != null && tx.isActive()) {
                tx.rollback();
            }
        } finally {
            em.close();
        }
    }
    return comics;
}


ASP.NET HTML code generation, two approaches I was asked to give more context. So here we go.

We want to create some HTML in code behind to add an "add" and a "refresh" button to a grid view header column. Approach #1 does this using string concatination to produce the necessary HTML, whereas approach #2 uses .NET objects to create exactly the same HTML.

This time I'm posting the complete classes:

Approach #1

class AddBtnTemplate : ITemplate {
    StatRefTimeGrid Parent;
    public AddBtnTemplate(StatRefTimeGrid parent) {
        Parent = parent;
    }
    public void InstantiateIn(Control container) {
        GridViewHeaderTemplateContainer ctr = (GridViewHeaderTemplateContainer)container;
        string html = "
";
        if (Parent.MayEdit)
            html += string.Format("", Texts.CompValue.AddRefTime[Parent.Parent.State.LangIdx]);
        if (ctr.Grid.VisibleRowCount > 1)
            html += string.Format("", Texts.CompValue.RefreshRefTimeGrid[Parent.Parent.State.LangIdx]);
        html += "
";
        ctr.Controls.Add(new LiteralControl(html));
    }
}

Approach #2

private class AddBtnTemplate : ITemplate {
    private readonly StatRefTimeGrid mParent;
    public AddBtnTemplate(StatRefTimeGrid parent) {
        mParent = parent;
    }
    public void InstantiateIn(Control container) {
        GridViewHeaderTemplateContainer templateContainer = (GridViewHeaderTemplateContainer)container;

        HtmlGenericControl div = new HtmlGenericControl("div") {
            Style = {[HtmlTextWriterStyle.Width] = "150px"},
        };
        if (mParent.MayEdit) {
            AddNewImage(div, Texts.CompValue.AddRefTime[mParent.Parent.State.LangIdx], "~/Img/add18d.png", "StatRefTimeGrid.addRow()");
        }
        if (templateContainer.Grid.VisibleRowCount > 1) {
            AddNewImage(div, Texts.CompValue.RefreshRefTimeGrid[mParent.Parent.State.LangIdx], "~/Img/refresh18d.png", "StatRefTimeGrid.refresh()");
        }

        templateContainer.Controls.Add(div);
    }
    private void AddNewImage(HtmlGenericControl div, string altText, string imageUrl, string onClick) {
        div.Controls.Add(new Image {
            AlternateText = altText,
            ToolTip = altText,
            ImageUrl = imageUrl,
            Attributes = {
                ["onclick"] = onClick,
            },
            Style = {
                [HtmlTextWriterStyle.Cursor] = "pointer",
                [HtmlTextWriterStyle.Position] = "relative",
                [HtmlTextWriterStyle.Top] = "3px",
            },
        });
    }
}

I'm looking for arguments for or against either solution. What do you think?

Second approach is better, according to the rule: do not repeat the code!

Don't use '+=' to join string, in your case better to use StringBuilder:

var sb = new StringBuilder();
sb.Append();
...
sb.Appden();
...
return sb.ToString();

I think this is better way, StringBuilder is made for this.
You can refactor this code better little more :-)


Copy first n lines of each file in a directory In an attempt to learn Haskell, I've built this - it accepts a number n, a source directory, and a sink directory  (ending with /) copies n lines from each file in the source directory to the sink directory.  It is not recursive. Thoughts?

module Main where
import System.Directory
import Control.Monad
import System.IO
import System.IO.Error
import Data.List
import Control.Applicative
import Control.Exception.Base
import System.Environment
import Data.List

--requires / or \ at end.
listFilesInDir :: FilePath -> IO [FilePath]
listFilesInDir dir= (getDirectoryContents dir) >>= filterM (\x ->doesFileExist $ dir ++ x)

readAndWrite:: Handle -> Handle -> IO ()
readAndWrite inHandle outHandle = do
    myLn<-tryIOError(hGetLine inHandle)
    case myLn of
         Left e -> if isEOFError e
                   then return ()
                   else ioError e
         Right myLn -> do hPutStrLn outHandle myLn
                          return ()

writeFirstNLines :: Int-> FilePath->FilePath -> IO ()
writeFirstNLines n source sink =do
    inHandle <- openFile source ReadMode
    outHandle <- openFile sink WriteMode
    replicateM_ n (readAndWrite inHandle outHandle)

main :: IO ()
main = do
    args <- getArgs
    let n = read $ args!!0
    let source = args!!1
    let sink   = args!!2
    let fileList=listFilesInDir  source
    fileSources <-fmap (map (source ++)) $ fileList
    fileSinks <-fmap (map (sink ++)) $ fileList
    --putStrLn "Sources"
    --mapM_ putStrLn fileSources
    --putStrLn "Sinks"
    --mapM_ putStrLn fileSinks
    q<-zipWithM (writeFirstNLines n) fileSources fileSinks
    return()

requires / or \ at end.

Your code requires that the user supplied source and destination
directories end in a path name separator. A better approach is
to simply add it. See the comments below on the (</>) operator.

partial functions

When processing the command line arguments you
are using functions which could throw errors, e.g. read, head, !!.
When they fail you'll get a generic error message like Prelude.read: no parse or Prelude.!!: index too large.

Hackage has libraries you can use to parse and validate command line arguments
like cmdargs or any of the optparse- packages. However, it's not too difficult to roll your own like this:

 import Data.Char

 parseArgs (nstr : source : dest : _) =
   if all isDigit nstr
     then ( read nstr, source, dest )
     else error "first argument is not a number"
 parseArgs _ = error "not enough arguments"

main = do
  args <- getArgs
  let (n, source, dest) = parseArgs args
  ...

parseArgs checks that there are at least three command line arguments and
also that the first one consists entirely of digits.

(++)

Currently the FilePath type is a String, so using (++) to join
together path name segments is fine. However, in System.FilePath
Haskell defines the operator (</>) which is meant to be a cross-platform
way to do this - i.e. on Windows it will use \\ and on Posix systems it
will use a /. Moreover using (</>) signals to the reader that you are
working with a path name string.

The module System.FilePath also has utility functions for dealing with
path names such as extracting the base / directory name from a path, extracting and replacing the suffix, etc. which are good to know about and may come
in handy.

A good place to use (</>) is in your listFilesInDir function, e.g.:

listFilesInDir dir =
  getDirectoryContents >>= filterM (\x -> doesFileExist (dir </> x))

Now dir does not have to end in a / or \\ because using (</>) will
add the appropriate separator.

fileList

Note that fileList is an IO-action, so in these lines:

fileSources <-fmap (map (source ++)) $ fileList
fileSinks <-fmap (map (sink ++)) $ fileList

you are repeating the IO-action twice. That is, you are performing
getDirectoryContents twice on the same directory. Instead you
want to perform fileList
only once and use its result to compute fileSources and fileSinks:

leafNames <- listFilesInDir source
let fileSources = map (source </>) leafNames
    fileSinks = map (sink </>) leafNames
zipWithM_ (writeFirstNLines n) fileSources fileSinks

Note I'm using zipWithM_ here - it works just like zipWithM but discards
the results. It's more efficient because zipWithM_ discards the result of
the zip operation immediately, whereas zipWithM saves the results in a
list thus consuming memory for data you'll never look at.

hClose

In Haskell you need to explicitly close your file handles when you are done
with them. Every process has limit on the number of handles which can be open at any one time, so if you never close any handles then eventually openFile will fail when you hit this limit.

A basic way to make sure your file handles always get closed is to use
the withFile function:

withFile :: FilePath -> IOMode -> (Handle -> IO r) -> IO r

withFile creates a file handle by opening a path with the specified IOMode
and passes the handle to an IO-action. It will ensure that the handle is
closed when the IO-action completes - whether it returns normally
or if an exception is thrown.

Using withFile your writeFirstNLines would look like:

writeFirstNLines n source sink = do
  withFile source ReadMode (\hin -> do
    withFile sink WriteMode (\hout -> do
      ... copy from hin to hout ...
    )
  )

Now both hin and hout will automatically get closed when the copy action
completes.

replicateM

Note that replicateM_ n action will always execute action
exactly n times - no more and no less. So this is not a good way
to write a loop which could be executed a variable number of times.

As an example of how inefficient using replicateM_ here could be,
suppose that n is 1000 and you are operating on a 3 line file.
Then:

replicateM_ n readAndWrite

will execute readAndWrite 1000 times even though the last
997 calls won't copy any lines.

Here is a recipe for a loop which is executed at most n times
with the possibility of breaking out early:

loop n | n <= 0 = return ()
loop n = do quit <- condition       -- check condition
            if quit
              then return ()        -- break out early
              else ... do stuff...
                   loop (n-1)       -- perform next iteration

You can use this to copy the first n lines by substituting:

condition        =  hIsEOF hin
...do stuff...   =  hGetLine hin >>= hPutStrLn hout


Swing Game EyeHandCoordination Question copied from the book:

  (Game: eye-hand coordination) Write a program that displays a circle
  of radius 10 pixels filled with a random color at a random location on
  a panel, as shown in Figure 16.28c. When you click the circle, it
  disappears and a new randomcolor circle is displayed at another random
  location. After twenty circles are clicked, display the time spent in
  the panel, as shown in Figure 16.28d.

My solution:

import javax.swing.*;
import java.awt.*;
import java.awt.event.*;

public class EyeHandCoordination extends JFrame {
    private final int CIRCLE_RADIUS = 10;
    private final int TOTAL_CIRCLES = 20;
    private RandomCirclePanel panel = new RandomCirclePanel();

    public EyeHandCoordination() {
        add(panel);
    }

    public static void main(String[] args) {
        EyeHandCoordination frame = new EyeHandCoordination();
        frame.setTitle("EyeHandCoordination");
        frame.setSize(300, 300);
        frame.setLocationRelativeTo(null); // Center the frame
        frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        frame.setVisible(true);
      }

    private class RandomCirclePanel extends JPanel {
        private long startTime;
        private long endTime;

        private int circleX = 0;
        private int circleY = 0;

        private int currentCircle = 0;

        public RandomCirclePanel() {
            startTime = System.currentTimeMillis();

            addMouseListener(new MouseAdapter() {
                @Override
                public void mousePressed(MouseEvent e) {
                    // Check if in circle
                    if (inCircle(e.getX(), e.getY()) && currentCircle < TOTAL_CIRCLES) {
                        currentCircle++;
                        repaint();
                    }
                }
            });
        }

        /** Display circle at a random location **/
        private void changeCircleLocation() {
            circleX = (int)(Math.random() * (getWidth() - CIRCLE_RADIUS * 2));
            circleY = (int)(Math.random() * (getHeight() - CIRCLE_RADIUS * 2));
        }

        /** Check if inCircle **/
        public boolean inCircle(int mouseX, int mouseY) {
            if (distanceFromCenterOfCircle(mouseX, mouseY) <= CIRCLE_RADIUS) {
                return true;
            }

            return false;
        }

        private int distanceFromCenterOfCircle(int x, int y) {
            int centerX = circleX + CIRCLE_RADIUS;
            int centerY = circleY + CIRCLE_RADIUS;

            return (int)(Math.sqrt((centerX - x) * (centerX - x) + (centerY - y) * (centerY - y)));
        }

        @Override
        protected void paintComponent(Graphics g) {
            super.paintComponent(g);

            int width = getWidth();
            int height = getHeight();

            changeCircleLocation();

            if (currentCircle >= TOTAL_CIRCLES) {
                g.setColor(Color.BLACK);
                endTime = System.currentTimeMillis();
                FontMetrics fm = g.getFontMetrics();
                String s = "Time spent: " + ((endTime - startTime) / 1000.0) + " seconds";
                g.drawString(s, width / 2 - fm.stringWidth(s) / 2, height / 2 - fm.getAscent());
            }
            else {
                g.setColor(new Color((int)(Math.random() * 256), (int)(Math.random() * 256), (int)(Math.random() * 256)));
                g.fillOval(circleX, circleY, CIRCLE_RADIUS * 2, CIRCLE_RADIUS * 2);
            }
        }
    }
}

public boolean inCircle(int mouseX, int mouseY) {
    if (distanceFromCenterOfCircle(mouseX, mouseY) <= CIRCLE_RADIUS) {
        return true;
    }

    return false;
}

Can be

public boolean inCircle(int mouseX, int mouseY) {
    return distanceFromCenterOfCircle(mouseX, mouseY) <= CIRCLE_RADIUS;
}

(int)(Math.random() * (getWidth() - CIRCLE_RADIUS * 2));

g.setColor(new Color((int)(Math.random() * 256), (int)(Math.random() * 256), (int)(Math.random() * 256)));

Looks like you could benefit from a function that takes an integer as maximum value... and then returns an integer between 0 and max minus 1.

Which does exist - you'd simply create a new instance of Random and then call randomInstance.nextInt(maxValue).

Lastly, I think you would be better off by renaming the currentCircle variable to amountOfCirclesDisplayed because that's what it really is. currentCircle seems to point to the idea that somewhere, you have a list of circles, and right now, you're working on 1 specific circle. But that's not true, because you HAVE no list of circles and all that really matters is how many you've displayed so far.


Switch statement in PHP - cleaner way to determine priority? I have a function that takes in a controller instance, a method, and parameters (this is for a little MVC experiment I'm working on). The purpose of the function is to execute the controller method with the given parameters.

However, there are a few caveats. First, if the controller has a "router" method, it will call that instead of the method that was passed in. Second, if any of the controller "annotations" return a response, that will also override the method. Then, if those cases fail, we return the response of the actual controller method passed in.

It looks like this:

    switch (true)
    {
        // Router methods get first priority!
        case ($controller->using_router):
            $response = call_user_func_array(array($controller, 'router'));
            break;

        // Annotation responses take precedence over method responses because they are often used for authentication.
        case ( ! is_null($response = Annotation\Runner::run(Annotation\Extractor::extract($controller, $method)))):
            break;

        // No router or annotation responses... we're ready to run the method!
        default:
            $response = call_user_func_array(array($controller, $method), $parameters);
    }

For some reason, this just feels dirty. I don't like the break after checking for an annotation response. But, I'm not sure of any other way to implement a response "priority".

Is there a better way?

That's certainly a bastardization of the switch statement.

What you should use is an if...else if... group:

$response = null; //initialize
if ( $controller->using_router)
{
  $response = $controller->router();
}
else
{
  $response = Annotation\Runner::run(Annotation\Extractor::extract( $controller, $method ) );
  if ( is_null( $response )
  {
    $response = call_user_func_array( array( $controller, $method ), $parameters );
  }
}

Note: this code flows in a readable manner. The response is set, if the controller is using the router, the response is set to the output of $controller->router(), otherwise the response is set to the extracted controller method.

If that is null, the response is finally set to whatever $controller->$method(...) produces.

Code should always flow in a readable manner (unless it's minified, obfuscated, or encoded).


Array initialization inside function to be called repeatedly I have written a Fortran95 function that will be called many times in a loop.  A much reduced version of it, called myFunction, is below.  Examples of its use is the fitting of data points to this function, or numerical integration of this function.  This function is part of a small module myFunctionCollection, which can be called from the user's main program.
My worry is that I am reinitializing the local variables arrayIn1, arrayIn2, ... arrayOut3 each time it is called during runtime, and will be unnecessarily slow down the whole calculation.  Is my worry unfounded?  Or, how can I rewrite this function inside the module so that it can more efficiently be called repeatedly, without reinitializing everything over and over again?
MODULE myFunctionCollection

USE bigFatPackage   ! contains bigFatSubroutine
IMPLICIT NONE

CONTAINS

DOUBLE COMPLEX FUNCTION myFunction(a,b,c,d)

  DOUBLE COMPLEX, INTENT(IN) :: a,b,c,d
  DOUBLE COMPLEX :: arrayIn1(7), arrayIn2(7), arrayIn3(7)
  DOUBLE COMPLEX :: arrayOut1(7), arrayOut2(7), arrayOut3(7)

  arrayIn1(1:7) = (/a, a+b, c, c+d, a+d, c+d, d**2/)
  arrayIn2(1:7) = (/a**2, a-b, c, c-d, a-d, dcmplx(0d0,0d0), d**2/)
  arrayIn3(1:7) = (/a**2, (a+b)**2, c, c, c, dcmplx(0d0,0d0),dcmplx(0d0,0d0)/)

  CALL bigFatSubroutine(arrayOut1, arrayIn1, 7, 7)
  CALL bigFatSubroutine(arrayOut2, arrayIn2, 7, 7)
  CALL bigFatSubroutine(arrayOut3, arrayIn3, 7, 7)

  myFunction = arrayOut1(6) + 2d0*arrayOut2(7) + (1/2d0)*arrayOut3(2) + !... more

END FUNCTION

END MODULE

Any further suggestions on coding style is welcome!

In your function myFunction, you are initializing the vectors arrayIn* with values that depend on all the input parameters a, b, c, d, so it is unclear why you claim that you are needlessly "reinitializing" them at each call. If the function is true to its name, as it should, one would assume that the input parameters would change from call to call, in which case recomputing arrayIn* is not optional.
If, on the other hand, the parameters a, b, c, d are indeed invariant from call to call, but the result of the function changes nonetheless due to some hidden dependencies, then you should:

make the dependencies explicit ("despaghettify" your code)
initialize arrayIn* separately from the function call, e.g., through a call to an init subroutine.

For example:
MODULE myFunctionCollection

  USE bigFatPackage
  IMPLICIT NONE
  private

  DOUBLE COMPLEX :: arrayIn1(7), arrayIn2(7), arrayIn3(7)
  public :: myFunction

CONTAINS

  subroutine myInit(a,b,c,d)
    DOUBLE COMPLEX, INTENT(IN) :: a,b,c,d
    arrayIn1(1:7) = (/a, a+b, c, c+d, a+d, c+d, d**2/)
    arrayIn2(1:7) = (/a**2, a-b, c, c-d, a-d, dcmplx(0d0,0d0), d**2/)
    arrayIn3(1:7) = (/a**2, (a+b)**2, c, c, c, dcmplx(0d0,0d0),dcmplx(0d0,0d0)/)
  end subroutine myInit

  DOUBLE COMPLEX FUNCTION myFunction( FormerlyHiddenParameter )
    DOUBLE COMPLEX, INTENT(IN) :: FormerlyHiddenParameter
    CALL bigFatSubroutine(arrayOut1, arrayIn1, 7, 7, FormerlyHiddenParameter )
    CALL bigFatSubroutine(arrayOut2, arrayIn2, 7, 7, FormerlyHiddenParameter )
    CALL bigFatSubroutine(arrayOut3, arrayIn3, 7, 7, FormerlyHiddenParameter )
    myFunction = arrayOut1(6) + 2d0*arrayOut2(7) + (1/2d0)*arrayOut3(2) + !... more
  END FUNCTION myFunction

end MODULE myFunctionCollection

Finally, the operations that you are trying not to repeat are computationally very cheap, and it is unclear if you save much time, compared to the time required to execute bigFatSubroutine [sic!]. Chances are that the speed up would be negligible. To identify the real bottlenecks in your code, you should measure the duration of its parts. The rest of the code should be improved for readability, maintainability, extensibility, usability, etc. etc. Any change that increases complexity with no real gain in efficiency should be avoided.
PS: For readers not familiar with the simple syntax of modern Fortran, I suggest the following quick modern Fortran tutorial.