Comparing Color objects with == This example comes from a code snippet posted on Stack Overflow:

private double brickVal(Color c) {
    if (c == Color.RED) {
        return 10.0;
    } else if (c == Color.ORANGE) {
        return 8.0;
    } else if (c == Color.YELLOW) {
        return 6.0;
    } else if (c == Color.GREEN) {
        return 4.0;
    } else if (c == Color.CYAN) {
        return 2.0;
    } else if (c == Color.MAGENTA) {
        return 1.0;
    } else {
        return 1.0;
    }
}

Is the use of == here correct / best practice?

No, it's not correct (and thus of course also not best practice). For example the condition c == Color.GREEN will be true if the method is called as brickVal(Color.GREEN), but false if it is called as brickVal(new Color(0, 255, 0)). Since Color.GREEN is equal to new Color(0, 255, 0) this behavior is most likely unintentional (at least I can't imagine a scenario where you'd want for brickVal(Color.GREEN) to behave differently than brickVal(new Color(0,255,0)).

Of course if you know that the method will only ever be called using the "pre-made" colors and never using new Color, it will behave correctly. However I'd still advise against using ==. I can see no good reason to not use equals and using == comes with the danger that someone might call brickVal with new Color anyway, not knowing that they're not supposed to.

Further given the fact that brickVal apparently is meant to be only called with some specific colors as arguments and it doesn't use any properties of the colors other than their identity, I think an enum might be more suitable here than using the Color class. This way you can use switch instead of if ... else if ... and don't have to worry about anybody passing in new Color as the argument.

As a somewhat unrelated note, I find it confusing that the brickVal variable is set in every case except if the argument is RED and in the else case. Obviously I don't know how and where the brickVal variable is going to be used, but this seems like a design smell to me. I also think it's a bad idea to have a variable with the same name as the method.


Inizialize an Jarray wih n JObject that contain an progressive index I need to inizialize an JArray with a JObject that contain a property areaNum that is an incremental integer.

Right now I wrote the below code, which works ok if I have a small number of JObjects, otherwise it takes too long.

I'm looking for a way to write the below code that take in consideration the performance at time side

i need to initialize an JArray with n JObject that contain the areaNum that is a incremental number

Function csvToWebSocketStringSuper3(lclrow As CsvRow) As String

Dim panelsArray As New JArray()
        Dim panel As New JObject

        For i As Integer = 1 To 5
            panel = New JObject(
                New JProperty("areaNum",            i),
                New JProperty("numbers",            New JArray({New JArray({}),New JArray({}),New JArray({})})),
                New JProperty("systems",            New JArray({""})),
                New JProperty("multiplier",         New JArray({""})),
                New JProperty("qp",                 false),
                New JProperty("series",             false),
                New JProperty("noseries",           false),
                New JProperty("twodigit",           false),
                New JProperty("onedigit",           false),
                New JProperty("void",               false))
            panelsArray.Add(panel)
        Next i
    Dim obj As JObject  = New JObject(
            New JProperty("playslipID",             2331),
            New JProperty("ticketID",               2100) ,
            New JProperty("participationMethod",    0),
            New JProperty("multidraws",             New JArray({formatStringToEmptyORIntegerReplace0(lclrow.getMultipleDraws)})),
            New JProperty("advancedraws",           New JArray({formatStringToEmptyORIntegerReplace0(lclrow.getDrawOffsets)})),
            New JProperty("proto",                  New JArray({formatStringToEmptyORIntegerReplace0(lclrow.getProductId)})),
            New JProperty("areas",                  panelsArray
            ))

        Return JsonConvert.SerializeObject(obj)

Instead of creating JObject after JObject you should create it once without the areaNum property. Then in a loop you should DeepClone() it and use AddFirst(), if positions matter, to add the areNum property like so

    Dim original As JObject = New JObject(
    New JProperty("numbers", New JArray({New JArray({}), New JArray({}), New JArray({})})),
    New JProperty("systems", New JArray({""})),
    New JProperty("multiplier", New JArray({""})),
    New JProperty("qp", False),
    New JProperty("series", False),
    New JProperty("noseries", False),
    New JProperty("twodigit", False),
    New JProperty("onedigit", False),
    New JProperty("void", False))

    For i As Integer = 1 To 5
        Dim panel As JObject = original.DeepClone
        panel.AddFirst(New JProperty("areaNum", i))
        panelsArray.Add(panel)
    Next i


Water pouring challenge I'm trying to write code to solve water pouring problem in Clojure. The following code is strongly influenced by Martin Odersky's solution lectured in his coursera course, Functional Programming Principle in Scala. He first created a class Pouring with parameter capacity, which is accessible everywhere in the class. Though he made the best use of case class in his solution, I just used plain clojure map to represent the moves.

I tried to solve the problem in the same way, and end up with the following code:

(ns pouring)

(declare capacity init-state)

(defn empty [state glass]
  (assoc state glass 0))

(defn fill [state glass]
  (assoc state glass (capacity glass)))

(defn pour [state [from to]]
  (let [amt (min (state from) (- (capacity to) (state to)))]
    (-> state
        (assoc from (- (state from) amt))
        (assoc to (+ (state to) amt)))))

(defn change [state move]
  (if state
    (cond (:empty move) (empty state (:empty move))
          (:fill move) (fill state (:fill move))
          (:pour move) (pour state (:pour move)))
    (change init-state move)))

(defn moves [capacity]
  (let [glasses (range (count capacity))]
    (lazy-cat
     (map (fn [g] {:empty g}) glasses)
     (map (fn [g] {:fill g}) glasses)
     (for [from glasses, to glasses :when (not= from to)]
       {:pour [from to]}))))

(defn extend-path [path move]
  {:history (conj (:history path) move)
   :end-state (change (:end-state path) move)})

(defn extend
  ([paths explored]
   (if-let [more (for [path paths
                       next-path (map #(extend-path path %) (moves capacity))
                       :when (not (contains? explored (:end-state next-path)))]
                   next-path)]
     (lazy-cat paths (extend more (conj explored (map #(:end-state %) more))))))
  ([] (extend #{{:history [], :end-state init-state}} #{init-state})))

(defn init [c]
  (def capacity c)
  (def init-state (vec (repeat (count c) 0))))

(defn solve [capacity target]
  (init capacity)
  (first (for [path (extend)
               :when (some #(= % target) (:end-state path))]
           path)))

The part I don't like is init function. Most clojurians say that using def inside a function is bad. If I don't use init function, perhaps I have to add parameter  capacity to almost every function in the code, which doesn't look pretty.

I was considered using nested function.

(defn solve [capacity target]
  (defn empty ...)
  (defn fill ...)
  (defn pour ...)
  (defn extend ...)
  ...)

However, using defn inside function is also bad, and the functions defined inside are actually not local functions, which are accessible outside the outer function.

Yes, there's letfn in clojure. I can put every function except solve into letfn. But in this case, the code is not very readable. I think letfn is only for short (which can be expressed in 1 or 2 lines) functions.

Perhaps there are better ways to do this. Any suggestions?

Warning: The following is opinionated at times. At other times it is more opinionated.
Channeling PG
I've heard on the internet so it must be true, that Paul Graham will get right to the point when interviewing applicants to Y-Combinator with "What problem does this solve?" It's hard to understand code without understanding what it is supposed to do. And only Google and watching a video twice make me think I might. Since I still don't understand the code though, I can't tell whether it solves the problem or not.
The "water pouring problem" is a canonical toy problem in programming curricula. Wolfram Mathworld describes it as a special case of the three jugs problem. The special case is where the third jug:
   capacity = +infinity
   contents = +infinity

The infinite capacity of the third jug allows it to represent a sink into which the contents of the other jugs may be emptied. The infinite contents allows the third jug to represent a faucet for filling other jugs.
The problem Ordersky presents is none of these. It is a generalized case based on a Python example from Peter Norvig's Design of Computer Programs course at Udacity. The problem Odersky is solving has an arbitrary number of jugs with finite capacity plus one jug with infinite capacity and contents. I would like to dub this the "n+1 Jugs Problem" because 1 captures the sink/faucet and n+1 hints at the notion of +infinity [at least in my little world].
Readability
A joke: I married my wife despite her adamant unwillingness to change her name. What's the big deal about being called "Donna" to match my tattoo?
Rich Hickey invented Clojure to help working programmers write good programs, not to give the world another excuse for tribal tattoos. I don't think init or def inside a function are big deals. They're very Pythonic and they couldn't make the code less readable for idiots like me.
Chiapas
I found the code opaque. "What is state?" I asked myself. I could look at the code for hours and never really figure it out. It took a half hour of concerted effort involving Google and watching a video of Martin Odersky...twice before I came up with a good answer: state is a vector of integers. In Ordersky's code this is explicit at line 6:
 type State = Vector[Int]

That's just standard Scala. It's fast in the sense that many people will understand it as soon as they read it. The Clojure implementation, on the other hand, is really slow because the source code doesn't make the nature of state clear. A [more opinionated][more opinionated] approach might be:
 ;;;; The state is a vector of integers.
 ;;;; Each element represents a glass.
 ;;;; The value of each element represents the contents of the glass.

A more direct implementation of the Scala code might use int-array for both state and glasses. There's nothing wrong with using types to make writing correct clear performant code easier.
Recommendations
Think about how much effort Odersky invests in making his code understandable. There's thirty minutes of video. There's the hours of video that come before it in the course sequence. The coding session starts with diagrams explaining the problem. Those are habits that are worth emulating. Following what Clojurists on the internet espouse doesn't make one an expert [I am aware of the irony]. Clojure is the product of Hammock Driven Development.
Extension
I'm a big fan of Lamport's idea specification as the starting point. One of the issues with Odersky's generalized approach is that it doesn't quickly trim the search space. Consider the case with two glasses:
 (def capacity  { :a 1
                  :b 100})
 (def target  50)           ; edit: early version used 99

The solution exists in the 99th layer of Odersky's onion because it requires 99 moves. With two glasses There are only six transition operations at each level of the onion:
 ; empty a
 ; empty b
 ; pour from a to b
 ; pour from b to a
 ; fill a
 ; fil  b

Yet the brute force search would cover:
 level | paths
 ------|-------
 1     |  2
 2     |  12
 3     |  72
 4     |  432
 ...   |  ...
 99    |  3.6295479083337274E76 = (6^99)/3

At an arbitrary iteration some may lead to states already visited and Odersky's approach trims those. What it doesn't do is capture the semantics of bags to trim the search space. Consider:
 (def capacity  { :a 1
                  :b 100
                  :c 1
                  :d 1
                  :e 1
                  :f 1})
 (def target  50)           ; edit: early version used 99

Many of the (Math/pow 42 99) states turn out to be equivalent and finding these equivalences would greatly reduce the search space and make for a more performant algorithm. For the purposes of Odersky's course [promoting Scala and functional programming] looking at algorithmic improvements is outside the mandate. For general code such as that under open review, it really isn't.


Class to read the property files I wrote the following code to read the config/property files. I will be exposing my api as jar to multiple application teams.

Functionality:

Reads the property files and returns the resource bundle object. I am returning the resource bundle object as, there can be multiple config files in the application.
Getting the properties or the configs as variables from the resource bundle and use it across the application.

public class FileConfigLoader {

    private static final Logger LOGGER = Logger.getLogger(FileConfigLoader.class);

    private static String EMPTY = "";

    public static ResourceBundle getBundle(String basePath, String baseName) throws MalformedURLException {

        File file = new File(basePath);
        URL[] uris = { file.toURI().toURL() };
        ClassLoader loader = new URLClassLoader(uris);
        ResourceBundle rb = ResourceBundle.getBundle(basePath, Locale.getDefault(), loader);
        return rb;
    }

    public static String getValue(ResourceBundle rb, String key) {
        String value = EMPTY;

        try {
            value = rb.getString(key);
        } catch (MissingResourceException mREx) {
            LOGGER.error("Missing Resource : " + key);
        }

        return value;
    }

}

I need code reviews on the following lines

Does my code looks reusable?
Do you think i am using the variable names and class names as per the java sandards?
If my code does not seems to be reusable, Please provide suggestions.

The below code is the demonstration of how my code used by applications in the realtime.

The config is the name of my config file.The file is located in the classpath.

import java.net.MalformedURLException;
import java.util.ResourceBundle;

import org.apache.log4j.Logger;

public class MainApp {

    private static final Logger LOGGER = Logger.getLogger(MainApp.class);
    public static void main(String[] args) {

        ResourceBundle resource = null;

        try {
            resource = FileConfigLoader.getBundle("config", "");
        } catch (MalformedURLException e) {
            e.printStackTrace();
        }

        String hive_db = FileConfigLoader.getValue(resource, "hive_db");
        String EDGE_HIVE_CONN = FileConfigLoader.getValue(resource, "EDGE_HIVE_CONN");
        String target_dir = FileConfigLoader.getValue(resource, "target_dir");
        String somekey = FileConfigLoader.getValue(resource, "somekey");

        LOGGER.info(hive_db);
        LOGGER.info(EDGE_HIVE_CONN);
        LOGGER.info(target_dir);
        LOGGER.info(somekey);
    }

}

A couple points:

Encapsulate the resource bundle in an instance of the class instead of using static methods.
If the key doesn't exist, don't return the empty string. If this situation is acceptable, return null or change the return type to Optional<String> and return Optional.none(). If this situation indicates an error, throw an exception.
Don't declare MalformedURLException. That is an implementation detail. Catch and throw a more appropriate exception.


Processing a webpage source I have developed a test app that works by getting webpages source and processing it. I have up to 100k URLs in queue and want to use a maximum of 25 threads. Please help me check if the code is good or point out any faults you might find.

Public Class frmMain
    Public Delegate Sub AddItemDelegate(ByVal Item As String, ByVal Status As String)
    Private TotalItems As Integer =0

    Public Sub AddItem(ByVal item As String, ByVal status As String)

        If Me.InvokeRequired Then
            Me.Invoke(New AddItemDelegate(AddressOf AddItem), Item, Status)
        Else
            Dim objLock As Object = New Object()
            Dim lvItem As New ListViewItem

            With lvItem
                .Text = Item
                If Status.Length < 200 Then
                    .SubItems.Add(Status)
                Else
                    .SubItems.Add(Status.Length.ToString)
                End If
            End With

            If listView1.Items.Count <TotalItems-1 Then
                listView1.Items.Add(lvItem)
            Else
                tsslstatus.Text ="Done"
                btnStart.Enabled =true
            End If
        End If
    End Sub

    Public Sub DoWork(ByVal objItem As Object)
        static _sem as Semaphore = new Semaphore(5,10)
        Dim objUrl as String = DirectCast(objItem, string)

        Try
            If objUrl Is Nothing Then
                exit sub
            End If

            Dim URL as New Uri(objUrl)
            _sem.waitone()
            AddItem(URL.AbsoluteUri,gethtml(URL))
            _sem.Release()
        Catch ex As Exception
            debug.Print("Error " & ex.StackTrace )
        End Try
    End Sub

    Private Sub btnStart_Click(ByVal sender As System.Object, _
        ByVal e As System.EventArgs) Handles btnStart.Click
        btnStart.Enabled =False
        dim blnResult as Boolean = ThreadPool.SetMaxThreads(25, 25)
        System.Threading.Thread.Sleep(1000)
        listView1.Items.Clear

        For Each sItem As String In txturls.Lines
            if sItem.Trim <>string.empty
                ' Queue a task
                If sItem.Contains("http://")=False Then
                    sItem="http://" & sItem
                End If

                ThreadPool.QueueUserWorkItem(New WaitCallback(AddressOf DoWork), sItem)
                TotalItems+=1
                tsslTotal.Text =String.Format ("Total Tasks: {0}",TotalItems)
            end if
        Next
    End Sub
End Class

Relying on the max number of thread pool threads to cap the number of concurrent downloads feels ... wrong. It may also cause problems if another component needs a thread pool while all 25 are waiting for a response from web servers.

(slight correction) Although you use semaphores to limit the number of concurrent downloads to ten, you will still have up to 25 thread pool threads in the DoWork() method (15 blocked on the semaphore).

Two alternatives:

Manage you own task list and threads (since 4k URLs per thread could take more than a few seconds).
Make your requests asynchronously and track the list of IAsyncResult objects (though this may get a little fiddly).

I'm not sure I like the idea of using a static Semaphore to protect instance data, you should store the semaphore in a local field. Even if you only ever have a single instance, I prefer to scope the lock similarly to the data to be protected. (sorry, I thought the semaphore was to protect the controls rather than used as a throttle)
You should release your semaphore in a finally block.
Only the thread that created a control should be used to update it. I recommend using ISynchronizeInvoke.Invoke() to call AddItem() (implemented implicitly on Control). This will also remove the need to use a semaphore to protect the control (though you may still need one to protect a pending URL list depending on what you do about the first point). (this is already done, sorry, missed it).
What is the purpose of the 1000 millisecond sleep? it appears before the tasks are even queued.


DAO for happenings registration website As a learn-by-doing project to learn Java webapps, I'm creating a website for registering and displaying happenings using JSP/Servlets, Apache Tomcat server, JDBC and a MySQL database.

I use JSP for views, Servlets for controller functionality and business logic, and I've created ONE DBManager.java class (my only DAO) to abstract SQL statements from my Servlets and JSPs. In my Servlets, I use the DBManager class by declaring a new instance and calling its public methods.

For instance:

DBManager dbm = new DBManager();
Boolean userIsUpdated = dbm.updateUserInDB(user);

My DAO looks briefly like this:

public class DBManager {

    final String DATABASE_URL = "NOPE, I won't give you this one ;)";
    Connection connection = null;
    PreparedStatement pstmt = null;

    public DBManager() {
        //Nothing in constructor yet.
    }

    public ArrayList<Happening> getAllActivatedHappenings() {
        //code removed for readability
    }

    public User logInAndGetUserByEmailAndPassword(String email, String password) {
        //code removed for readability
    }

    public User insertUserIntoDB(User user) {
        //code removed for readability
    }

    public boolean updateUserInDB(User user) {
        //code removed for readability
    }

    public boolean insertHappeningIntoDB(Happening happening) {
        //code removed for readability
    }

    public boolean deleteHappeningFromDB(int happeningId, int userId) {
        //code removed for readability
    }

    public ArrayList<HappeningType> getAllHappeningTypes() {
        //code removed for readability
    }

    public ArrayList<Happening> getAllHappeningsByUserId(int userid) {
        //code removed for readability
    }

    public byte[] getPhotoByHappeningId(int happeningId, String imageType) {
        //code removed for readability
    }

    public Map<Integer, String> getCountiesMap() {
        //code removed for readability
    }

    public Boolean userIsHappeningOwner(int userId, int happeningId) {
        //code removed for readability
    }

    public Happening getHappeningByHappeningId(int happeningId) {
        //code removed for readability
    }

    public boolean updateHappening(Happening happeningDummy, String updateImage) {
        //code removed for readability
    }

    public ArrayList<Happening> getFilteredActivatedHappenings(HttpServletRequest request) {
        //code removed for readability
    }

    public boolean activateHappening(int happeningId) {
        //code removed for readability
    }

    public boolean updateEmailNotificationForUser(User user, String emailnotificationhappeningtypeids, String emailnotificationcountyids) {
        //code removed for readability
    }

    public User getUserByEmail(String email) {
        //code removed for readability
    }
}

As you can see, my DAO handles quite a lot of different transfer objects (happenings, users, cities, happening types etc.). Should I break this DAO into several smaller DAO classes (like HappeningDAO, UserDAO, etc.)? Any other reasons for doing this than readability?

Also, is this approach thread-safe as long as I declare a new instance of DBManager within my doGet/doPost methods in the servlets?

Just a quick note: ArrayList<...> return types should be simply List<...>. See: Effective Java, 2nd edition, Item 52: Refer to objects by their interfaces


Mutex implementation for a uniprocessor bare metal embedded OS I wrote this recently for one of my projects. Are there any error you can spot or a feature which could be implemented without eating up resources or some optimisations? Also, this isn't meant for multi-cores.

/*******************************************************************************
 * @file    mutex.c
 * @date    31st August 2012
 * @brief   Generic implementation of mutex, read /notes/thread-safety
 ******************************************************************************/

#include <mutex.h>

OS_ERR mutex_acquire_try(mutex_t *mutex)
{
    if(unlikely(cpu_atomic_cmpxchg(&mutex->lock, OS_UNLOCKED, OS_LOCKED) != OS_OKAY))
    {
        if(mutex->owner == current_thread)
            goto recursive;

        return OS_EBUSY;
    }

    mutex->owner = current_thread;

recursive:
    mutex->recnt++;
    return OS_OKAY;
}

static inline OS_ERR mutex_acquire_helper(mutex_t *mutex, time_t timeout)
{
    while(unlikely(cpu_atomic_cmpxchg(&mutex->lock, OS_UNLOCKED, OS_LOCKED) != OS_OKAY))
    {
        if(mutex->owner == current_thread)
            goto recursive;

        if(mutex->lock == OS_DEAD)
            return OS_EINVAL;

        if(current_thread->priority >= mutex->owner->priority)
            thread_boost(mutex->owner);

        if(thread_queue_block(&mutex->queue, timeout) != OS_OKAY)
            return OS_ETIMEOUT;
    }

    mutex->owner = current_thread;

recursive:
    mutex->recnt++;
    return OS_OKAY;
}

OS_ERR mutex_acquire_timeout(mutex_t *mutex, time_t timeout)
{
    return mutex_acquire_helper(mutex, timeout);
}

OS_ERR mutex_acquire(mutex_t *mutex)
{
    return mutex_acquire_helper(mutex, OS_TIME_INFINITE);
}

OS_ERR mutex_release(mutex_t *mutex)
{
    if(mutex->owner != current_thread)
        return OS_EINVAL;

    if(--mutex->recnt)
        goto exit;

    thread_deboost(mutex->owner);
    mutex->owner = NULL;
    cpu_atomic_set(&mutex->lock, OS_UNLOCKED);
    thread_queue_wake_one_now(&mutex->queue);

exit:
    return OS_OKAY;
}

void mutex_init(mutex_t *mutex)
{
    mutex->recnt = 0;
    mutex->owner = NULL;
    thread_queue_init(&mutex->queue);
    cpu_atomic_set(&mutex->lock, OS_UNLOCKED);
}

void mutex_deinit(mutex_t *mutex)
{
    if(mutex->owner == current_thread)
    {
        if(mutex->recnt > 1)
            LOGE("Mutex: Deinit called on recursive lock @ %p\n", mutex);

        mutex_release(mutex);
    }

    do {
        while(thread_queue_count(&mutex->queue))
            thread_yield();
    } while(cpu_atomic_cmpxchg(&mutex->lock, OS_DEAD, OS_UNLOCKED) != OS_OKAY);
}

Does this really work?  You call cpu_atomic_cmpxchg without taking any notice of its return value. I guess the function must return true or false (1 or 0 etc) according to whether the lock was already taken or not and it seems likely that you should take notice of that.

Also, if it were my code I'd drop all the unlikely() and likely() calls.  For my money, they just obscure the code and I doubt they are necessary.

I haven't read the functions in detail, so maybe I misunderstood the code...

EDIT: Read it again and have some comments:

Your point 10: "Mutex MUST not be freed without calling mutex_deinit." is
strange.  Isn't there a difference between freeing the mutex (lock/free is
what you normally do, no?) and de-initialising it (which you would do when
it is no longer needed, if at all)?
'restrict' used in functions with a single parameter has no purpose
Reviewers and readers would be more likely to understand what is going on if
your functions had some commets as to their intended purpose/use/return
values etc.
How do the 'atomic' functions wait for mutex availability?
Shouldn't mutex_init fail on an already-initialised mutex?
How do you know when it is safe to deinit a mutex?
Your mutex flags are confusing.  You have DEAD, BUSY, LOCKED, SUCCESS,
READY.  Seems likely to me that there is some redundancy here.  Or
conflating state flags and return values.
What is the point in all those MUTX flags if you assume the value of one of
them (in  if(unlikely(ret))) - assuming the atomic function returns one.
What is the difference between BUSY and LOCKED?
mutex_acquire_timeout doesn't tell the caller whether a timeout occurred!
(or whether enquing falied!)
So if the mutex has an owner already in mutex_acquire_timeout, you put the
thread onto a queue, with a timeout.  Presumably thread_queue_enqueue
returns either when awoken by thread_queue_wake_highest_priority in
mutex_release, or when there is a timeout.   But you carry on and
replace the mutex owner whatever the case...
Your point 4 says a mutex can only be released by its owner, and yet
mutex_release goes ahead and plays with the mutex queue whoever calls it.
Also sets state (ie releasing the local lock) before playing with the queue;   surely there will be a race condition there.

Sorry, but I don't believe this code 'works' in a meaningful way.  Why are
you writing these primitives anyway?  Why not use an existing kernel?  It is
generally a mistake to write your own (although I've worked at several that
have done, with bad consequences).


Efficiency with strcpy, strcat and malloc I'm still learning C, but I'm trying to make sure I've got a decent grasp on working with "strings" and data structures.

If possible, I'd like a little input on how I'm handling this and see if

it could be done more efficiently
I'm setting myself up for unforeseen negative consequences
I'm using the functions appropriately

void test(char *username, char *password) {

    printf("Checking password for %s - pw: %s\n",username,password);
    char *query1 = "SELECT password FROM logins WHERE email = '";
    char *query2 = "' LIMIT 1";

    char *querystring = malloc(strlen(query1) + strlen(username) + strlen(query2) * sizeof(char));

    strncpy(querystring,query1,strlen(query1));
    strncat(querystring,username,strlen(username));
    strncat(querystring,query2,strlen(query2));

    printf("Query string: %s\n",querystring);

    mysql_query(mysql_con,querystring);
    MYSQL_RES *result = mysql_store_result(mysql_con);

    int num_fields = mysql_num_fields(result);
    int num_rows = mysql_num_rows(result);

    if (num_rows != 0) {

        MYSQL_ROW row;
        printf("Query returned %i results with %i fields\n",num_rows,num_fields);

        row = mysql_fetch_row(result);

        printf("Password returned: %s\n",row[0]);

        int comparison = strncmp(password, row[0], strlen(password));

            if (comparison == 0) {
                printf("Passwords match!\n");
            } else {
                printf("Passwords do NOT match!\n");
            }

            }
    else {
        printf("No such user... Password is invalid");
    }
        free(querystring);
}

Output of program:

Checking password for test@blah.com - pw: 5f4dcc3b5aa765d61d8327deb882cf99
Query string: SELECT password FROM logins WHERE email = 'test@blah.com' LIMIT 1
Query returned 1 results with 1 fields
Password returned: 5f4dcc3b5aa765d61d8327deb882cf99
Passwords match!

Fairly good C. Few notes:

sizeof(char) is by 1 definition. It doesn't hurt to multiply by it, but still totally unnecessary.
malloc return value better be tested against NULL.
Calling functions of strn (emphasis on n) family in this context is kinda paranoid: you've already allocated enough memory; no need for extra safety. In fact, I'd rather go for sprintf("%s%s%s", querystring, query1, username, query2).
int comparison is just noise. It's OK to directly test results of strcmp(). BTW, strncmp in this context looks paranoid again.
I am not familiar with mysql. Shouldn't you release row somehow?


UNUSED() with teeth in C++11 debug builds When you turn up the compiler warnings with -Wunused-variable, one tends to then employ an UNUSED() macro, to say something is intentionally not used at this time.

But that's not very binding, since it's generally just a void cast (or near-equivalent).  Hence if you get the warning, add an UNUSED(), and some time later actually do use the variable...it's misleading to to the reader of the code.

I got the odd idea of making C++11 give this some level of "enforcement", by seeing if it got in a POD lvalue it could trash it.  Obviously won't work for everything (unused constants, etc.)  But I kind of fiddled with it until it had the effect I wanted--while I can't say exactly why every variation I tried didn't work.

You could break it out in various ways to do something special for std::is_pointer<> or other types, or even have some kind of "trashme()" method detection for non-POD values.  But here's the core of the idea:

#if !defined(NDEBUG)

template<
    typename T,
    typename TRR = typename std::remove_reference<T>::type,
    typename std::enable_if<
        !std::is_lvalue_reference<T &&>::value
        || std::is_const<TRR>::value
        || !std::is_pod<TRR>::value
    >::type* = nullptr
>
void UNUSED(T && v) {
    static_cast<void>(v);
}

template<
    typename T,
    typename TRR = typename std::remove_reference<T>::type,
    typename std::enable_if<
        std::is_lvalue_reference<T &&>::value
        && !std::is_const<TRR>::value
        && std::is_pod<TRR>::value
    >::type* = nullptr
>
void UNUSED(T && v) {
    memset(&v, 0xBD, sizeof(TRR)); // or 0xDECAFBAD pattern, etc.
}

#endif

Anything cleaner/simpler to achieve the same effect, or ideas to make it better?

Missing headers

This code needs

#include <type_traits>
#include <cstring>

Use sizeof operator on a value

While we can write sizeof (type), in this case we have a value, and we can use it here - sizeof will read through the reference, saving us some work:

std::memset(&v, 0xBD, sizeof v);

Reduce repetition

There are two expressions that need to be maintained in parallel in the std::enable_if template.  We can refactor these into a single trait template:

template<typename T>
struct is_mutable_pod_lvalue
    : std::integral_constant<bool,
                             std::is_lvalue_reference<T&&>::value
                             && !std::is_const<typename std::remove_reference<T>::type>::value
                             && std::is_pod<typename std::remove_reference<T>::type>::value>
{};

Then the functions reduce to

template<typename T>
typename std::enable_if<!is_mutable_pod_lvalue<T>::value>::type
UNUSED(T && v) {
    static_cast<void>(v);
}

template<typename T>
typename std::enable_if<is_mutable_pod_lvalue<T>::value>::type
UNUSED(T && v) {
    std::memset(&v, 0xBD, sizeof v);
}

In C++14, we could use the _t and _v forms to avoid typing ::type and ::value respectively, and in C++17, we could use std::bool_constant instead of std::integral_constant.

If we were using C++17, we could further reduce to a single function, by moving the test within the function body:

template<typename T>
UNUSED(T&& v) {
    if constexpr (is_mutable_pod_lvalue<T>::value)
        std::memset(&v, 0xBD, sizeof v);
}

Now we're no longer using is_mutable_pod_value more than once, we can just inline it back again:

template<typename T>
void UNUSED(T&& v) {
    static const bool is_mutable_pod_lvalue =
        std::is_lvalue_reference<T&&>::value
        && !std::is_const<typename std::remove_reference<T>::type>::value
        && std::is_pod<typename std::remove_reference<T>::type>::value;

    if constexpr ((is_mutable_pod_lvalue)
        std::memset(&v, 0xBD, sizeof v);
}

Enhancement: accept multiple arguments

Here's a varargs template that makes it easier to use:

template<typename T, typename... U>
void UNUSED(T&& v, U&&... u)
{
    UNUSED(v);
    UNUSED(u...);
}

Again, C++17 makes this simpler, with fold expressions:

template<typename... T>
void UNUSED(T&&... v)
{
    (UNUSED(v) , ...);
}

A definition is required when NDEBUG is set

Probably just an oversight, but the code is missing an #else block.  It's probably worth reordering so that the positive branch comes first (confusing because NDEBUG is itself a negative...).  Further, since the only difference between the debug and non-debug cases it the use of std::memset() in one of the implementations, we can reduce the scope of #ifdef to just that (at a small cost to non-debug compilation times):

template<typename T>
typename std::enable_if<is_mutable_pod_lvalue<T>::value>::type
UNUSED(T&& v)
{
    static_cast<void>(v);
#ifndef NDEBUG
    std::memset(&v, 0xBD, sizeof v);
#endif
}

Revised code

#include <type_traits>
#include <cstring>
template<typename T>
struct is_mutable_pod_lvalue
    : std::integral_constant<bool,
                             std::is_lvalue_reference<T&&>::value
                             && !std::is_const<typename std::remove_reference<T>::type>::value
                             && std::is_pod<typename std::remove_reference<T>::type>::value>
{};

template<typename T>
typename std::enable_if<!is_mutable_pod_lvalue<T>::value>::type
UNUSED(T&& v)
{
    static_cast<void>(v);
}

template<typename T>
typename std::enable_if<is_mutable_pod_lvalue<T>::value>::type
UNUSED(T&& v)
{
    static_cast<void>(v);
#ifndef NDEBUG
    std::memset(&v, 0xBD, sizeof v);
#endif
}

template<typename T, typename... U>
void UNUSED(T&& v, U&&... u)
{
    UNUSED(v);
    UNUSED(u...);
}

#include <memory>
void f(int i, const int ci, int& ri, const int& cri, int *pi,
       std::unique_ptr<int> ui, std::unique_ptr<int>&& rrui)
{
#ifdef TEST_UNUSED
    UNUSED(i, ci, ri, cri, pi, ui, rrui);
#endif
}

Four compilations are required to test this fully for all combinations of TEST_UNUSED and NDEBUG.


Counting the number of 1 bits in a sorted bit array I wrote a function in python to count the number of 1-bits in a sorted bit array. I'm basically using binary search but the code seems unnecessarily long and awkward. I did test it for several different cases and got the correct output but am looking to write a cleaner code if possible.

Note: I want to use a binary-search inspired solution that theoretically takes O(lg n ) time.

def countOnes(arr):
    l, r = 0, len(arr)-1
    while l <= r:
        mid = l + (r-l)/2
        if mid == 0 and arr[mid] == 1:
            return len(arr)
        if mid == len(arr)-1 and arr[mid] == 0:
            return 0
        if arr[mid] == 1 and arr[mid-1] == 0:
            return len(arr) - mid
        elif arr[mid] == 1 and arr[mid-1] == 1:
            r = mid - 1
        elif arr[mid] == 0 and arr[mid+1] == 1:
            return len(arr) - (mid+1)
        else:
            l = mid + 1

print countOnes([0, 0, 0]) # got 0
print countOnes([0, 0, 1, 1]) # got 2
print countOnes([1, 1, 1, 1]) # got 4
print countOnes([0, 1, 1, 1]) # got 3
print countONes([1, 1, 1, 1, 1]) # got 5

I agree with your assessment of the code being unnecessarily long and awkward. It looks like the root cause is an opportunistic optimization.

The bad news is that most of the secondary tests (like arr[mid - 1] == 0 and arr[mid - 1] == 1) are bound to fail, so not only they contribute to awkwardness of the code - they also hurt the performance (by about a factor of 2, if I am not mistaken).

I recommend to keep it simple, and test only what is necessary:

    l, r = 0, len(arr)
    while l < r:
        mid = l + (r - l) // 2
        if arr[mid] == 1:
            r = mid
        else:
            l = mid + 1
    return len(arr) - l

Notice that working on a semi-open range (that is, mid is beyond the range) also makes it cleaner.


"File Fix-it" challenge I'm learning Python and have found this problem from Google Code Jam:

How many mkdir commands does it take to construct a given directory tree:
Input
The first line of the input gives the number of test cases, T. T test cases follow. Each case begins with a line containing two integers N and M, separated by a space.
The next N lines each give the path of one directory that already exists on your computer. This list will include every directory already on your computer other than the root directory. (The root directory is on every computer, so there is no need to list it explicitly.)
The next M lines each give the path of one directory that you want to create.
Each of the paths in the input is formatted as in the problem statement above. Specifically, a path consists of one or more lower-case alpha-numeric strings (i.e., strings containing only the symbols 'a'-'z' and '0'-'9'), each preceded by a single forward slash. These alpha-numeric strings are never empty.
Output
For each test case, output one line containing "Case #x: y", where x is the case number (starting from 1) and y is the number of mkdir you need.

I've solved it by writing code shown below, and it works correctly, but how can I make it faster?
import sys

def split_path(path_file,line_count_to_be_read):
    for i in range(line_count_to_be_read):
        # Get the Path line
        line = path_file.readline()
        # Skip the first slash
        line = line[1:]
        line = line.strip()
        splited = line.split('/')
        # make each subpaths from the source path
        for j in range(1,len(splited)+1):
            joined = "/".join(splited[:j])
            yield joined

def main():

    file_name = ""

    try:
        file_name = sys.argv[1]
    except IndexError:
        file_name = "A-small-practice.in"

    with open(file_name) as path_file:

        # skip the first line
        line = path_file.readline()
        total_testcases = int(line) # Number of Test Cases - Unused

        case_no = 0

        while True:
            line = path_file.readline()

            if not line:
                break

            # Get Existing path and New path count
            existing_count,new_count = line.split()
            existing_count = int(existing_count)
            new_count = int(new_count)

            # Split Every Path and Make all Subpaths
            existing_paths = list(split_path(path_file,existing_count)) # Existing Subpaths
            new_paths = list(split_path(path_file,new_count)) # New Subpaths

            # Remove all paths that is in Existing list from the New list
            new_mkdir_set = set(new_paths) - set(existing_paths)

            case_no += 1

            # length of new_set contains number of needed mkdir(s)
            print "Case #{0}: {1}".format(case_no,len(new_mkdir_set))

if __name__ == '__main__':
    main()

Rather than using a list, consider approaching the problem from a graph perspective.  Build a tree from the root directory using the existing directories, then traverse it with the new directories, and output a result line each time you have to add a leaf to the graph.  This should be a little faster than comparing your two sets.


Method Ordering - Readability / Maintainability What is your opinion on method ordering?

Should it be public methods at the top followed by private methods?

or

Should it be linear order so that methods calling methods stack on top of each other?

Which do you think is better for maintainability / readability?

Examples

Public first:

public class Main {

    @Override
    public void onCreate(Bundle savedInstanceState) {
        //
    }

    @Override
    protected void onResume() {
        startUploadActivity();
    }

    @FromXML
    public void onSignInClick(View button){
        startAuthorisationForYouTube();
    }

    @Override
    protected void onActivityResult(int requestCode, int resultCode, Intent data) {
        if(resultIsASignInAttempt(requestCode) && signedInSucccessfully(resultCode)){
            dealWithResult(data);
        } else
        if(resultIsASignInAttempt(requestCode) && signInFailed(resultCode)){
            startRefusalActivity();
        }
    }

@Override
protected void onStop() {
    super.onStop();
}

    private void startUploadActivity() {
        //
    }

    private void startAuthorisationForYouTube() {
        //
    }

    private static boolean resultIsASignInAttempt(int requestCode) {
        //
    }

    private static boolean signedInSucccessfully(int resultCode) {
        //
    }

    private static boolean signInFailed(int resultCode) {
        //
    }

    private void dealWithResult(Intent data) {
        //
    }

    private static Tokens getAccessTokens(Intent data) {
        //
    }

    private void startRefusalActivity() {
        //
    }
}

Calling order:

public class Main {

    @Override
    public void onCreate(Bundle savedInstanceState) {
        //
    }

    @Override
    protected void onResume() {
            startUploadActivity();

    }

    private void startUploadActivity() {
        //
    }

    @FromXML
    public void onSignInClick(View button){
        startAuthorisationForYouTube();
    }

    private void startAuthorisationForYouTube() {
        //
    }

    @Override
    protected void onActivityResult(int requestCode, int resultCode, Intent data) {
        if(resultIsASignInAttempt(requestCode) && signedInSucccessfully(resultCode)){
            dealWithResult(data);
        } else
        if(resultIsASignInAttempt(requestCode) && signInFailed(resultCode)){
            startRefusalActivity();
        }
    }

    private static boolean resultIsASignInAttempt(int requestCode) {
        //
    }

    private static boolean signedInSucccessfully(int resultCode) {
        //
    }

    private static boolean signInFailed(int resultCode) {
        //
    }

    private void dealWithResult(Intent data) {
        //
    }

    private static Tokens getAccessTokens(Intent data) {
        //
    }

    private void startRefusalActivity() {
        //
    }

@Override
protected void onStop() {
    super.onStop();
}
}

I don't have strong feelings, but I keep methods in calling order.  The argument for "public up top" seems to be that someone wants to see what methods they can call on an Object as opposed to what a particular method does.  However in today's world, this is generally accomplished by an IDE or javadoc.  If that's not possible, the time lost jumping through a file looking for the public methods is minimal.

When I'm looking at code for a class I haven't written, I'm inevitably trying to find out how it works.  And when reading the content of a method, jumping to private methods becomes more important, and slightly more time is lost if the class isn't ordered for callability.


Finding palindromic numbers most efficiently After reading why is my project euler #4 program not working?, I was contemplating the most efficient way to approach this (finding the target palindrome as early as possible).

Is this snippet about as efficient as it gets?  I am only considering the merits of the logic, not the choice of language.  It would be easier (but not as efficient) to brute-force it with two nested loops.

Also, is this logic common enough for it to have a name?

#!/usr/bin/perl
use strict;
$^W = 1;
$\ = "\n";

my ($n) = @ARGV;
$n = 3 if (! defined $n || $n < 2 || $n > 8);
my ($lo,$hi,$lm);
$lo = substr('10000000000',0,$n);
$hi = substr('99999999999',0,$n);
$lm = substr('100000000000000000000',0,$n*2);

my ($a,$b,$x) = ($hi,$hi);
for (;;) {
    $x = $a*$b;
    if ($x eq reverse $x) {
        print "$x is $a * $b" if ($x > $lm);
        last if ($n > 3); # show them all for small numbers
    }
    next if (!(--$a < $lo) & !(++$b > $hi)); # no short-cut operator here
    ++$a; --$b;
    print "  edge $a $b" if ($n < 3); # show edge for small numbers
    $a += $b; $b = int --$a/2; $a -= $b;
    last if ($a*$b < $lm);
}

Here, you first check if $x is palindrome, but then you will only print it if $x > $lm:

if ($x eq reverse $x) {
    print "$x is $a * $b" if ($x > $lm);
    last if ($n > 3); # show them all for small numbers
}

It would be more efficient to do the other way around, checking $x > $lm first:

if ($x > $lm) {
    if ($x eq reverse $x) {
        print "$x is $a * $b";
        last if ($n > 3); # show them all for small numbers
    }
}

With \$n = 3\$, this makes a big difference: it saves reversing $x 23602 times.

I also tried rewriting the reverse operation,
somewhat inspired by the the duck's comment:
instead of comparing an entire string with its reverse,
it should be more efficient to compare its first half with the reverse of its second half:

sub is_palindrome {
    my ($word) = (@_);
    my $l = length($word);
    foreach my $i (0 .. $l / 2) {
        if (substr($word, $i, 1) ne substr($word, $l - $i - 1, 1)) {
            return;
        }
    }
    return 1;
}

Although this function does only half of the comparisons as in $x eq reverse $x,
at least as interpreted code, this cannot compete with the speed of the compiled reverse function. Using this function the script runs much slower, unfortunately.


Euler 35 - python solution taking too long Here is my solution for Project Euler 35. (Find the number of circular primes below 1,000,000. Circular meaning all rotations of the digits are prime, i.e. 197, 719, 971.) The code takes about 30 minutes to run. Can you help me identify which parts of the algorithm are hogging up computation time?

I know there are many solutions out there, I just want to know why this one is soooo slow. I suspect it has something to do with the p.count function call.

p is initialized to a list of all primes below 1 million using the Sieve of Eratosthenes.
total is initialized to 0.

for i in p:
    primer = str(i)
    circ = 1
    for j in range(len(primer)-1):
        primer = primer[1:]+primer[:1]
        if (p.count(int(primer)) == 1):
            circ += 1
    if circ == len(primer):
        total += 1

Your p is a list.  Then you do p.count(x) to see if x is a prime.  This does a linear search of the list, and in fact, has to examine every element of p, not just those up to the occurrence of x.  Instead, change p to a set.  It will run much faster.  Also, once you see that a rotation is not prime, you can stop checking all the rotations.


Creating and parsing query strings In a moment of boredom, I decided to write two methods, one of which would create a query string, while the other would parse it, the constraint I imposed on myself being that each method could not exceed one statement. The result is by no means something I'd ever consider using at work or in my personal projects, for that matter; my reason for posting this question is that I'd like to know how, given the constraint I set for myself, the code could be improved on? Like, what's the best code you could write to solve the problem at hand - creating and parsing a query string - without having the two methods constitute more than 2 statements? I'm sure you could do it in a much more elegant manner, and I'm looking forward to any feedback coming my way!

const createQueryString = input => encodeURI(`?${Object.keys(input).map(name
=> `${name}=${input[name]}`).join('&')}`);

const queryString = createQueryString({
  name: 'Darrell',
  age: '3     2',
  nationality: 'Swedish'
});

console.log(queryString);

const parseQueryString = (input, obj = Object.create(null)) =>
decodeURI(input.slice(input.indexOf('?') + 1,
input.length)).split('&').map(entry => [
  [entry.split('=')[0]],
  [entry.split('=')[1]]
]).reduce((obj, query) => Object.assign(obj, {
  [query[0][0]]: query[1][0]
}), {});

const parsedQueryString = parseQueryString(queryString);

console.log(parsedQueryString);

Well this is a fascinating question!

I don't see much you could do in createQueryString besides potentially using Object.entries instead of Object.keys and a slight rearrangement to get rid of the outer template string.

const createQueryString = input =>
  '?' + encodeURI(Object.entries(input)
    .map(([name, value]) => `${name}=${value}`).join('&'));

Do note that there is a problem here if your name or value contains &. To fix this you would need to call encodeURIComponent on each part and then drop the outer enocdeURI call.

const createQueryString = input =>
  '?' + Object.entries(input)
    .map(([name, value]) =>
      `${encodeURIComponent(name)}=${encodeURIComponent(value)}`
    ).join('&');

parseQueryString can be improved in several ways.

You don't use obj = Object.create(null), so get rid of it.
There is a (mostly) a built in way to do this which I will pretend doesn't exist for the rest of this post. (One disadvantage with this method is that it requires a full URI, not just the query string)

[...new URL(uri).searchParams.entries()]
    .reduce((obj, entry) => Object.assign(obj, { [entry[0]]: entry[1] }), {})

You can use regular expressions to simplify the code slightly.

const parseQueryString = (input) =>
  input.slice(input.indexOf('?') + 1)
  .match(/[\w\d%\-!.~'()\*]+=[\w\d%\-!.~'()\*]+/g)
  .map(s => s.split('=').map(decodeURIComponent))
  .reduce((obj, [key, value]) => Object.assign(obj, { [key]: value }), {})

Full demo:

const createQueryString = input =>
  '?' + Object.entries(input)
    .map(([name, value]) =>
      `${encodeURIComponent(name)}=${encodeURIComponent(value)}`
    ).join('&');

const queryString = createQueryString({
  name: 'Darrell',
  age: '3     2',
  nationality: 'Swedish',
  messy: '&hi=bye'
});

const parseQueryString = input =>
  input.slice(input.indexOf('?') + 1)
  .match(/[\w\d%\-!.~'()\*]+=[\w\d%\-!.~'()\*]+/g)
  .map(s => s.split('=').map(decodeURIComponent))
  .reduce((obj, [key, value]) => Object.assign(obj, { [key]: value }), {})

const parsedQueryString = parseQueryString(queryString);

console.log(parsedQueryString)


Shortest distance from one word to another. eg: cat -> cot -> dot -> dog Lil' preparation before the interview. This is my solution to the problem in title. The code is pretty straight forward. Requesting suggestions for improvement and also verification of complexity, which I guess is O(26n)

public class CatToDog {

        static Set<String> dictionary;

        static {
            dictionary = new HashSet<String>();
            dictionary.add("camera");
            dictionary.add("cat");
            dictionary.add("cot");
            dictionary.add("cot");
            dictionary.add("dome");
            dictionary.add("dot");
            dictionary.add("dog");

    //        dictionary = new TreeSet<String>();
    //        BufferedReader br = null;
    //        try {
    //            try {
    //                br = new BufferedReader(new FileReader("/Users/ameya.patil/Desktop/text.txt"));
    //                String line;
    //                while ((line = br.readLine()) != null) {
    //                    dictionary.add(line.split(":")[0]);
    //                }
    //
    //            } finally {
    //                br.close();
    //            }
    //        } catch (Exception e) {
    //            throw new RuntimeException("Error while reading dictionary");
    //        }

        }

        private CatToDog () { };

        public static List<String> listWords(String startWord, String endWord) {
            final Queue<String> queue = new LinkedList<String>();
            final Map<String, String> backTrack = new HashMap<String, String>();
            queue.add(startWord);
            backTrack.put(startWord, null);

            while (!queue.isEmpty()) {
                String currentWord = queue.poll();
                if (currentWord.equals(endWord)) {
                    return mapToList(backTrack, endWord);
                }
                addValidOneChangeWords(currentWord, queue, backTrack);
            }
            return Collections.EMPTY_LIST;
        }

        private static void addValidOneChangeWords(String startWord, Queue<String> queue, Map<String, String> backTrack) {
            for (int i = 0; i < startWord.length(); i++) {
                char[] endWord = startWord.toCharArray();
                for (char ch = 'a'; ch < 'z'; ch++) {
                    endWord[i] = ch;
                    String word = new String(endWord);
                    if (validate(word, backTrack, startWord)) {
                        queue.add(word);
                        backTrack.put(word, startWord);
                    }
                }
            }
        }

        private static boolean validate(String word, Map<String, String> backTrack, String startWord) {
            return dictionary.contains(word) && !backTrack.containsKey(word) && !word.equals(startWord);
        }

        private static List<String> mapToList (Map<String, String> backTrack, String endWord) {
            final List<String> wordList = new ArrayList<String>();
            String word = endWord;
            while (backTrack.containsKey(word)) {
                wordList.add(word);
                word = backTrack.get(word);
            }
            Collections.reverse(wordList);
            return wordList;
        }

        public static void main(String[] args) {
           for (String string :  listWords("cat", "dog")) {
               System.out.println(string);
           }
        }
    }

I'm not sure about the exact complexity, but it is certainly huge! One-letter edits are fine to spell check one word given a large dictionary, but does not work if you only have a few words and the edit distance can be arbitrarily large. To look for close words in the dictionary, you should use the Levenshtein distance, as mentioned by ratchet freak. (Implementing the Levenshtein distance is a good idea.)

//        dictionary = new TreeSet<String>();
//        ....

Please don't post unused code here unless you want it reviewed too.

private CatToDog () { };

A default constructor is provided by Java already, only use that if you need to put something in there.

public static List<String> listWords(String startWord, String endWord) {
    final Queue<String> queue = new LinkedList<String>();
    final Map<String, String> backTrack = new HashMap<String, String>();
    queue.add(startWord);
    backTrack.put(startWord, null);

    while (!queue.isEmpty()) {
        String currentWord = queue.poll();
        if (currentWord.equals(endWord)) {
            return mapToList(backTrack, endWord);
        }
        addValidOneChangeWords(currentWord, queue, backTrack);
    }
    return Collections.EMPTY_LIST;
}

How do you know this is the shortest path? It feels a lot like depth first search, but I think you want to implement Dijkstra's algorithm instead to be able to notice shorter subpaths.

private static void addValidOneChangeWords(String startWord, Queue<String> queue, Map<String, String> backTrack) {
    for (int i = 0; i < startWord.length(); i++) {
        char[] endWord = startWord.toCharArray();
        for (char ch = 'a'; ch < 'z'; ch++) {
            endWord[i] = ch;
            String word = new String(endWord);
            if (validate(word, backTrack, startWord)) {
                queue.add(word);
                backTrack.put(word, startWord);
            }
        }
    }
}

You're only focusing on one-letter edits here, but what about deletions, insertions and even transpositions (dog -> dgo)? The Damerau-Levenshtein distance handles them.

And finally, mapToList is poorly named: I already know that the type change, I want to know about semantics! Something like editPath would be better.


Return a String containing only alphabets without spaces Scenario: Return a String that contains only alphabets without spaces.

input = "A man, a plan, a canal: Panama"

output = "AmanaplanacanalPanama"

Can you please critique my code?

private String OnlyAlphabets(final String string) {
    StringBuilder sb = new StringBuilder();
    char ch;

    for (int i = 0; i < string.length(); i++) {
        ch = string.charAt(i);
        if (!Character.isAlphabetic(ch)) {
            continue;
        }
        sb.append(ch);
    }
    return sb.toString();
}

char ch;

In general, we want to declare as late and at as small a scope as possible.  You never use this outside the loop, so it could just be

        char ch = string.charAt(i);

inside the loop.

Even better, use the range-based/foreach style:

    for (char character : string.toCharArray()) {

Now you don't have to worry about i or charAt at all.

If you're using Java 8, you may want to look into the streaming API.  E.g. combining this post and this post.

    return string.codePoints()
                 .filter( Character::isAlphabetic )
                 .collect( StringBuilder::new,
                           StringBuilder::appendCodePoint,
                           StringBuilder::append )
                 .toString();

I haven't tested this.  I just combined the two solutions to similar problems in a way that I believe does what you want.


Is this calculator written in Python clean/good? As a beginner I wrote my first program (calculator) with Python and as far as functionality is concerned it just works fine but I have some concerns about one thing at the end of the code. Under CalculateAgain() in the if statement, Calculate() is without the return statement. If I used it, it would just run the function one time and then it exits the program. That's why i left it without the statement. Is this ok or should I change something?

#Choose the operation
def OperationType():

    while True:

        Type = input('What type of operation do you want? +, -, *, /: \n')

        if Type in ('+', '-', '*', '/'):
            return Type
        else:
            print('Invalid input')

#Enter the first number
def InputFirstNumber():

    while True:
        try:
            firstNumber = float(input('Enter your first number: '))
            return firstNumber
        except ValueError:
            print ('Please enter a number')

#Enter the second number
def InputSecondNumber():

    while True:
        try:
            secondNumber = float(input('Enter your second number: '))
            return secondNumber
        except ValueError:
            print ('Please enter a number')

#Calculating
def Calculate():

    Type = OperationType()
    firstNumber = InputFirstNumber()
    secondNumber = InputSecondNumber()

    if Type == '+':
        print('{} + {} = '.format(firstNumber, secondNumber), firstNumber + secondNumber)

    elif Type == '-':
        print('{} - {} = '.format(firstNumber, secondNumber), firstNumber - secondNumber)

    elif Type == '*':
        print('{} * {} = '.format(firstNumber, secondNumber), firstNumber * secondNumber)

    elif Type == '/':
        print('{} / {} = '.format(firstNumber, secondNumber), firstNumber / secondNumber)

    else:
        print('Input not valid')

Calculate()

#Asks if the User want to calculate again
def CalculateAgain():

    while True:

        calculating_again = input('Do you want to calculate again? Y/N \n')

        if calculating_again in ('Y','y'):
            Calculate()

        elif calculating_again in ('N','n'):
            print('Quitting the program')
            break

        else:
            print('Not a valid answer')

CalculateAgain()

This code is broken into multiple unnecessary functions. Below is all thats needed:

operations = {
    "+": lambda x, y: x + y,
    "-": lambda x, y: x - y,
    "*": lambda x, y: x * y,
    "/": lambda x, y: x / y
}

def calculate():

    # Get numbers #
    one, two = input("Enter two numbers separated by a space: ").split()

    while operator := input("Enter an operator: + - * /"):
        if operator in ["+", "-", "*", "/"]:
            print(operations[operator](int(one), int(two)))
            return

Getting multiple values from input

Instead of prompting for each number, simply ask for two numbers separated by a space then split the input with a given delimiter, by default a space. .split returns a list, and since you only anticipate two values, can assign the output to one, two.

Walrus Operator

If you're using python-3.8, you can utilize the walrus operator, also called assignment expressions. It allows you to assign and return a value in an expression. This reduces the need to define a variable before the loop and use it.

Checking in

I do believe using a list/tuple instead of a string when checking in this particular case because it can catch some invalid input. A catch such as if operator in "+-*/":, it will allow "+-" as a valid operator, which we don't want. So good job there.

Lambdas

I've decided to use lambdas here because it's much easier to store them in a dictionary and then simply retrieve the value from the dictionary with the operator. If you want to learn more about how lambdas work, this Google Search brings up a plethora of sources that can explain the concept much better than I can.


Forced Async operation in a (event like) method I have a piece of code that I am not feeling comfortable.

I don't want to block the UI
Read operation works synchronously (reading may broke 50 ms rule)
Save operation supports async (returns Task)
I have to await SaveChanges (so operation could be completed before Dispose)

public Task AddToMyProject(string directory) {
    return Task.Run(async () => {
        if (!Directory.Exists(directory)) return;

        using (var context = _contextFactory.CreateContext()) {
            var existing = context.WatchDirectories.FirstOrDefault(wd => wd.Directory == directory);
            if (existing == null) {
                var childWatchers = context.WatchDirectories.Where(wd => wd.Directory.StartsWith(directory));
                foreach (var watchDirectory in childWatchers) {
                    context.Delete(watchDirectory);
                    RemoveDirectoryWatcher(watchDirectory.Directory);
                }

                context.Insert(new WatchDirectory { Directory = directory });
                await context.SaveChanges()
                    .ContinueWith(t => AddDirectoryWatcher(directory));
            }
        }
    });
}

I just want to know if there is a better approach for this.

Instead of WatchDirectories.FirstOrDefault you can make use of Any() which is more meaningful.

Also if you are checking Any() you can return early and save horizontal spacing.

public Task AddDirectoryWatcher(string directory) {
    return Task.Run(async () => {
        if (!Directory.Exists(directory)) return;

        using (var context = _contextFactory.CreateContext()) {
            if (context.WatchDirectories.Any(wd => wd.Directory == directory)) { return; };

            var childWatchers = context.WatchDirectories.Where(wd => wd.Directory.StartsWith(directory));

This AddDirectoryWatcher() is using a context to delete and insert items. Doesn't the RemoveDirectoryWatcher() method already uses a context too ? If yes, the call to context.Delete(watchDirectory); is redundant.

Instead of checking if the passed directory exists and silently returning , I would check this before calling the method and throw a DirectoryNotFoundException.


Implementing a linked list using C++ Any suggestions?

#include<iostream>

using namespace std;

class node
{
        friend class linked;

        private:
                int data;

        public:
                node *link;
                node()
                {
                        data=0;
                        link=NULL;

                }

};
class linked
{
        public:
                node *start=NULL;
                node *ptr;
                node *temp=NULL;

                void append(int item);
                void traverse();

};

void linked::append(int item)
{
        ptr = new node();
        ptr->data=item;
        if(start==NULL)
        {
                start=ptr;
                ptr->link=NULL;
                temp=start;

        }
        else
        {
                while(temp->link!=NULL)
                {
                     temp=temp->link;

                }
                temp->link=ptr;
                ptr->link=NULL;

         }

}
void linked::traverse()
{
        node *trav=start;
        while(trav!=NULL)
        {
                cout<<trav->data<<endl;
                trav=trav->link;
        }

}
int  main()
{
        int i;

        linked box;

        for(i=0;i<10;i++)
        {
           box.append(i);

        }

        box.traverse();
}

Starting at the top

//any suggestions

Yes, plenty. But don't add that to the code.

using namespace std;

Nearly every beginner includes this (and it's understandable because this is what they do in books).

In actual practice this is a bad idea. Never include this line in real code. The reason books include it is to save space (because each printed page costs extra). When writing real code, this will create bad habits that will bite you in the end so it is a habit you should break now.

see: Why is “using namespace std;” considered bad practice?

node class

protection level

There is no need to make the node class public. This is internal to your linked list so you should hide its implementation from other people by making it a private member class of the linked class.

style

class Node
{
        friend class linked;

        private:
                int data;

        public:
                Node *link;
                Node()
                {
                        data=0;
                        link=NULL;

                }

};

Small note on vertical spacing: there is way too much here. It's a small class so it does not stand out. But if you do the same thing on a bigger class it becomes hard to read. Now don't take this to the extreme and completely eliminate vertical space-- break things up into logical units.

Since we are making this class a private member of linked there's no need for friendship; just make everything public.

But we can improve the code by having a constructor that takes the data value and the link (if you look at your code you could use both at the same time. When writing constructors, it's preferred to use the initializer list syntax to initialize members:

                Node(int data, Node* link)
                    : data(data)
                    , link(link)
                {}

You will notice here that everything is lower case. It is relatively common to define user-defined types with an initial capital letter, while objects have an initial lower case letter. This allows you to quickly see the difference between types and objects (an important distinction in C++).

So, following this style, your code should look like this:

class Linked
{
    struct Node
    {
        int    data;
        Node*  next;
        Node(int data, Node* next)
            : data(data)
            , next(next)
        {}
    };

    ....

linked class

class linked
{
        public:
                Node *start=NULL;
                Node *ptr;
                Node *temp=NULL;

                void append(int item);
                void traverse();

};

Not sure what all the pointers are for? OK start marks the start of the list (usually called head). But what are the others supposed to represent? If they are really just things you use a throw away then they can be local to the function methods and not part of the class.

You call new in your code. For every call to new there must be a corresponding call to delete. Otherwise you will leak memory (and resources). So the best place to do this is in the destructor.

Also you are missing a constructor. This means that all you member variables are likely to be undefined (this means they have random values in them. C++ (unlike Java and other languages at this level) does not do automatic initialization of variables.

see: Why aren't pointers initialized with NULL by default?

So lets add the constructor and destructor

class Linked
{
    struct Node
    {
        int    data;
        Node*  next;
        Node(int data, Node* next)
            : data(data)
            , next(next)
        {}
    };

    Node*   head;

    Linked():
        head(nullptr)
    {}
    ~Linked()
    {
        Node* tmp;
        for(;head;head = tmp)
        {
            tmp = head->next;
            delete head;
        }
    }
    ...

If we look at append. You get the correct test for adding an element to an empty list. But it starts to go wrong in the second half where you are using a loop.

The member temp is not initialized. It is also public -- this means it could have been altered since last time you used it. So it is best to use a local variable that you initialize to head.

Note: you can also use the new constructor for node to reduce code in this function.

void Linked::append(int item)
{
    if (head == nullptr)
    {
        head = new Node(item, nullptr);
    }
    else
    {
        // First find a pointer to the last item in the chain.
        Node* last = head;
        while(last->next)
        {
             last = last->next;
        }
        // Now just add the node.
        last->next = new Node(item, nullptr);

        // Notice this loop happens each time you add an element.
        // This makes adding an element relatively expensive.
        // To avoid this a lot of implementation keep two pointers
        // in the list a `head` and a `tail`. That way data elements
        // can be quickly added to the tail.

        // When you start adding things like delete.
        // It becomes useful to know your previous element.
        // So it is also very common for the node object to have
        // two pointers `next` and `prev`
     }

Note: we always seem to place nullptr as the second argument. So we could potentially remove it. But if you try to implement an insert function then it becomes useful:

void Linked::insert_at_head(int value)
{
    head = new Node(item, head); // See very useful.
}

Not sure what your traverse is doing.

void linked::traverse()
{
        Node *trav=start;
        while(trav!=NULL)
        {
                cout<<trav->data<<endl;
                trav=trav->link;
        }

}

I would just name that print; traverse implies going through a tree.

One way to improve the functionality and modularity of traverse would be for it to take a parameter of a function that should be applied to all the nodes (but I think that is a bit beyond this exercise). So a couple of things here:

Rename to print
Mark the function as const
It does not alter the state of the object. So it is safe to call on a const version of the object.
Printing in C++ is usally done by operator<<
so create one of these that calls your print method.

Like this:

void Linked::print(std::ostream& str = std::cout) const // Note const
{
        Node *trav=start;
        while(trav!=NULL)
        {
                // Added space here so yo can see the <<
                // and what they are applied too.
                //
                // Also prefer "\n" to std::endl (especially in here)
                // The std::endl forces a buffer flush. Doing this
                // after printing every integer becomes very inefficient.
                str << trav->data << "\n";
                trav=trav->next;
        }
}
std::ostream& operator<<(std::ostream& str, Linked const& data)
{
     data.print(str);
     return str;
}

I think we have now covered enough in one review. I would have another bash at this example. Once you have done that post the code for review again and we can go into some other details.


Generic copy function Common Lisp does not have a generic copy function for reasons outlined in Kent Pitman's article The Best of Intentions. However, it may still be convenient for rapid prototyping purposes to provide such a function that would recursively copy any given lisp object down to its immutable core objects. In the initial stages of development, this could obviate the need to write or import specialized copiers for complex sequences, arrays, hash tables, and structures.

The following generic methods attempt to specify a function called ucopy (for universal copy) that implements universal recursive copying. An example of use might be to copy an array composed of structures containing a hash table: (setq array1 (ucopy array0)). As this is my first attempt at writing a library(?), I would appreciate corrections, improvements, or any other instructive comments. Thanks.

;;;; Filename: universal-copy.lisp

;;; Deep (recursive) copy of a Common Lisp object.
;;; For example, (ucopy array0) where array0 is an array of structures.
;;; Assumes MOP is loaded.

(defmethod ucopy ((sym symbol))
  "Simply return the symbol."
  sym)

(defmethod ucopy ((num number))
  "Simply return the number."
  num)

(defmethod ucopy ((char character))
  "Simply return the character."
  char)

(defmethod ucopy ((fn function))
  "Simply return the function."
  fn)

(defmethod ucopy ((path pathname))
  "Simply return the path."
  path)

(defmethod ucopy ((seq sequence))
  "Copy a sequence recursively."
  (map (type-of seq) #'ucopy seq))

(defmethod ucopy ((ht hash-table))
  "Copy a hash table recursively."
  (loop with new-ht = (make-hash-table
                        :test (hash-table-test ht)
                        :size (hash-table-size ht)
                        :rehash-size (hash-table-rehash-size ht)
                        :rehash-threshold (hash-table-rehash-threshold ht))
      for key being the hash-key in ht using (hash-value value)
      do (setf (gethash (ucopy key) new-ht) (ucopy value))
      finally (return new-ht)))

(defmethod ucopy ((arr array))
  "Copy an array recursively."
  (let ((new-arr (make-array (array-dimensions arr)
                             :element-type (array-element-type arr)
                             :adjustable (adjustable-array-p arr))))
    (dotimes (i (array-total-size arr))
      (setf (row-major-aref new-arr i)
        (ucopy (row-major-aref arr i))))
    new-arr))

(defmethod ucopy ((struct structure-object))
  "Copy a structure recursively."
  (let ((new-struct (copy-structure struct))
        (slots (class-direct-slots (class-of struct))))
    (dolist (slot slots)
      (let ((slot-name (slot-definition-name slot)))
        (setf (slot-value new-struct slot-name)
          (ucopy (slot-value struct slot-name)))))
    new-struct))

(defmethod ucopy ((inst standard-object))
  "Copy an instance of a class recursively."
  (let ((new-inst (allocate-instance (class-of inst)))
        (slots (class-direct-slots (class-of inst))))
    (dolist (slot slots)
      (let ((slot-name (slot-definition-name slot)))
        (when (slot-boundp inst slot-name)
          (setf (slot-value new-inst slot-name)
            (ucopy (slot-value inst slot-name))))))
    new-inst))

Given the restrictions you outline that seems sensible, I also couldn't
find a CDR for it (in contrast to an extensible equality operator for
example), however I'd probably try to find a better name than UCOPY.
Also, since you often might have to choose between shallow and deep
copies, perhaps introduce a parameter for that.

The method for STRUCTURE-OBJECT isn't guaranteed to work, so it might
be more advisable to leave out that method and require the user to
implement them by hand.

Rest looks fine to me.


Entry Validation in Xamarin.Forms I have an Entry that I want to give a red outline when the entry is empty. I'm using SyncFusion's SFTextInputLayout for my Entry and it has a property HasError that once it's set to true, it'll automatically highlight my entire in Red.

Here is the following XAML code for SFTextInputLayout

 <inputLayout:SfTextInputLayout Grid.Column="0" Hint="Phone Number" ContainerType="{StaticResource RepairOrderContainerType}" HasError="{Binding IsPhoneNumberError}" FocusedColor="{StaticResource AccentColor}" VerticalOptions="Center" HorizontalOptions="Start">
       <Entry Keyboard="Telephone" Style="{StaticResource TextInputStyle}" Text="{Binding PhoneNumber}"/>
 </inputLayout:SfTextInputLayout>

As you can see, I have two bindings that handles the text of the entry and another one to check if it has an error or not.

While this solution works, it will get redundant pretty soon as the number of my entry fields grow. For every entry field I have, I need another boolean to cover its Error property as shown below.

  private string _phoneNumber;
    public string PhoneNumber
    {
        get => _phoneNumber;
        set
        {
            IsPhoneNumberError = string.IsNullOrWhiteSpace(value) ? true : false;
            this.RaiseAndSetIfChanged(ref _phoneNumber, _phoneNumber);
        }
    }

    private bool _isPhoneNumberError = false;
    public bool IsPhoneNumberError
    {
        get => _isPhoneNumberError;
        set
        {
            this.RaiseAndSetIfChanged(ref _isPhoneNumberError, value);
        }
    }

I'm wondering how to simplify this code?

You could have a collection of ValueConverters which would validate your TextInputs' strings. For example one could be a PhoneNumberValidator or PhoneToBoolConverter (example names there). This way HasError would bind to HasError="{Binding PhoneNumber, Converter={StaticResource PhoneNumberValidator}}". Converters are handy and you can reuse them in multiple places around your code if they are declared in your App.xaml.cs for example.


Longest non-decreasing subsequence, in Haskell OK, so I recently posted this solution in Python to the longest non-decreasing subsequence problem, and cleaned it up with some expert advice.  As a next step, I wanted to translate this solution into Haskell.  This presents a challenge because the algorithm as originally formulated depends heavily on mutable arrays with O(1) lookup and update behavior, which are standard in Python but a little harder to come by in Haskell.

Some excellent answers on Stackoverflow pointed me towards Data.Seq as a mutable, indexable array type, and Numeric.Search.Range for a generic binary search operation (which is key to making the algorithm efficient).

I wanted to express this algorithm as a fold over the input list because that seemed like a natural way to do it.  However the Python version depends on many O(1) lookups to arbitrary places in the input list - not really possible within a Haskell fold over an ordinary list.  I saw that this could be avoided by, rather than doing what the Python version does - namely storing indices into the input array of "the best subsequence of length n found so far" - storing the best actual subsequence as a Haskell list.  At first sight, this seems incredibly wasteful, because at the end of the calculation for an input list of length n there could be as many as n "best subsequence" lists which are on average n / 2 long, creating space requirements of order n^2!  However, since these are Haskell lists derived from each other, they actually share a lot of the same space... for example, in the case where the input list is already a non-decreasing list of length n, the calculation will involve creating n subsequence lists of lengths [1..n], but each one shares a tail with its predecessor and the total space requirement is just proportional to n.

The code is below.  What I am looking for is: 1) general comments on readability, correctness, etc. and 2) a way to prove an upper bound on the space requirement.

import Data.Sequence as DS
import Numeric.Search.Range

seqLast::Seq a -> a
seqLast xs = index xs ((DS.length xs) - 1)

-- Return the longest non-decreasing subsequence of an input sequence of integers
nonDecreasing::[Int]->[Int]
nonDecreasing = Prelude.reverse . seqLast . makeM
  where makeM = foldl updateM empty
          where updateM m x | DS.null m = singleton [x]
                            | insert_idx == Nothing = m |> (x:(seqLast m))
                            | just_insert_idx == 0 = update 0 [x] m
                            | otherwise = update just_insert_idx (x:(index m (just_insert_idx - 1))) m
                  where insert_idx = searchFromTo (\idx -> x < (head (index m idx))) 0 ((DS.length m) - 1)
                        just_insert_idx = maybe 0 id insert_idx

A few general remarks

I find that stepped whiles are as evil as nested if statements for readability of the code. Here, you have three levels of nesting.
Relying on the surrounding environment for variables can some times obscure possibility to generalize (You make use of x and m for inner definitions)

So here is what I would do

import Data.Sequence as DS
import Numeric.Search.Range as R

seqLast::Seq a -> a
seqLast xs = index xs ((DS.length xs) - 1)

insert_idx :: Ord a => a -> Seq [a] -> Maybe Int
insert_idx y m = searchFromTo (fn y) 0 $ (DS.length m) - 1
  where fn y idx = y < (head (index m idx))

-- Return the longest non-decreasing subsequence of an input sequence of integers
nonDecreasing::[Int] -> [Int]
nonDecreasing = Prelude.reverse . seqLast . makeM
  where makeM = foldl updateM empty
        updateM m x | DS.null m = singleton [x]
                    | otherwise = case insert_idx x m of
                        Nothing -> m |> (x: seqLast m)
                        Just l -> update l (x:geti l m) m
        geti 0 _ = []
        geti l m = index m (l -1)


Basic PHP Factory Pattern I have an object, KbArticle, that originally in it's constructor, would try to return null if the SQL query that retrieves the data for its required properties came up with 0 rows. A friend told me that constructors shouldn't return null, and that I should look into the Factory Pattern. I did and I see the benefits, so I'm wondering how I can implement this better/"properly."

Quick overview: Knowledgebase.php is the page an end-user sees, it calls a function in  factory.php which calls the constructor in kb_fnc.php and then it ends up creating the object back on Knowledgebase.php if things all go well.

Knowledgebase.php

<?php
if ($kb = Factory::make('KbArticle', $_GET['a'])) {
    $kb->echoNavTrail();
} else {
    die('failed constructor');
}
?>

factory.php

Note: It disappoints me that apparently the functions in here have to be public instead of private, because my factory method is static? This seems not ideal, or am I incorrect?

<?php

require_once('kb/kb_fnc.php');

class Factory {
    public static function make($obj, $params) {
        $succ = false;
        switch($obj) {
            case('KbArticle'):
                return tryNewKnowledgebaseArticle($params);
                break;
        }
    }
}

function tryNewKnowledgebaseArticle($id) {
        try {
            $kb = new KbArticle($id);
        } catch (Exception $e) {
            return null;
        }
        return $kb;
    }
?>

kb_fnc.php

Note: Is throwing an exception here smart? In my quick tests yesterday, I couldn't access the custom string on the exception after it was chained back, so it seems not that useful—at least not without knowing how to log that string to console at the end or something, for debugging purposes.

class KbArticle {
    function __construct($id) {
        //do SQL
        if (odbc_num_rows($res) > 0) {
            //fill in properties with data from table
        } else {
            throw new Exception('KbA constructor failed SQL query');
        }
    }
}

My biggest questions are regarding:

Does this look sensible?
Can I access that custom exception string in my factory class, so I can ultimately encode it in JSON for a console.log for easier debugging if things go awry down the line?
The functions in my factory class being public instead of encapsulated as private bothers me.

My eyes hurt :(
I don't think you fully understand OO. Let alone the factory pattern.
The factory pattern is a pattern that helps us with creating objects. It is best used when creating an object takes a couple of steps or is very hard, for instance a constructors with many vars.
This is however not your problem. In fact, I don't really see a problem - a part from you not understanding OO.
You created a Factory that is responsible for creating objects. Yet the only thing it does, is call a function and catche an Exception.
This is not a factory.
What you are doing is creating objects that internally only call functions in the global scope. That is called abuse!
Back to your real question, the pattern:
What I think would be a good pattern to use is the Repository pattern. Before I explaing the repository pattern, let's first review your code:
<?php
if ($kb = Factory::make('KbArticle', $_GET['a'])) {
    $kb->echoNavTrail();
} else {
    die('failed constructor');
}
?>

Don't use the closing ?> tag. Just don't
Use coding standards (PSR-1 and PSR-2 )
Use Exceptions, don't just die.
Use better variable names

Better code would look like this:
<?php

try
{
    $whatEverAIs = $_GET['a']; //what is a?
    //we should check or A is of a correct value, int? string?

    $kbArticle = KbArticleFactory::make($whatEverAIs)
}
catch( Exception $e )
{
    print $e->getMessage(); //or log the exception or show a nice debug screen or ...
    die();
}

//still here? good, we now can use our $kbArticle
print $kbArticle->getNavigationTrail();

This code reads better, and gives us a better view of what is going on. We have a KbArticleFactory that returns a KbArticle. We create a KbArticle with variable $whatEverAIs. If it fails, we catch the exception and do what ever we want to do with it. If it succeeds, we simply continue with our magic.
The design is bad
Let's analyze our design. We have a Factory for KbArticle's that throws an exception if the KbArticle couldn't be created. If look deeper into your code, this means that you KbArticle wasn't found. and $a looks to be some kind of KbArticleId.
wow, I don't think you need to be einstein to notice something fishy in that explanation. Let's write down our problem.
We have a DataSource (SQL) with records in it. Every record is in fact a KbArticle. In stead of manually having to query the database all the time, we want some kind of interface that does all the queriyng for us, and returns a KbArticle object.
Enter the Repository pattern.
We have a repository:
<?php
interface RepositoryInterface {

    public function all();

    public function find($id);

    public function create($data);

    public function update($id, $data);
}

this gives us a set of methods to interact with things. So let's create our KbArticle interface:
<?php

//because we don't need extra methods for our KbArticleRepository (yet) we simply only extend our existing interface
interface KbArticleRepositoryInterface extends RepositoryInterface {}

So, let's create our repository:
<?php

class KbArticleRepository implements KbArticleRepositoryInterface
{
    public function __construct($dataSource)
    {
        $this->dataSource = $dataSource;
    }

    public function all()
    {
        $kbArticles = $this->dataSource($sql_for_all_articles);
        return $kbArticles;
    }

    public function find($id)
    {
        $kbArticle = $this->dataSource($sql_for_this_article);

        if ( is_null($kbArticle) )
        {
            throw new KbArticleNotFoundException();
        }

        return $kbArticle;
    }

    //...
}

We would then use this repository as follows:
<?php

$kbArticleId = $_GET['a'];
if ( preg_match('[0-9]+',$kbArticleId) === 0 )
{
    throw new Exception('a needs to be an int'); // let some other code handle the error
}

$sqlDataSource = new SqlDataSource();//get the datasource

$repository = new KbArticleRepository($mySqlDataSource);

try
{
    $kbArticle = $repository->find($kbArticleId);
}
catch (KbArticleNotFoundException $e)
{
    print 'Article not found';
    die();
}

//still here? good, we now can use our $kbArticle
print $kbArticle->getNavigationTrail();

Depending on how hard it is to create our KbArticle object. We can simply do it in our repository and return that object:
public function find($id)
{
    //...
    return new KbArticle($kbArticle);
}

Or we could delegate it to a factory:
public function __construct($datasource, $factory)
{
    //...
    $this->factory = $factory;
}

public function find($id)
{
    //...
    return $this->factory->make($kbArticle);
}

I hope this all makes a bit sense.


Finding max, min, mode Could you review my code for finding max, min, and mode?

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

int main()
{
    std::vector <int> v = { 2, 5, 3, 4, 3, 2, 3, 3, 3 , 6, 6, 6, 6, 6, 6, 6, 6, 1};
    sort(v.begin(), v.end());
    int number = v[0];
    int max = v[0];
    int min = v[0];
    int mode;
    for (int i = 1, countMode = 1, count = 1; i < v.size(); ++i) {
        if (v[i] == number)
            ++countMode;
        if (countMode > count) {
            count = countMode;
            mode = number;
        }
        if (v[i] > max)
            max = v[i];
        if (v[i] < min)
            min = v[i];
        number = v[i];
    }
    std::cout << max << ' ' << min << ' ' << mode;
}

Here are some things that may help you improve your code.

Use direct initialization

In the first line, we have this:

std::vector <int> v = { 2, // ...

However, this may be shortened slightly by simply omitting the =:

std::vector <int> v{ 2, // ...

This is list initialization which was standardized in C++11.

Use "range-for" to simplify the loop

Instead of using this somewhat complicated for loop:

for (int i = 1, countMode = 1, count = 1; i < v.size(); ++i) {

You could instead use a range-for:

for (const auto n : v) {

Avoid doing needless calculations

By definition, in a sorted list the first number will be min and the last max, so there's not much point in making those calculations.

Use better naming

Variables min and max are good because they give a good clue as to what they hold.  However, number might be better called prev since it's the previous number in the sorted list.

Here's one way to do all of that:

int max = v.back();
int min = v.front();
int prev = max;
int mode;
int maxcount = 0;
int currcount = 0;
for (const auto n : v) {
    if (n == prev) {
        ++currcount;
        if (currcount > maxcount) {
            maxcount = currcount;
            mode = n;
        }
    } else {
        currcount = 1;
    }
    prev = n;
}


Check if each element of my list param contains all data string passed as a parameter My code below is working but I am searching for performance and improvement.
It consists of looping my PartList  elements and check if my string parameter orginalDataStringList contains the elements on label string for each Part element.

private boolean excludeAllInformationVariation(List<String> orginalDataStringList, PartList parts) {
    for (Part part : parts) {
        List<String> listTemp = new ArrayList<String>();
        listTemp.addAll(orginalDataStringList);
        if (!part.getOptLogic().isLabelNullOrEmpty()) {
            String[] dataArrays = part.getOptLogic().getLabel().split("( )");
            for (String data : dataArrays) {
                listTemp.remove(data);
            }
        } else {
            return false;
        }
        if (!listTemp.isEmpty()) {
            return false;
        }
    }
    return true;
}

public class PartList extends ArrayList<Part> {
    public PartList() {
        //Empty constructor
    }
}

public class Part {
    private Logic optLogic;
    public Logic getOptLogic() { return optLogic; }
    public void setOptLogic(Logic optLogic) { this.optLogic = optLogic; }
}

public class Logic {
    private String label;
    public String getLabel() { return label; }
    public void setLabel(String label) { this.label = label; }
}
public boolean isLabelNullOrEmpty() {
    return (this.getLabel() == null || this.getLabel().isEmpty()) ? true : false;
}

Thanks for sharing your code.

Naming

Finding good names is the most important but also hardest part in programming. So always take your time to find good names that express your intention of what the code does.

The name of your method excludeAllInformationVariation is a lie. It implies that there is something changed either in the object itself (its state) or the passed parameter object. But this method does not alters neither the objects stat nor the parameter.
Therefore it should have a different name.
Since it returns a boolean this name should start with is or has.
eg.: isAllDataCovered(...)

Single Responsibility / Separation of Concerns

Your  classes Part and Logic look like  data transfer objects (DTOs), but the method isLabelNullOrEmpty() is some kind of business logic.

You should not mix DTOs with business logic. The method isLabelNullOrEmpty() should be in the same class as the method  excludeAllInformationVariation and have a parameter of type String on which it should work.


Playing sound on a button click I am new to javascript and want to make sure I am on the right track when playing a sound. Is there anything I should not be doing better or not be doing at all. Below is a simple function.

  Click here to hear a sound

I am not sure whether I should be using the audio tag or not. I appreciate any feedback

Regarding the HTML:

script element

For JavaScript, you can simply use <script> without the type attribute:

  If the language is not that described by "text/javascript", then the type attribute must be present

The language attribute is obsolete:

  Authors should not specify a language attribute on a script element.

a element

I'd say a real button should be used here, not a link:

button element with button type: <button type="button" onclick="…">Click here to hear a sound</button>
input element with button type: <input type="button" onclick="…" value="Click here to hear a sound" />


Multiple AFNetworking calls In a view controller Multiple  AFNetworking calls In a view controller.
The code is very wield.

I think , to negotiate with the server , and combine those into one url, instead of many urls.

@interface HomeViewController ()

#pragma mark - init user info
- (void) initUserData
{
    [self get_userInfo];// user login info about
    [self getRule];// get ship rule
    [self getAppSystem];
    [self getPunchCardInfo]; // get work register stauts
    [CardTypePickerView updateCardTypes:nil]; //  get id type
    [self getProblemTypes];
    [self getFetch_ad];/ get ad
    [self updateSMSCount];// update small messages
    [CGAreaData updateAddressList:NO failure:^(NSString *failure) {}];// update province , city , district
}

#pragma mark -- user login info about
- (void)get_userInfo
{
    __block UserInfoModel *userInfo = [UserInfoModel getUserInfoModel];
    [NetWorkManager info_userSuccess:^(id result, NSString *message) {
        userInfo = [UserInfoModel getUserInfoModel];
        if ([UiUtil checkIsNull:result] == NO) {
            [userInfo setValuesForKeysWithDictionary:result];
            [userInfo saveUserInfoModel];
            [self.headerTaskView setName:userInfo.realName];
        }
    } failure:nil];
    [self.headerTaskView setName:userInfo.realName];
}

#pragma mark --  get ship rule
- (void)getRule
{
    [NetWorkManager getBillRuleWithSuccess:^(NSDictionary *dict) {
        if (![UiUtil checkIsNull:dict]) {
            [[EGOCache globalCache] setObject:dict forKey:EGO_BAR_RULE];
        }

    } failure:^(NSString *errmsg) {
    }];
}

#pragma mark -- get app version
- (void)getAppSystem
{
    [NetWorkManager app_systemWithSuccess:^(NSDictionary *result, NSString *message) {
        if (![UiUtil checkIsNull:result]) {
            [[EGOCache globalCache] setObject:result forKey:EGO_APP_SYSTEM_RULE];
            // check if new version exists
            [VersionCheck versionCheckComplete:result[@"VERSION_IOS"]];

        }

    } failure:^(NSString *errmsg) {

       // check if new version exists
    }];
}

......
// network request info etc

Under the hood,  those functions all use AFNetworking

AFHTTPSessionManager *manager = [AFHTTPSessionManager manager];
AFHTTPRequestSerializer *requestSerializer = [AFHTTPRequestSerializer serializer];

Any suggestion to optimize these batch requests?

What is the harm of code like this? I think it is the battery consuming.

There is nothing wrong with multiple network requests, it will not have any impact on battery that you need to worry about.

Your code however needs a lot of work. None of these network requests should be happening inside a ViewController. It's purpose is to configure the View and handle user actions, nothing else.